[rfc][icedtea-web] ResourceTracker Thread Pool Enhancement

[Jacob Wisor]

On 10/01/2014 at 06:26 PM, Jie Kang wrote:
> Hello,
>
> This patch replaces the manual thread management system in Resource Tracker with a fixed thread pool using Java's Executor Service class.
>
> The functionality remains the same as before. In terms of testing, using the Performance Test [1], running the reproducers and performing manual testing has shown no regressions.
>
> Thoughts?
>
>
> [1] : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2014-August/028980.html
>
>
> Regards,

Hey, this looks interesting. :-)

I am aware that you have been aiming for identical functionality, so please keep 
in mind that this post is more of a discussion than a comment on the code quality.

> +    private static final ExecutorService threadPool = Executors.newFixedThreadPool(5);

A constant pool size of 5 seems rather arbitrary. I suppose the most suiting 
thread pool scheduler/model in this case would be one that tries to utilize the 
CPU(s) most effectively, choosing threads that are most probable to finish 
first. The thread pool size should/could probably be calculated based on the 
available bandwidth, average resource size, average latency to a resource, and 
the number of cores or CPUs. But then, this would also require gathering some 
considerable amount of statistical data which seems not feasible for this 
purpose. Hmm, I do know, 5 seems as arbitrary as 42. ;-)

Furthermore as I understand (please correct me if am I wrong), the fixed thread 
pool does not run queued threads when all slots in the pool are blocked or 
waiting, say for example for a response on the network. In this case, which 
probably happens a lot, this thread pool does not help much. With many resources 
to be download, waiting for the first 5 to complete while others might be 
downloading instead, is a zero-sum game.

Also, the positive runtime effects of recycling or reusing existing threads is 
negligible in this scenario either. Actually, such an effect is only even 
measurable if the threads' average lifetime is below 100 ms and hundreds if not 
thousands of threads are to be created per second. So, a thread pool size of 5 
and the average lifetime of a thread downloading a resource for about several 
ten seconds if not more, has no practical effect on the overall performance. 
Again, I know you are just recreating already existing functionality with the 
concurrent framework. But, if you do so, you may want to consider improvements 
to IcedTea-Web that the concurrent framework might enable.

> -        if (threads == 0) {
> -            synchronized (prefetchTrackers) {
> -                queue.trimToSize(); // these only accessed by threads so no sync needed
> -                active.clear(); // no threads so no trackers actively downloading
> -                active.trimToSize();
> -                prefetchTrackers.trimToSize();
> -            }
> +        synchronized (prefetchTrackers) {
> +            queue.trimToSize(); // these only accessed by threads so no sync needed
> +            prefetchTrackers.trimToSize();
>          }

A few questions:
1. Why is the ResourceTracker.queue still required if there is a thread pool 
managing a queue now? Shouldn't every resource get its own thread to be queued 
in the thread pool?

2. Why is ResourceTracker.queue accessed while holding a lock on 
ResourceTracker.prefetchTrackers? If access to ResourceTracker.queue does need 
to be synchronized, then why not synchronize access to it too?

I hope this helps a bit with thinking. ;-)

Jacob