May 2016 - core-libs-dev

Cleaner cleanup
by Peter Levart 19 May '16

19 May '16

Hi Roger and others, When the new Cleaner API was created the implementation of Cleanable(s) was split into the low-level abstract [Soft|Weak|Phantom]Cleanable classes to be used internally for purposes where the footprint matters and their corresponding CleanerImpl.[Soft|Weak|Phantom]CleanableRef subclasses used as implementations that take a Runnable cleanup action and are exposed via the public Cleaner API. When thinking of possible JDK internal use cases for the low-level API, I came to the conclusion that [Soft|Weak|Phantom]Cleanable classes are not suitable as is, because in cases where footprint matters, it is usually also the case that the number of [Soft|Weak|Phantom]Cleanable instances created is larger and that construction performance also matters. Especially multi-threaded construction. I'm thinking of the use cases of auto-cleanable concurrent data structures. In such use cases, the present features of [Soft|Weak|Phantom]Cleanable classes, namely the guaranteed just-once cleanup action invocation and keeping the Cleanable instance reachable until the cleanup action is performed, are actually not needed and just present footprint and performance (contention) overhead. They also present an overhead as they don't allow GC to automatically collect the Cleanable instances if the data structure containing them becomes unreachable and corresponding registered cleanup actions obsolete. The mentioned features are important for public Cleaner.Cleanable instances as they are usually used for cleanup of native resources where the performance of their creation is not so drastically important and where there is no intrinsic data structure to hold them reachable. I propose to move those features from the [Soft|Weak|Phantom]Cleanable classes down the hierarchy to the CleanerImpl.[Soft|Weak|Phantom]CleanableRef subclasses: http://cr.openjdk.java.net/~plevart/jdk9-dev/Cleaner.cleanup/webrev.01/ In this change I also removed the CleanerImpl.[Soft|Weak]CleanableRef subclasses as they are not needed and I believe will never be. I also renamed the CleanerImpl.PhantomCleanableRef subclass to CleanerImpl.PhantomCleanableImpl. Changes to the implementation are straightforward. The most work was put into the corresponding test. I did some clean-up to it and also changed it to accommodate for the new behavior of [Soft|Weak|Phantom]Cleanable classes. The changes speak for itself. One of the not-so obvious changes was to replace the CleanableCase.clearRef() action with the CleanableCase.releaseReferent() action. The old clearRef() action did not serve any purpose. Whether this method was called or not, the behavior of the corresponding Cleanable was unchanged as the Reference instance (referenced from the 'ref' field) was always of the same strength as the Cleanable itself. So clearing it could not affect the behavior of the Cleanable. I changed 'ref' to hold a direct reference to the referent and renamed the field to 'referent'. I changed the EV_XXX int constants to Event enum constants with helper methods used in CleanableCase.expectedCleanups() method that now returns the number of expected cleanup invocations - in the PhantomCleanableImpl case this is the number of expected cleanup action invocations while in the plain XxxCleanable subclass cases it is the number of Cleanable.clean() method invocations. I added the no-actions case to both PhantomCleanableImpl and XxxCleanable cases and extended the number and combinations of XxxCleanable cases. The checkCleaned() method was extended to verify that the number of cleanup invocations is *no more* and no less then the expected. See how WeakKey test is now simplified. This is the typical use-case for WeakCleanable I was talking about. So, what do you think of this cleanup? Regards, Peter

4 9

RFR (M) 8148604: JEP 280, Switch to more optimal concatenation strategy
by Aleksey Shipilev 19 May '16

19 May '16

Hi, We have been waiting for this for at least 8 weeks, waiting for Jigsaw to settle in, and hs-main -> jdk9/dev integration to happen that brings a few testbug fixes into the jdk9/dev. Bug: https://bugs.openjdk.java.net/browse/JDK-8148604 Webrev: http://cr.openjdk.java.net/~shade/8148604/webrev.01/ (the change is XS, but impact is M) Targeted benchmarks show significant improvements with the MH_INLINE_SIZED_EXACT strategy, see the bug for the test results. Performance improvements are due to these reasons: *) it does not rely on OptimizeStringConcat in C2, but instead does lift up the concatenation into the Java code, spelling it out with java.lang.invoke building blocks. This also helps to improve C1 performance significantly; *) it uniformly covers the cases that OptimizeStringConcat does not cover, e.g. non-int/char arguments where OptimizeStringConcat bails, etc; *) it uniformly covers the Compact Strings (JEP 254) changes, e.g. selecting the final coder for the concatenated String, and calling appropriate coder-specific routines from JDK. Doing this in Java has the apparent benefit of piggy-backing on normal Java code profiling to figure out the hot branches -- a luxury OptimizeStringConcat-constructed code lacks. Unfortunately, we cannot test this with our regular performance testing, because it requires tests compiled with -target 9. The only easy target we have now is javac, and switching the concat strategy does not regress it (see JDK-8148605). MH_INLINE_SIZED_EXACT is functionally tested routinely with java/lang/String/concat/ jtregs on all platforms. Testing: java/lang/String/ jtregs; JPRT -testset hotspot Thanks, -Aleksey

