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

[Claes Redestad]

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
>