Re: [xwiki-devs] [xwiki-notifications] r9071 - xwiki-platform/core/branches/xwiki-core-1.3/xwiki-core/src/main/java/com/xpn/xwiki/store
Hi JV, Are you sure about this patch? It looks like it's adding a synchronize() that was not there before and thus all threads will be waiting on this synchronize. Since checkHibernate is called for all database access I'm worried this could lead to performance issues. Has this been tested for performance with a tool like JMeter to simulate multiple users? Note that I haven't analyzed the patch. I'm just worried that we might be introducing a regression in term of performance. If the session factory is only created once then maybe it should be created on app init? Thanks -Vincent On Apr 10, 2008, at 6:21 PM, jvdrean (SVN) wrote:
Author: jvdrean Date: 2008-04-10 18:21:07 +0200 (Thu, 10 Apr 2008) New Revision: 9071
Modified: xwiki-platform/core/branches/xwiki-core-1.3/xwiki-core/src/main/ java/com/xpn/xwiki/store/XWikiHibernateBaseStore.java Log: XWIKI-2300 : HibernateStore synchronization problem
Applied patch by Raffaello Pelagalli without modification. Added comment.
Modified: xwiki-platform/core/branches/xwiki-core-1.3/xwiki-core/src/ main/java/com/xpn/xwiki/store/XWikiHibernateBaseStore.java =================================================================== --- xwiki-platform/core/branches/xwiki-core-1.3/xwiki-core/src/main/ java/com/xpn/xwiki/store/XWikiHibernateBaseStore.java 2008-04-10 14:45:54 UTC (rev 9070) +++ xwiki-platform/core/branches/xwiki-core-1.3/xwiki-core/src/main/ java/com/xpn/xwiki/store/XWikiHibernateBaseStore.java 2008-04-10 16:21:07 UTC (rev 9071) @@ -510,13 +510,18 @@ */ public void checkHibernate(XWikiContext context) throws HibernateException { - + // Note : double locking is not a recommended pattern and is not guaranteed to work on all + // machines. See for example http://www.ibm.com/developerworks/java/library/j-dcl.html if (getSessionFactory() == null) { - initHibernate(); - - /* Check Schema */ - if (getSessionFactory() != null) { - updateSchema(context); + synchronized(this) { + if (getSessionFactory() == null) { + + initHibernate(); + /* Check Schema */ + if (getSessionFactory() != null) { + updateSchema(context); + } + } } } }
_______________________________________________ notifications mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/notifications
On Thu, Apr 10, 2008 at 6:31 PM, Vincent Massol <[email protected]> wrote:
Hi JV,
Are you sure about this patch?
It looks like it's adding a synchronize() that was not there before and thus all threads will be waiting on this synchronize. Since checkHibernate is called for all database access I'm worried this could lead to performance issues.
The synchronized is done only when the sessionFactory has not been created (and it's created once).
Has this been tested for performance with a tool like JMeter to simulate multiple users?
XWiki.org is currently running with this patch.
If the session factory is only created once then maybe it should be created on app init?
Thanks -Vincent
On Apr 10, 2008, at 6:21 PM, jvdrean (SVN) wrote:
Author: jvdrean Date: 2008-04-10 18:21:07 +0200 (Thu, 10 Apr 2008) New Revision: 9071
Modified: xwiki-platform/core/branches/xwiki-core-1.3/xwiki-core/src/main/ java/com/xpn/xwiki/store/XWikiHibernateBaseStore.java Log: XWIKI-2300 : HibernateStore synchronization problem
Applied patch by Raffaello Pelagalli without modification. Added comment.
Modified: xwiki-platform/core/branches/xwiki-core-1.3/xwiki-core/src/ main/java/com/xpn/xwiki/store/XWikiHibernateBaseStore.java =================================================================== --- xwiki-platform/core/branches/xwiki-core-1.3/xwiki-core/src/main/ java/com/xpn/xwiki/store/XWikiHibernateBaseStore.java 2008-04-10 14:45:54 UTC (rev 9070) +++ xwiki-platform/core/branches/xwiki-core-1.3/xwiki-core/src/main/ java/com/xpn/xwiki/store/XWikiHibernateBaseStore.java 2008-04-10 16:21:07 UTC (rev 9071) @@ -510,13 +510,18 @@ */ public void checkHibernate(XWikiContext context) throws HibernateException { - + // Note : double locking is not a recommended pattern and is not guaranteed to work on all + // machines. See for example http://www.ibm.com/developerworks/java/library/j-dcl.html if (getSessionFactory() == null) { - initHibernate(); - - /* Check Schema */ - if (getSessionFactory() != null) { - updateSchema(context); + synchronized(this) { + if (getSessionFactory() == null) { + + initHibernate(); + /* Check Schema */ + if (getSessionFactory() != null) { + updateSchema(context); + } + } } } }
_______________________________________________ notifications mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/notifications
_______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
-- Jean-Vincent Drean
Vincent Massol wrote:
Hi JV,
Are you sure about this patch?
It looks like it's adding a synchronize() that was not there before and thus all threads will be waiting on this synchronize. Since checkHibernate is called for all database access I'm worried this could lead to performance issues.
This should not affect performance as the synchronize() will take place only during the first request, when hibernate is initialized.
Has this been tested for performance with a tool like JMeter to simulate multiple users?
Note that I haven't analyzed the patch. I'm just worried that we might be introducing a regression in term of performance.
If the session factory is only created once then maybe it should be created on app init?
+1. We need to initialize things on startup, and not on requests.
Thanks -Vincent
On Apr 10, 2008, at 6:21 PM, jvdrean (SVN) wrote:
Author: jvdrean Date: 2008-04-10 18:21:07 +0200 (Thu, 10 Apr 2008) New Revision: 9071
Modified: xwiki-platform/core/branches/xwiki-core-1.3/xwiki-core/src/main/ java/com/xpn/xwiki/store/XWikiHibernateBaseStore.java Log: XWIKI-2300 : HibernateStore synchronization problem
Applied patch by Raffaello Pelagalli without modification. Added comment.
Modified: xwiki-platform/core/branches/xwiki-core-1.3/xwiki-core/src/ main/java/com/xpn/xwiki/store/XWikiHibernateBaseStore.java =================================================================== --- xwiki-platform/core/branches/xwiki-core-1.3/xwiki-core/src/main/ java/com/xpn/xwiki/store/XWikiHibernateBaseStore.java 2008-04-10 14:45:54 UTC (rev 9070) +++ xwiki-platform/core/branches/xwiki-core-1.3/xwiki-core/src/main/ java/com/xpn/xwiki/store/XWikiHibernateBaseStore.java 2008-04-10 16:21:07 UTC (rev 9071) @@ -510,13 +510,18 @@ */ public void checkHibernate(XWikiContext context) throws HibernateException { - + // Note : double locking is not a recommended pattern and is not guaranteed to work on all + // machines. See for example http://www.ibm.com/developerworks/java/library/j-dcl.html if (getSessionFactory() == null) { - initHibernate(); - - /* Check Schema */ - if (getSessionFactory() != null) { - updateSchema(context); + synchronized(this) { + if (getSessionFactory() == null) { + + initHibernate(); + /* Check Schema */ + if (getSessionFactory() != null) { + updateSchema(context); + } + } } } }
-- Sergiu Dumitriu http://purl.org/net/sergiu/
On Apr 10, 2008, at 6:42 PM, Sergiu Dumitriu wrote:
Vincent Massol wrote:
Hi JV,
Are you sure about this patch?
It looks like it's adding a synchronize() that was not there before and thus all threads will be waiting on this synchronize. Since checkHibernate is called for all database access I'm worried this could lead to performance issues.
This should not affect performance as the synchronize() will take place only during the first request, when hibernate is initialized.
Yes you're right, I hadn't read correctly... Thanks -Vincent
Has this been tested for performance with a tool like JMeter to simulate multiple users?
Note that I haven't analyzed the patch. I'm just worried that we might be introducing a regression in term of performance.
If the session factory is only created once then maybe it should be created on app init?
+1. We need to initialize things on startup, and not on requests.
Thanks -Vincent
On Apr 10, 2008, at 6:21 PM, jvdrean (SVN) wrote:
Author: jvdrean Date: 2008-04-10 18:21:07 +0200 (Thu, 10 Apr 2008) New Revision: 9071
Modified: xwiki-platform/core/branches/xwiki-core-1.3/xwiki-core/src/main/ java/com/xpn/xwiki/store/XWikiHibernateBaseStore.java Log: XWIKI-2300 : HibernateStore synchronization problem
Applied patch by Raffaello Pelagalli without modification. Added comment.
Modified: xwiki-platform/core/branches/xwiki-core-1.3/xwiki-core/ src/ main/java/com/xpn/xwiki/store/XWikiHibernateBaseStore.java =================================================================== --- xwiki-platform/core/branches/xwiki-core-1.3/xwiki-core/src/main/ java/com/xpn/xwiki/store/XWikiHibernateBaseStore.java 2008-04-10 14:45:54 UTC (rev 9070) +++ xwiki-platform/core/branches/xwiki-core-1.3/xwiki-core/src/main/ java/com/xpn/xwiki/store/XWikiHibernateBaseStore.java 2008-04-10 16:21:07 UTC (rev 9071) @@ -510,13 +510,18 @@ */ public void checkHibernate(XWikiContext context) throws HibernateException { - + // Note : double locking is not a recommended pattern and is not guaranteed to work on all + // machines. See for example http://www.ibm.com/developerworks/java/library/j-dcl.html if (getSessionFactory() == null) { - initHibernate(); - - /* Check Schema */ - if (getSessionFactory() != null) { - updateSchema(context); + synchronized(this) { + if (getSessionFactory() == null) { + + initHibernate(); + /* Check Schema */ + if (getSessionFactory() != null) { + updateSchema(context); + } + } } } }
-- Sergiu Dumitriu http://purl.org/net/sergiu/ _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
Vincent Massol <[email protected]> writes:
Hi JV,
Are you sure about this patch?
It looks like it's adding a synchronize() that was not there before and thus all threads will be waiting on this synchronize. Since checkHibernate is called for all database access I'm worried this could lead to performance issues.
Has this been tested for performance with a tool like JMeter to simulate multiple users?
Note that I haven't analyzed the patch. I'm just worried that we might be introducing a regression in term of performance.
If the session factory is only created once then maybe it should be created on app init?
Thanks -Vincent
It has been tested with siege (50 users at the same time), profiled with yourkit, and is now on xwiki.org which seems to be faster than before ;) -- Raffaello Pelagalli XPertNet - XWiki.com
Raffaello Pelagalli wrote:
It has been tested with siege (50 users at the same time), profiled with yourkit, and is now on xwiki.org which seems to be faster than before ;)
Maybe faster than before, but still quite slow... -- Sergiu Dumitriu http://purl.org/net/sergiu/
participants (4)
-
Jean-Vincent Drean -
Raffaello Pelagalli -
Sergiu Dumitriu -
Vincent Massol