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
