Bug 293882

Comment 1 Min Idzelis 2009-11-01 16:01:32 EST

Created attachment 151032 [details]
Fix

This fix uses a single job and dynamic queue to process the items.

Comment 2 Paul Webster 2009-11-02 08:15:46 EST

I thought the Jobs were queued and then processed by worker threads.  Is that not sufficient?

PW

Comment 3 Min Idzelis 2009-11-02 08:46:02 EST

(In reply to comment #2)
> I thought the Jobs were queued and then processed by worker threads.  Is that
> not sufficient?

Actually, it is not. Since the job that was created did not specify a scheduling rule it is allowed to run concurrently. In IBM products, 100+ plugins can be activated at once (during startup). This means that 100 jobs will be queued and simultaneously executed. This creates a spike of 100 worker threads that execute in parallel. This creates high thread contention and can slow down the rest of the system.

Comment 4 John Arthorne 2009-11-02 09:38:19 EST

Min is correct that jobs don't automatically impose any limits on the number of jobs running at once. However in this case it happens for each bundle that defines a working set descriptor - is this really hundreds of bundles or is this just a hypothetical problem? The other simple solution is to use a scheduling rule that would only allow one of these to run at once.

Comment 5 Min Idzelis 2009-11-02 09:57:59 EST

(In reply to comment #4)
> Min is correct that jobs don't automatically impose any limits on the number of
> jobs running at once. However in this case it happens for each bundle that
> defines a working set descriptor - is this really hundreds of bundles or is
> this just a hypothetical problem? The other simple solution is to use a
> scheduling rule that would only allow one of these to run at once.

Unfortunately, it is about a hundred. (At least, not hundreds). 

Scheduling rule would work for getting rid of the overhead of starting so many threads. But, since these are really short lived Jobs it still has the overhead of notifying job listeners repeatedly. This affects the progress view since it listens to all job states. 

I've opened a bug for a API queue-processing job to aid with this common pattern in job use. See bug 293372. 

I'm also working on contention (among other) issues in the progress view. That's in bug 293877.

Comment 6 Hitesh 2009-11-02 11:07:05 EST

The workingsetUpdater is optional to use in the extension point and the get/add updater code runs in UI thread only - and only for bundles that just got activated.

:)


A WorkbenchJob is used, which is perfectly valid for many reasons (ui code, property change, sync, etc) .I assume the switch to UI Thread should happen rather too quickly to let a contention build up. The contention even if it does happen ,which IMHO is a rare condition here, is possible only when we had many applicable bundles getting activated at the same time, and the various jobs were changing their threads at the same time.

Thanks for the patch, Min ,I 'll look at it in detail later. Just for the sake of theory, a change to  non-ui-thread will certainly break some clients; for example one obvious change is property change notification.

I am not sure about scheduling rule either (I'll look at it later), but a quick glimpse shows we catch lock on the field 'updaters' in other places as well. For example addToWorkingSet removeWorkingSet,etc, which, ideally, should be accessed from UIthread, but bugs show a different story; and things people can do with listeners :).

Comment 7 Hitesh 2009-11-02 11:13:18 EST

(In reply to comment #5)


Ooops!! I replied on a page loaded some while back (had to leave desk for a while).

> Unfortunately, it is about a hundred. (At least, not hundreds). 
> 
> Scheduling rule would work for getting rid of the overhead of starting so many
> threads. But, since these are really short lived Jobs it still has the overhead
> of notifying job listeners repeatedly. This affects the progress view since it
> listens to all job states. 
> 

You are right about the Progress View, and I noticed your bug on the same.

> I'm also working on contention (among other) issues in the progress view.
> That's in bug 293877.

This sounds like a great idea.

Comment 8 Min Idzelis 2009-11-02 11:21:24 EST

(In reply to comment #6)

> Thanks for the patch, Min ,I 'll look at it in detail later. Just for the sake
> of theory, a change to  non-ui-thread will certainly break some clients; for
> example one obvious change is property change notification.

Oh, didn't realize it was a WorkbenchJob. Even though WorkbenchJobs are UI jobs that run in the UI thread, worker threads are still spawned off to invoke the asyncExec(). I did not intend to convert the job from a UI job to a non-UI job. It would be safe to subclass WorkbenchJob instead.

Comment 9 Hitesh 2009-11-02 11:36:19 EST

> A WorkbenchJob is used, which is perfectly valid for many reasons (ui code,
> property change, sync, etc) .I assume the switch to UI Thread should happen
> rather too quickly to let a contention build up. The contention even if it does
> happen ,which IMHO is a rare condition here, is possible only when we had many
> applicable bundles getting activated at the same time, and the various jobs
> were changing their threads at the same time.
> 

> I am not sure about scheduling rule either (I'll look at it later), but a quick
> glimpse shows we catch lock on the field 'updaters' in other places as well.
> For example addToWorkingSet removeWorkingSet,etc, which, ideally, should be
> accessed from UIthread, but bugs show a different story; and things people can
> do with listeners :).

These points still remain, although I plan to look at them both later. 

The fix lies in Progress View, there are places where such fixes can't be made( I can name an area for sure where it's even more lively;) but that is a different story). The fix in progress view will be fix-most types; I have some ideas myself and will share them on the bug.

Comment 10 Hitesh 2009-12-02 03:21:19 EST

Closing as WONTFIX, I guess this is not worth the effort.