[10] RFR: 8193507: [REDO] Startup regression due to JDK-8185582
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