[xwiki-devs] Call for review thread safety new Component Manager
Hi guys, I'm trying to reduce the enormous number of lock done by the current implementation of Embedded Component Manager and I could use some more brains to check what I did. You can find the new version on https://github.com/xwiki/xwiki-commons/blob/feature-improvecm/xwiki-commons-... Quick comparison with the current one: * removed all "synchronized (this)" that was in pretty much all the methods and which made the CM block everything whenever anyone was using it * used ConcurrentHashMap to securize the insertion/search of components * basically the only remaining explicit lock is on singleton component initialization mostly to make sure we don't initialize the same component at the same time If you find some small modification to improve a bit the speed don't hesitate too. There is possible improvements to speed up lookuplist and lookupmap (not going though the whole map basically) but it require important refactoring and I would prefer not doing too much things at once since it's a pretty critical peace of code and locking is the main issue right now. Thanks, -- Thomas Mortagne
Hi Thomas, On Nov 27, 2011, at 6:19 PM, Thomas Mortagne wrote:
Hi guys,
I'm trying to reduce the enormous number of lock done by the current implementation of Embedded Component Manager and I could use some more brains to check what I did.
You can find the new version on https://github.com/xwiki/xwiki-commons/blob/feature-improvecm/xwiki-commons-...
Quick comparison with the current one: * removed all "synchronized (this)" that was in pretty much all the methods and which made the CM block everything whenever anyone was using it * used ConcurrentHashMap to securize the insertion/search of components * basically the only remaining explicit lock is on singleton component initialization mostly to make sure we don't initialize the same component at the same time
If you find some small modification to improve a bit the speed don't hesitate too. There is possible improvements to speed up lookuplist and lookupmap (not going though the whole map basically) but it require important refactoring and I would prefer not doing too much things at once since it's a pretty critical peace of code and locking is the main issue right now.
Cool. What's the plan? I feel this is a potentially dangerous commit for a RC1 so I'm not entirely comfortable having this in 3.3 as of now. What I'd propose is this: - try it on xwiki.org as part of our upgrade of xwiki.org to see if it helps (since we're blocked on this upgrade) - depending on the result decide if the benefits are greater than the risks for 3.3 final (I still think we might not have enough data to decide about this since this would need to run for a while to be sure it's ok - We also think that the issue on xwiki.org is not really with the CM so it might not help fix the issue we have with the 3.3M2 upgrade on xwiki.org). - maybe install it on myxwiki.org and let it run for some time and then commit it in a 3.3.1/3.4M1 if there's no problem? WDYT? Thanks -Vincent
On Mon, Nov 28, 2011 at 9:10 AM, Vincent Massol <[email protected]> wrote:
Hi Thomas,
On Nov 27, 2011, at 6:19 PM, Thomas Mortagne wrote:
Hi guys,
I'm trying to reduce the enormous number of lock done by the current implementation of Embedded Component Manager and I could use some more brains to check what I did.
You can find the new version on https://github.com/xwiki/xwiki-commons/blob/feature-improvecm/xwiki-commons-...
Quick comparison with the current one: * removed all "synchronized (this)" that was in pretty much all the methods and which made the CM block everything whenever anyone was using it * used ConcurrentHashMap to securize the insertion/search of components * basically the only remaining explicit lock is on singleton component initialization mostly to make sure we don't initialize the same component at the same time
If you find some small modification to improve a bit the speed don't hesitate too. There is possible improvements to speed up lookuplist and lookupmap (not going though the whole map basically) but it require important refactoring and I would prefer not doing too much things at once since it's a pretty critical peace of code and locking is the main issue right now.
Cool.
What's the plan? I feel this is a potentially dangerous commit for a RC1 so I'm not entirely comfortable having this in 3.3 as of now.
What I'd propose is this: - try it on xwiki.org as part of our upgrade of xwiki.org to see if it helps (since we're blocked on this upgrade) - depending on the result decide if the benefits are greater than the risks for 3.3 final (I still think we might not have enough data to decide about this since this would need to run for a while to be sure it's ok - We also think that the issue on xwiki.org is not really with the CM so it might not help fix the issue we have with the 3.3M2 upgrade on xwiki.org). - maybe install it on myxwiki.org and let it run for some time and then commit it in a 3.3.1/3.4M1 if there's no problem?
WDYT?
Usually my plan for something like that would be to commit it in a M1 but in this particular cases I started working on it because it seems to be an important speed issue on 3.3 in the context of xwiki.org (even if I really doubt that's what blocking the farm). Since we now have a dev server for xwiki.org my plan is to first try to reproduce the issue we currently have when trying to upgrade it and put my modification and see if it gets any better. Also note that as part of this refactoring I fixed http://jira.xwiki.org/browse/XCOMMONS-64 which is a bit hard to separate from the rest now (I hit it while I was doing testing of my modifications). Even if I tagged it as blocker I'm OK to commit it in 3.4M1 only (and maybe 3.3.1 then) since providers did not existed before and and it's not really a regression.
Thanks -Vincent
_______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
-- Thomas Mortagne
participants (2)
-
Thomas Mortagne -
Vincent Massol