3 3

Can an object be finalized while still weakly reachable?
by Martin Buchholz 19 May '16

19 May '16

I have some evidence that an object's finalize method can run while a weak reference pointing to it is not yet cleared, which surprised me. E.g. class F { protected void finalize() { assert wref.get() != this; } } static WeakReference wref = new WeakReference(new F()); If this is a bug, I can try to give y'all a repro recipe. If not, we should fix the docs """When the weak references to a weakly-reachable object are cleared, the object becomes eligible for finalization.""" (It's also quite possible I made a mistake diagnosing this)

3 5

RFR 8157239 java/lang/invoke/VarHandles/ tests fail by timeout with -Xcomp with lambda form linkage
by Paul Sandoz 19 May '16

19 May '16

Hi, Please review: http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8157239-vh-tests-xcomp-timeout… <http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8157239-vh-tests-xcomp-timeout…> A recent change modified the VH tests to test the lambda form linkage route: -Djava.lang.invoke.VarHandle.VAR_HANDLE_GUARDS=false However this causes test timeouts when the tests are run with -Xcomp. I think this is due to -Xcomp triggering recompilation storms due to lack of caching of linking lambda forms (which will be sorted out later on). The solution for the moment is to remove that test execution mode on iteration-based tests, and the mode will be added back once -Xcomp execution issues have been resolved. Paul.

2 1

RFR (XS): 8157188: 2 test failures in demo/jvmti due to unexpected class file version 53
by David Holmes 19 May '16

19 May '16

Not sure who really owns this file so cc'ing core-libs and serviceability. bug: https://bugs.openjdk.java.net/browse/JDK-8157188 The file src/java.base/share/native/include/classfile_constants.h describes information about classfiles and is used by libverify and ./demo/share/jvmti/java_crw_demo/java_crw_demo.c This file has not been updated for classfile version 53 and so asserts will fail in java_crw_demo.c when it encounters classes compiled for version 53 - as they now are. This version change caused test failures in the hotspot forest when it was pulled down earlier this week. This fix trivially bumps the current version number to 53 to fix the failing tests. It is a separate issue as to whether other changes are needed in this file to reflect what is new with classfile version 53. Diff below. Thanks, David ------ diff -r 3eea6819cc1f src/java.base/share/native/include/classfile_constants.h --- a/src/java.base/share/native/include/classfile_constants.h +++ b/src/java.base/share/native/include/classfile_constants.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004, 2012, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2004, 2016, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -31,7 +31,7 @@ #endif /* Classfile version number for this information */ -#define JVM_CLASSFILE_MAJOR_VERSION 52 +#define JVM_CLASSFILE_MAJOR_VERSION 53 #define JVM_CLASSFILE_MINOR_VERSION 0 /* Flags */

4 5

RFR(s): 8133977u1 add specification for serial form of immutable collections
by Stuart Marks 19 May '16

19 May '16

Hi all, Please review this updated webrev/specdiff. Changes since previous: - removal of mention of CollSer from the API specification - make CollSer's Object[] field transient, and handle its contents as custom serial data in readObject/writeObject - specification changes to reflect the above Webrev: http://cr.openjdk.java.net/~smarks/reviews/8133977/webrev.1/ API specdiff: http://cr.openjdk.java.net/~smarks/reviews/8133977/specdiff.1/api.specdiff/… serialized-form.html diff: http://cr.openjdk.java.net/~smarks/reviews/8133977/specdiff.1/serial.specdi… Thanks, s'marks

1 0

Re: RFR(s): 8133977 add specification for serial form of immutable collections
by Stephen Colebourne 18 May '16

18 May '16

CollSer should not be public, especially not just for serialization reasons. Stuart, I disagree with using an int for the flags, it should be a byte. If you need future expansion you can use 0xff to indicate it with the parser reacting accordingly. That is the strategy for JSR 310 Stephen On 17 May 2016 8:29 a.m., "Peter Levart" <peter.levart(a)gmail.com> wrote: Hi Stuart, On 05/17/2016 12:48 AM, Stuart Marks wrote: > Hi all, > > Please review this changeset that adds specifications of the serialized > forms (really, a single serialization proxy class) for the immutable > collections implementation. There are no code changes in this changeset, > just documentation. > > It's somewhat odd, but the class doc for the serialization proxy isn't > actually included in the serialized-form.html output. I had to jigger > around the method doc for readResolve() to include some general information > about the class. > > I also added links manually from the List, Map, and Set interfaces to the > serialized form and vice-versa. I'm not aware of another way for a private > class to link to its (proxied) serialized form. > > I was able to coerce specdiff into giving a diff of serialized-form.html. > It's not very convenient, though; like serialized-form.html, the html diff > is one big file. The only difference is the addition of java.util.CollSer. > > Webrev: > > http://cr.openjdk.java.net/~smarks/reviews/8133977/webrev.0/ > > API specdiff: > > > > http://cr.openjdk.java.net/~smarks/reviews/8133977/specdiff.0/api.specdiff/… > > serialized-form.html diff: > > > > http://cr.openjdk.java.net/~smarks/reviews/8133977/specdiff.0/serial.specdi… > > Thanks, > > s'marks > Perhaps java.util.CollSer could be promoted to be a public class. It does, after all, specify the API (serialization form == API). I don't see a point in hiding it and then jiggering around the method doc for readResolve()... You could just restrict its use by keeping the constructor(s) package-private... What do you think? Regards, Peter

4 11

RFR: 8156807: Pack200 must support v53.0 class files
by Kumar Srinivasan 18 May '16

18 May '16

Hi, Alex or someone please review this simple fix, the first part is simply bumping the class file version number, and some cleanup of redundant constants in the native unpacker. The second part is fixing up the test, which does not read the process streams correctly and causes a test hang, simplified the test with time tested utilities. http://cr.openjdk.java.net/~ksrini/8156807/webrev.00/ Thanks Kumar PS: Appreciate a quick turnaround as this is causing intermittent test failures in our internal CI systems.

3 3

RFR 8154192: Deprivilege java.scripting module
by Sundararajan Athijegannathan 18 May '16

18 May '16

Please review fix for https://bugs.openjdk.java.net/browse/JDK-8154192 java.scripting module is assigned to platform class loader (instead of boot loader). And java.scripting module is given AllPermission [previously it had AllPermission implicitly because of being boot loader code] jdk repo: http://cr.openjdk.java.net/~sundar/8154192/jdk/webrev.00/ top level repo: http://cr.openjdk.java.net/~sundar/8154192/top/webrev.00/ Tests: javax.script API tests, jrunscript, jjs tool tests and nashorn tests. Thanks, -Sundar

4 6

RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
by Brent Christian 18 May '16

18 May '16

Hi, Please review this fix, courtesy of Peter Levart (thanks!), that I would like to get in. https://bugs.openjdk.java.net/browse/JDK-8029891 http://cr.openjdk.java.net/~bchristi/8029891/webrev.0/ There is some discussion of it in the bug report, starting at 2014-12-31. The problem, as stated by Mandy: "System Properties is a hashtable that synchronizes on itself for any access. Currently System.getProperties returns the Properties instance accessed by the system in which any application code might synchronize on it (that's what the test is doing). The problem reported JDK-6977738 is about Properties.store method that was fixed not to synchronize on this instance. System property is a common way for changing the default setting and so it's impractical to expect the class loading code path not to call System.getProperty." This fix changes java.util.Properties to store its values in an internal ConcurrentHashMap, ignoring its Hashtable heritage. In this way, Properties can be "de-sychronized": all methods inherited from Hashtable are overridden, to remove synchronization, and delegate to the internal CHM. The serialized form is unchanged. An alternative approach considered would be for System.getProperties() to return a duplicate snapshot of the current Properties. This presents a compatibility risk to existing code that keeps a reference to the return value of System.getProperties() and expects to either read new properties added afterwards, or set properties on the cached copy. -Brent

7 41