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
