[10] RFR: 8193507: [REDO] Startup regression due to JDK-8185582
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
On 14/12/2017 11:07, Claes Redestad wrote:
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. The changes means some code duplication but it's not too bad. Can the ZStreamRef(long) constructor go away so that we don't need to think about creating a ZStreamRef without a cleanable. Also can the fields in InflaterCleanupAction be final.
-Alan
On 2017-12-14 13:00, Alan Bateman wrote:
On 14/12/2017 11:07, Claes Redestad wrote:
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. The changes means some code duplication but it's not too bad. Can the ZStreamRef(long) constructor go away so that we don't need to think about creating a ZStreamRef without a cleanable.
We need the cleanable = null case for the FinalizableZStreamRef case, otherwise we'd go into spin. The cleanable is implicitly checked for null in the ZStreamRef.get method, which is the only one used outside of these classes.
Also can the fields in InflaterCleanupAction be final.
Sure: http://cr.openjdk.java.net/~redestad/8193507/open.01/ /Claes
On 14/12/2017 12:25, Claes Redestad wrote:
We need the cleanable = null case for the FinalizableZStreamRef case, otherwise we'd go into spin. The cleanable is implicitly checked for null in the ZStreamRef.get method, which is the only one used outside of these classes.
Okay but can the clean method at least check if cleanable is null to avoid needing to wonder if it's possible. Also can the constructors be grouped as it's very confusing to not have them grouped together (dropping the space in "ZStreamRef (" would help too. Otherwise I think the approach is okay. -Alan
On 2017-12-14 13:29, Alan Bateman wrote:
On 14/12/2017 12:25, Claes Redestad wrote:
We need the cleanable = null case for the FinalizableZStreamRef case, otherwise we'd go into spin. The cleanable is implicitly checked for null in the ZStreamRef.get method, which is the only one used outside of these classes.
Okay but can the clean method at least check if cleanable is null to avoid needing to wonder if it's possible.
It isn't possible: cleanable can be null only when we're a FinalizableZStreamRef, which overrides clean and ignores cleanable. This awkwardness stems from the fact that we *don't* want ZStreamRef to have a finalizer, so we need to inherit in the "unnatural" direction. We could possibly model this more cleanly with an interface and two disjoint implementations, but I don't want to attempt such a refactoring now.
Also can the constructors be grouped as it's very confusing to not have them grouped together (dropping the space in "ZStreamRef (" would help too. Otherwise I think the approach is okay.
I've dropped the ZStreamRef(long) constructors and fixed a few whitespace issues in the latest webrev. /Claes
On 14/12/2017 12:54, Claes Redestad wrote:
:
Also can the constructors be grouped as it's very confusing to not have them grouped together (dropping the space in "ZStreamRef (" would help too. Otherwise I think the approach is okay.
I've dropped the ZStreamRef(long) constructors and fixed a few whitespace issues in the latest webrev. webrev.01 looks good to me.
-Alan
Hi Claes, Looks good; though I would rename the nested classes to have different names to avoid reader confusion. Like InflaterZStreamRef and DeflaterZStreamRef. No further review if you take up the renames. Thanks, Roger On 12/14/2017 9:20 AM, Claes Redestad wrote:
On 2017-12-14 15:16, Alan Bateman wrote:
webrev.01 looks good to me.
Thanks, Alan!
/Claes
On 2017-12-14 16:03, Roger Riggs wrote:
Hi Claes,
Looks good; though I would rename the nested classes to have different names to avoid reader confusion. Like InflaterZStreamRef and DeflaterZStreamRef.
Sure: http://cr.openjdk.java.net/~redestad/8193507/open.02/
No further review if you take up the renames.
I'll go ahead and push once tests complete. :-) Thanks! /Claes
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. 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
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
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
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
HI Peter, We are back to the "the order of resource relocation and cleaner registration" discussion again :-) 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?) or try to catch an unexpected unchecked exception. 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. 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. 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
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_cleanupImp... 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
On 12/18/17, 12:22 AM, Peter Levart wrote:
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_cleanupImp...
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.
Looks good. Let's do it for jdk11. -Sherman
On 12/18/17 12:22 AM, Peter Levart wrote:
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_cleanupImp...
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.
On 12/18/17 11:42 AM, mandy chung wrote:
On 12/18/17 12:22 AM, Peter Levart wrote:
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_cleanupImp...
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.
This patch looks good to me. I agree that this is not critical to go to JDK 10. Mandy
participants (6)
-
Alan Bateman
-
Claes Redestad
-
mandy chung
-
Peter Levart
-
Roger Riggs
-
Xueming Shen