[rfc][icedtea-web] Resource/ResourceTracker clean up

[Jiri Vanek]

On 05/13/2014 04:56 PM, Andrew Azores wrote:
> On 05/07/2014 04:21 AM, Jiri Vanek wrote:
>> On 05/06/2014 09:37 PM, Andrew Azores wrote:
>>> Hi,
>>>
>>> Prompted by Omair's recent patches to fix caching, I started looking into the cache package too. I
>>> was not too happy with what I found in Resource and ResourceTracker :( here's the summary of the
>>> changes made:
>>>
>>> Resource:
>>> - stop using ints as bit fields for status flags. Use enum with EnumSet instead. isSet,
>>> changeStatus, and getStatusString heavily refactored for this.
>>> -- setStatusFlag, unsetStatusFlag, resetStatus, isInitialized, hasFlags added
>>> - made status field private
>>> - made several fields final, some others can't be made final (at least not without even more
>>> refactoring)
>>> - "transferred" and "size" long fields made volatile so that accesses are atomic
>>> !! hashCode override added, because equals was already overridden without it !!
>>> -- AND, Resource was used as the key type in a ConcurrentHashMap in ResourceTracker. Wonderful.
>>>
>>> ResourceTracker:
>>> - "import static" rather than creating copies of the status values in this class too...
>>> - Collection fields declared more generically (eg as just Collection) where possible, which was most
>>> places
>>> -- "queue" renamed "requestedDownloads" and changed from ArrayList to HashSet - the methods that
>>> provide for selecting the next resource to download from the "queue" don't even care about the
>>> ordering in the queue anyway, really, so being a List is an unnecessary restriction. I don't think
>>> we would want to allow for duplicates either, so a Set makes more sense to me unless duplicate
>>> entries are actually desired
>>> -- "resources" also now a HashSet instead of ArrayList, for similar reasoning - ordering didn't
>>> matter to begin with, and I don't see the point in allowing duplicates
>>> - SO many instances of ugly bit-fiddling to check a Resource's status cleaned up
>>> - #wait() takes a Collection rather than an array, allows for some cleanup in #waitForResources()
>>> - #findBestUrl() formatting fixed, no actual changes made here
>>> - #selectByFlag() renamed to #selectByStatus(), extracted most logic into #selectByFilter()
>>> (required since there is no more explicit UNINITIALIZED status flag)
>>>
>>> There's still a lot of fixing that can be done in ResourceTracker however. eg using a proper
>>> ThreadPool, proper synchronization rather than the global "lock" object and locking on a few other
>>> important fields. I'll start looking into that after this goes in.
>>>
>>> Thanks,
>>>
>>
>> As omair pointed out:
>>
>> > In the future, though, please try and keep separate changes as separate.
>> > At least formatting changes should be done as a separate patch.
>>
>> please. this is quite important. Butok n context of this patch.
>>
>>
>> > Also, would it be possible to add tests?
>>
>> ugh... I would like to stop this being pushed until the unitttest for *original* behavior are
>> done. And then they are adapted to new "behavior". This part of itw is lacking the tests,  so here
>> is actually no trace if behvaiour was preserved.
>>
>> Otherwise the patch is ok.
>>
>>
>> One more nit - I'm also working n fixing the cache. So we should talk a bit so we do not duplicate
>> an effort:)
>>
>> My main task is to make itw-javaws run offline. End even behind the meaning of <offline> tag,
>> which is often misused. With some luck, also the applets will run offline :)
>>
>> J.
>>
>
> Here's a cleaned up version of the main patch again now that the first round of tests have gone in
> separately. The tests have been updated to match the new code. I'll deal with fixing formatting and
> such in another patch.
>
> Thanks,
>
Lets wait for Omair, But ok from my side.And thank you for tests.

J.