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

[Peter Levart]

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