[10] RFR: 8193507: [REDO] Startup regression due to JDK-8185582

Xueming Shen xueming.shen at oracle.com
Sat Dec 16 19:29:24 UTC 2017


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
>>>>
>>>>
>>>
>>
>



More information about the core-libs-dev mailing list