Bugzilla – Full Text Bug Listing
| Summary: | [Progress] synchronization in ProgressManager too expensive | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Randall Theobald <rstheo> | ||||||||
| Component: | UI | Assignee: | Prakash Rangaraj <prakash> | ||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | chrisri, edwinc, john.cortell, Kevin_McGuire, min123, pwebster, Tod_Creasey | ||||||||
| Version: | 3.4.1 | ||||||||||
| Target Milestone: | 3.5 M5 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Windows XP | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Randall Theobald
Created attachment 120665 [details]
Patch using ListenerList
Randall,
I've basically done the same thing thru the ListenerList class. Can you apply my patch and check whether this solves the problem?
Paul,
This bug is raised against 3.4.1 Do we still have time to push in bugs for 3.4.2? Or should we target for 3.5?
I notice that the new patch doesn't include the schedule delay. Is this just an oversight or is there some other reason to not include that portion of that original patch? See https://bugs.eclipse.org/bugs/attachment.cgi?id=120102&action=diff#Eclipse%20UI/org/eclipse/ui/internal/progress/ProgressViewUpdater.java_sec1. I'm fine with the implementation using the ListenerList, however, like Min mentioned, you dropped the changed to ProgressViewUpdater that limits the number of schedule calls. Is there a reason? (In reply to comment #3) > I'm fine with the implementation using the ListenerList, however, like Min > mentioned, you dropped the changed to ProgressViewUpdater that limits the > number of schedule calls. Is there a reason? First I wanted to make sure that the ListnerList stuff works (it didn't break any of our tests, but wanted to make sure of the performance) Will be adding that change as well. (In reply to comment #3) > I'm fine with the implementation using the ListenerList, however, like Min > mentioned, you dropped the changed to ProgressViewUpdater that limits the > number of schedule calls. Is there a reason? > I did a small test for the schedule this: long start = System.currentTimeMillis(); for(int i=0;i<115*1000;i++) { job.schedule(100); } long end = System.currentTimeMillis(); System.out.println("Time taken:"+(end-start)+"ms"); I ran it several times and on average scheduling a job 115k times takes 15 ms. Do we really have to optimize that out? Just chiming in again... uncontended synchronization in Java is very fast, in some books I've read they say that it basically free. However, contended synchronization is slow and gets slower with the number of simultaneous requests. In the workload the Randall and I have been working on, there an Auto Build and Validation is running in many many jobs. This puts a lot of strain on the ProgressManager and causes it to schedule even more jobs. Your test was done in a single thread where there is no chance for contention. If instead you created, say, 50-100 threads and each one of those threads was calling schedule on that job you may see some slower results due to contention. Additionally, you may need to have >1 core or >1 processor to experience the worst forms of contention. I would recommend avoiding the JobManager as much as possible in tight loops. Yes, these calls are normally very fast, especially from the same thread (when there is no lock contention). However, on a system with a lot of load and a lot of threads/jobs, and with the requests spread across many threads, the cost of this definitely goes up. The contention spikes that I investigated were consistently on the locks in the Jobs infrastructure, so removal of any completely unnecessary grabbing of these locks would be good. I would vote yes, to optimize it. Optimizing all such UI-related updates to not occur more often than a human can consume them would help with general resource allocation. Created attachment 123174 [details]
Patch v02
Patch using ListenerList + sync delay
(In reply to comment #8) > Created an attachment (id=123174) [details] > Patch v02 Released to HEAD >20090121 PW . Verified in I20090126-1800 I believe this introduced a race condition bug in the Progress viewer. I've opened bug 395645 |