On 2014-10-11 02:31, Mandy Chung wrote:
On 10/10/2014 8:10 AM, Claes Redestad wrote:
Hi all,
please review this patch which attempts to clean up synchronization
and improve scalability when
defining and getting java.lang.Package objects.
I agree with David that getting Package objects are not performance
critical. On the other hand, the code defining/getting Packages is
old and deserves some cleanup especially the synchronization part.
If you run helloworld program, how does that change the list of loaded
classes besides the new CachedManifest class?
webrev: http://cr.openjdk.java.net/~redestad/8060130/webrev.02/
ClassLoader.java
line 272: can you change the declared type as Map.
Map misses the atomicity requirement of putIfAbsent, ConcurrentMap is OK
but leaves some
related questions open (why we can't add a null value, specifically).
I'm glad it was brought up
and discussed and will use ConcurrentHashMap for private fields unless
there's a strong
preference otherwise.
definePackage throws IAE if there exists an existing package either
in this class loader or one of its ancestors.
- this change would not catch if two definePackage calls to define
a package of the same name but with different spec version, title, etc
concurrently.
You may not be able to avoid synchronizing on packages for this case.
Right, I was I think synchronization can still be avoided by throwing
IAE if
putIfAbsent doesn't return null:
if (packages.putIfAbsent(name, pkg) != null) {
throw new IllegalArgumentException(name);
}
return pkg;
move line 1623 to 1630 so that the declaration of map is closer to
the assignment.
Ok
Package.java
line 557 there is a possibility that new Package[pkgs.size()] is not
big enough and a new array would be created. As this method is not
popularly used, it's okay if another array is created.
Yes, an unlikely race.
line 563 and 565 can be merged
line 570-575: do you think you can modify the private
Package(String name, Manifest man, URL url, ClassLoader loader)
constructor
to take null Manifest and null url so that these lines can be folded into
pkg = new Package(name, cachedManifest.getManifest(),
cachedManifest.getURL(), null);
I think I'll take your suggestion below and ensure cachedManifest and
it's getManifest()
never evaluate to null, which makes for a cleaner patch. There is some
code duplication
here with URLClassLoader#definePackage. Future cleanup?
It would seem the ClassLoader argument in this ctor is always called
with null. Remove?
I think CachedManifest class and the createCachedManifest method need
some work. Perhaps we can have the CachedManifest constructor to
obtain the URL.
Each invalid fn will have one instance instead of NO_MANIFEST singleton
but that should not happen as fn is the filename where the classes
loaded from the bootclasspath. CachedManifest.url can then become final.
line 587-601 would not be needed. Can we avoid line 606 and write
the createCachedManifest method this way:
if (!manifests.containsKey(fn)) {
manifests.putIfAbsent(fn, new CachedManifest(fn));
}
return manifests.get(fn);
Yes. Looked a bit dangerous, but it seems we still maintain the
necessary guarantees.
You may be able to further simplify CachedManifest and remove the
resolved
field by storing an empty Manifest when loadManifest returns null.
That may help the private Package constructor not require any change
to merge line 570-575 as my comment noted above.
Sure! Taking in all these suggestions as well as realizing a race could
cause different Package
to return from subsequent calls to Package.defineSystemPackage brings me
to this:
http://cr.openjdk.java.net/~redestad/8060130/webrev.03/
Now... How about a slightly alternative approach: instead of caching
Manifests we could create
and cache a Package - call it a prototype - then add a private
constructor taking the
package name and the "prototype" Package. The Package objects should
come with a
smaller footprint and have the added benefit of being effectively
immutable. Does that
sound like an improvement?
You will need to check there is any test to verify Package created with
and without manifest. Do you mind making this change and tests (I
realize
it might be out of scope of this performance improvement you initially
anticipated)?
I'll take a look at the current test coverage and give it some thought.
Thanks!
/Claes
Mandy
