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

[Andrew Azores]

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,

-- 
Andrew A

-------------- next part --------------
A non-text attachment was scrubbed...
Name: resource-enumset-4.patch
Type: text/x-patch
Size: 35616 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20140513/a02596fe/resource-enumset-4-0001.patch>