Bugzilla – Full Text Bug Listing
| Summary: | BundleContextImpl has unsync'ed access to EventListeners. | ||
|---|---|---|---|
| Product: | [Eclipse Project] Equinox | Reporter: | Ikuo Yamasaki <yamasaki.ikuo> |
| Component: | Framework | Assignee: | equinox.framework-inbox <equinox.framework-inbox> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | Ikuo_Yamasaki, yamasaki.ikuo |
| Version: | 3.2 | ||
| Target Milestone: | 3.3 M2 | ||
| Hardware: | PC | ||
| OS: | Windows XP | ||
| Whiteboard: | |||
| Attachments: | |||
|
Description
Ikuo Yamasaki
Created attachment 45857 [details]
Illustrative Patch of BundleContextImpl#removeXXXListener()
Is there a measure performance improvement here? We need to know the potential saving before every possible change. I do not like this proposed change. And I don't believe there is a reasonable performance improvement here. While considering the patch I found some memory model problems with code caused by the late creation of the EventListeners objects in BundleContextImpl (I will look at fixing these next week). They really should be created in the constructor. However not all bundles will use listeners or each type of listener. Allowing the EventListeners objects to be discarded will only further complicate the memory model issues. Unless there is a compeling performance gain here, I do not support the proposed changes to discard the EventListents objects. (In reply to comment #2) > Is there a measure performance improvement here? We need to know the potential > saving before every possible change. No performance results I have. To test it, I should set up the situation as follows (in case ServiceListener); - In advance, Start many bundles each of which calls addServiceListener() and removeServiceListener but doesn't stop. - In test, Measure how long ServiceEvent delivery takes. # I don't have any test that simulates a bunch of bundles' activation, yet. > I do not like this proposed change. And I don't believe there is a reasonable > performance improvement here. I agree on the point that it might not improve performance much. Imagine that there are "N" bundles that add ServiceListeners and remove all of them, but doesn't stop. If the patch is applied, the number of entries in framework.serviceEvent will be reduced by "N" and the size of for-loop in ListenerQueue#dispatchEventSynchronouss() will be reduced by "N". Even if not, it will result in "N" calls of EventManager.dispatchEvent() which does nothing acutually (because of listeners.length=10). (I just wanted to ask if the patch will cause a problem or there is any reason to implement as current one ? I don't strongly insist on applying my patch :-) ) > While considering the patch I found some memory > model problems with code caused by the late creation of the EventListeners > objects in BundleContextImpl (I will look at fixing these next week). They > really should be created in the constructor. However not all bundles will use > listeners or each type of listener. I don't understand why should the EventListeners (e.g. BundleContextImpl#serviceEvent)be really created in the constructor. As you mentioned, at its construction, we cannot know if the bundleContextImpl use listeners or not. I hope you will explain (next week is fine). > Allowing the EventListeners objects to be > discarded will only further complicate the memory model issues. Unless there is > a compeling performance gain here, I do not support the proposed changes to > discard the EventListents objects. OK. I agree in this point, too. Created attachment 47796 [details] new patch (In reply to comment #2) > I do not like this proposed change. And I don't believe there is a reasonable > performance improvement here. While considering the patch I found some memory > model problems with code caused by the late creation of the EventListeners > objects in BundleContextImpl (I will look at fixing these next week). They > really should be created in the constructor. However not all bundles will use > listeners or each type of listener. Allowing the EventListeners objects to be > discarded will only further complicate the memory model issues. Unless there is > a compeling performance gain here, I do not support the proposed changes to > discard the EventListents objects. BJ, I do see the memory model issues you are referring to. The code always enters a synchronization block before checking to see if the serviceEvent == null. Is this the code pattern that is bad? ... synchronized (framework.serviceEvent) { if (serviceEvent == null) { serviceEvent = new EventListeners(); framework.serviceEvent.addListener(this, this); } serviceEvent.addListener(listener, filteredListener); } Should the EventListeners fields be volitile? Or should we just create all the event listener objects in the constructor? I modified the patch to apply to all types of listeners and I moved the null check within the sync block when removing a listener. I doubt the level of performance gain we will see from such a change. I vote -1 for releasing a fix. But I would still like to understand the memory model issues you are referring to. I really want to avoid creating EventListeners unless a context adds a listener. For example, hardly any bundles are bundle listeners or framework listeners. And in eclipse today hardly any bundles are service listeners. 151645(In reply to comment #4) > But I would still like to understand the memory model issues you are referring > to. I really want to avoid creating EventListeners unless a context adds a > listener. For example, hardly any bundles are bundle listeners or framework > listeners. And in eclipse today hardly any bundles are service listeners. I also would like to understand that issue. It must be reflected to the implementation of Bugzilla#151645, where I proposed more efficient ServiceEvent Delivery implementations than now. Created attachment 48040 [details] Fixes to correct unsynchronized references to BundleContextImpl's EventListener objects (In reply to comment #2) > While considering the patch I found some memory > model problems with code caused by the late creation of the EventListeners > objects in BundleContextImpl (I will look at fixing these next week). There were several places where the EventListener objects in BundleContextImpl were referenced without proper synchronization (since they are shared mutable state of BundleContextImpl). The attached patch adds proper synchronization. Created attachment 48041 [details]
Simplified patch for the unsynchronized access fix
I updated the pacth to remove the changes to the Framework class. The references to the EventListener objects were already synchronized in the publishXxxEvent methods.
(In reply to comment #4) > Created an attachment (id=47796) [edit] > new patch > The patch seems to be sufficient. It addresses some of the unsynchronized access issues I mention (as memory model issues which is too broad a term). However, I still don't see any performance measurements (less memory used, faster execution) that warrant this change. I am talking about the real world here and not some specifically created microbenchmark. Of course we can construct such a benchmark that would demonstrate that almost any small change will be of benefit. We need real world numbers to drive making a change like this. How much time/memory is saved with this change when running Eclipse IDE (or some other larger Equinox based product)? At this point, I am still not inclined to make the proposed change with the demonstrated performance improvement. (However the patch for unsynchronized access should be applied.) I released the latest patch from BJ to fix the unsync'ed access to EventListeners. We should not release the other changes proposed by this bug because the performance benifits have not been confirmed. I changed the title to reflect what was fixed. errr! I mean Fixed 3.3 M2. |