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



Reply via email to