RFR: 8060130: Simplify the synchronization of defining and getting java.lang.Package

[Peter Levart]

So we all came to same conclusions after all. I would call this a 
redundant concurrent "initialization" ;-)

Regards, Peter

On 10/13/2014 05:06 PM, Claes Redestad wrote:
> Hi Peter,
>
> On 10/13/2014 04:32 PM, Peter Levart wrote:
>> On 10/12/2014 04:09 AM, Claes Redestad wrote:
>>>
>>>
>>> http://cr.openjdk.java.net/~redestad/8060130/webrev.03/
>>>
>>
>> Hi Claes,
>>
>> First, one more nit:
>>
>> 1635                 if (!map.containsKey(pkgName)) {
>> 1636                     map.put(pkgName, pkg);
>> 1637                 }
>>
>> ..could be written as:
>>
>> map.putIfAbsent(pkgName, pkg);
>>
>
> Nice catch!
>
>>
>> And second, I re-read the code of CachedManifest and noticed the 
>> following. You initialize the volatile instance field 'manifest' like 
>> that:
>>
>>  609         private volatile Manifest manifest = EMPTY_MANIFEST;
>>
>>
>> So in following code:
>>
>>  631         public Manifest getManifest() {
>>  632             resolveManifest();
>>  633             return manifest;
>>  634         }
>>  635
>>  636         private void resolveManifest() {
>>  637             if (manifest != null || url == null) {
>>  638                 return;
>>  639             }
>>  640             synchronized(this) {
>>  641                 if (manifest == null) {
>>  642                     manifest = AccessController.doPrivileged(new 
>> PrivilegedAction<Manifest>() {
>>  643                         public Manifest run() {
>>  644                             return loadManifest(fileName);
>>  645                         }
>>  646                     });
>>  647                 }
>>  648             }
>>  649         }
>>
>>
>>
>> ... loadManifest() will never be executed. getManifest() will always 
>> return EMPTY_MANIFEST.
>>
>> You probably wanted line 637 to be written as:
>>
>> if (manivest != EMPTY_MANIFEST || url == null) { ...
>>
>>
>> Likewise in loadManifest(), wou write:
>>
>>  586     private static Manifest loadManifest(String fn) {
>>  587         try (FileInputStream fis = new FileInputStream(fn);
>>  588              JarInputStream jis = new JarInputStream(fis, false))
>>  589         {
>>  590             return jis.getManifest();
>>  591         } catch (IOException e) {
>>  592             return EMPTY_MANIFEST;
>>  593         }
>>  594     }
>>
>> ..but you probably wanted the line 592 to return 'null' instead.
>
> Yes, I realized I got this messed up and have prepared a new patch 
> which passes tests and includes your previous
> comments:
>
> http://cr.openjdk.java.net/~redestad/8060130/webrev.04
>
> Aleksey Shipilev pointed out offline that we can't trust the volatile 
> to be completely initialized here (requiring a
> null check either way), so the better solution is to initialize 
> manifest to null and set it to EMPTY_MANIFEST as
> we resolve and consistently only do null checks.
>
> Inlining resolveManifest and loadManifest into getManifest as you 
> suggested seems to get rid of a few generated
> access methods. It's cutting corners on line length limits, though, 
> but maybe it can be allowed in this case?
>
> By also moving the static final EMPTY_MANIFEST into CachedManifest we 
> avoid generating any access method for
> this as well, with no impact on readability.
>
> For reference I also did some java -verbose:class experiments: any 
> Hello World program will load most
> ConcurrentHashMap classes already - in the same order - and the new 
> inner class in j.l.Package won't be loaded
> unless the application goes to lengths to get a system package, so no 
> reordering of startup load order is to be
> expected. The deviant is getPackages, which when invoked will 
> additionally load a couple of ConcurrentHashMap
> traversal related classes, but I don't think that matters.
>
> Byte code size and complexity seems to drop a bit, too, if anyone's 
> counting:
>
> javap -v java.lang.Package | wc -l
> before: 1263, after: 1139
>
> javap -v java.lang.ClassLoader | wc -l
> before: 2800, after: 2687
>
> Thanks!
>
> /Claes
>
>>
>>
>> Regards, Peter
>>
>