[10] RFR: 8193507: [REDO] Startup regression due to JDK-8185582 (original) (raw)

Peter Levart peter.levart at gmail.com
Mon Dec 18 08:22:44 UTC 2017

Hi Sherman, Claes,

On 12/16/2017 08:29 PM, Xueming Shen wrote:

HI Peter,

We are back to the "the order of resource relocation and cleaner registration" discussion again :-)

Yes, the story repeats itself :-)

The approach you are suggesting and was implemented in the previous version for de/inflater via the lambda  basically is the same to have a callback mechanism () to delay the resource allocation until the Cleaner successfully registers the target object. I understood the benefit of dong that and agree it might be desired in certain circumstance, with the expectation that the Cleaner.register() might fail. But I think the Cleaner API is designed and implemented (at least for now) way that the "register()" is not going to fail in "normal" situation and you are not supposed (requested) to check if the returned Cleanable is null (never?).

No, Cleaner.register never returns null. This is by the spec.

or try to catch an unexpected unchecked exception.

No, you should not do that.

Basically if a fundamental mechanism like Cleaner.register() is broken/failed. You probably don't have lot of choices to recover from such a failure but simply propagate the failure message/exception to upper level.

Cleaner.register() is not broken but we can still expect a failure in the form of OutOfMemoryError, because registration involves allocation. Finalizable objects usually did not suffer from this as their registration happened very early in their construction. We should try to follow this order with Cleaner API too.

Java is pretty robust in recovering from most OutOfMemoryError(s) regardless of the programmer effort, simply because it has GC. Handling locks and native resources are a couple of exceptions (and there might be others) where the programmer must be carefully to maintain overall robustness. JVM has recently been given special feature to enable robustness in critical sections of synchronization primitives with @ReservedStackAccess annotation. If locks are that important, why should native resources be much less so?

It might be preferred not have the underlying resource allocated in this situation but I doubt it makes lots of difference and is worth the extra effort. But I think we can leave this to the future discussion when adding those newly proposed methods into the Cleaner interface.

That said, in this case, it appears it does not require any "extra effort" to simply move the init(nowrap) invocation further down into the zstream constructor. I'm fine to go with it.

I created an enhancement request:

https://bugs.openjdk.java.net/browse/JDK-8193685

Here's also the webrev that goes with it:

http://cr.openjdk.java.net/~plevart/jdk-dev/8193685_ZipInDeFlater_cleanupImprovement/webrev.01/

Since jdk10 stabilization repo has already been forked, this will probably be destined to JDK 11. Unless you think it should go to JDK 10. In that case I shall ask for permission to push to stabilization repo. The fix is not critical, but is also a low risk.

Regards, Peter

Thanks, Sherman

On 12/16/17, 2:22 AM, Peter Levart wrote: Hi Sherman,

Xueming Shen je 16. 12. 2017 ob 01:46 napisal: On 12/15/17, 3:43 PM, Peter Levart wrote: Hi Claes,

I see this is already pushed. I don't have any additional comments, but just want to know what was wrong with old code. You say that you used non-static inner classes. I don't see that in old code. All the lambdas you replaced with nested static classes should have already captured just local variables. I must be missing something and I would really like to know what. Perter, Since that code triggered all zip/cleaner regression tests failed. I rollback-ed that fix overnight to make our overnight tests happy. So what in the webrev is against my original code.

Here is my webrev that undo-ed the non-static inner classes. http://cr.openjdk.java.net/~sherman/8193490/webrev/ Sherman I see. So the 1st change was an attempt to fix a startup performance regression (JDK-8193471) by replacing lambdas with anonymous inner classes, which unfortunately capture 'this' if they are constructed in instance context. I'm sorry I was busy and haven't noticed this RFR or I would probably spot the mistake. The 2nd change is a re-attempt to do this with static classes (JDK-8193507). This fixes the startup performance regression problem, but it also re-introduces another one, which was carefully avoided by using the lambdas in the initial version. The problem is that in latest version of code, the initialization order is: - init(nowrap) - Cleaner registration while in lambda version it was the other way around: - Cleaner registration - init(nowrap) If the registration of Cleanable fails, the latest version of code leaks a native resource. Now that you have specialized ZStreamRef classes, you could post-pone the native resource allocation in a simple way, directly in the XxxZStreamRef constructor: Index: src/java.base/share/classes/java/util/zip/Inflater.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== --- src/java.base/share/classes/java/util/zip/Inflater.java (revision 48342:003d6365ec6a529616d0e5b119144b4408c3a1c8) +++ src/java.base/share/classes/java/util/zip/Inflater.java (revision 48342+:003d6365ec6a+) @@ -118,7 +118,7 @@ * @param nowrap if true then support GZIP compatible compression */ public Inflater(boolean nowrap) { -        this.zsRef = InflaterZStreamRef.get(this, init(nowrap)); +        this.zsRef = InflaterZStreamRef.get(this, nowrap); } /** @@ -442,9 +442,9 @@ private long address; private final Cleanable cleanable; -        private InflaterZStreamRef(Inflater owner, long addr) { +        private InflaterZStreamRef(Inflater owner, boolean nowrap) { this.cleanable = (owner != null) ? CleanerFactory.cleaner().register(owner, this) : null; -            this.address = addr; +            this.address = init(nowrap); } long address() { @@ -470,23 +470,23 @@ * This mechanism will be removed when the {@code finalize} method is * removed from {@code Inflater}. */ -        static InflaterZStreamRef get(Inflater owner, long addr) { +        static InflaterZStreamRef get(Inflater owner, boolean nowrap) { Class<?> clz = owner.getClass(); while (clz != Inflater.class) { try { clz.getDeclaredMethod("end"); -                    return new FinalizableZStreamRef(owner, addr); +                    return new FinalizableZStreamRef(owner, nowrap); } catch (NoSuchMethodException nsme) {} clz = clz.getSuperclass(); } -            return new InflaterZStreamRef(owner, addr); +            return new InflaterZStreamRef(owner, nowrap); } private static class FinalizableZStreamRef extends InflaterZStreamRef { final Inflater owner; -            FinalizableZStreamRef(Inflater owner, long addr) { -                super(null, addr); +            FinalizableZStreamRef(Inflater owner, boolean nowrap) { +                super(null, nowrap); this.owner = owner; } This could be a refinement of the last patch. What do you think? Regards, Peter

Regards, Peter Claes Redestad je 14. 12. 2017 ob 12:07 napisal: Hi, my previous fix failed due to use of non-static inner classes which kept the cleanable objects around. Also, Sherman suggested a more thorough fix to Inflater/Deflater after I had already pushed. Webrev: http://cr.openjdk.java.net/~redestad/8193507/open.00/ Verified all java/util/zip tests pass this time. /Claes

More information about the core-libs-dev mailing list