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

[Jacob Wisor]

On 10/06/2014 at 07:13 PM, Jie Kang wrote:
> ----- Original Message -----
>> 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?
>
> Hello,
>
> I chose to keep the queue in place for this patch to make the change as small as possible, in the hopes that reviewing would be easier and regressions would be prevented. I plan to continually revamp the tracker in small, incremental patches in order to keep the probability of introducing regressions at a minimum.
>
> As well, as a kind of side note, the aim for these refactors is to make it easier to maintain and expand the code in the future. I intend to add the ability to use JNLP's <update>, <check> and <policy> tags [1] to ITW, but I found it quite annoying to do so with the current state of the ResourceTracker + Cache system.

Sounds great! :-) However, I would deem adding the install feature more 
important to IcedTea-Web before anything else. Well, the update feature seems to 
depend on install, I guess. ;-) But you're free to proceed as you see fit, of 
course.

> I actually have a completely new ResourceTracker that I believe works fine, but rather than send in a huge patch that replaces the old tracker with a new one, I thought it would be better to incrementally modify the old tracker.

Okay, let's have it.

> [1] http://docs.oracle.com/javase/7/docs/technotes/guides/javaws/developersguide/syntax.html#update
>
>>
>> 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?
>
>
> The whole system revolving around prefetchTrackers is unnecessary IMO. In the context of ITW's downloading of resources for an applet, we have three scenarios.
>
> 1. We download something and require it immediately, so we wait. (Might as well have not threaded the download)
> 2. We download something, perform some operations in between, and then wait for it right before we need it.
> 3. We download something in the background, while the applet is running.
>
> (Currently, option 3 is not supported, but it will be)
>
> These scenarios really don't require the complicated system we have now involving the prefetchTrackers.

Right. However, I think IcedTea-Web should support the indexing of JAR files 
feature.

> In the end these things will be removed. I wasn't sure why the code was written this way and so I decided to keep it as is. On further review, I have found that all calls to queue are done within a synchronization on 'lock'. Some of these synchronizations are done in methods earlier in the call stack. So there are methods that have comments like: "must only call this method with synchronization on lock". Ugh..

Yeah, you name it ... ugh... :-D

Anyway, +1 from me to push.

Jacob