On Sat, 14 Jan 2023 01:54:23 GMT, Archie L. Cobbs <d...@openjdk.org> wrote:
>>> Ok - I thought false negative was the thing to absolutely avoid - and that >>> was the no. 1 concern. >> >> You're right. I think at the time I reasoned that it would be unusual enough >> for the type of an expression to start as an instanceof X, then change to >> not being an instanceof X, and then change back, that it was worth it to go >> ahead and filter these out. But admittedly that was based on intuition, not >> science. >> >>> I think a possible approach to keep both the filtering and the code more or >>> less similar to what you have, is to save the type of the expression in the >>> ExprRef. Then, I believe that, at the end of scan() you can just get >>> whatever type is there, and check that it's the correct one, if not drop it. >> >> That's a nice idea... thanks. I'll play around with it some. > > I was curious how much of a difference this type filtering makes, so I built > the JDK with and without it. > > The results were identical except for one case: > > package java.lang.invoke; > ... > public abstract sealed class CallSite permits ConstantCallSite, > MutableCallSite, VolatileCallSite { > ... > CallSite(MethodType targetType, MethodHandle createTargetHook) throws > Throwable { > this(targetType); // need to initialize target to make > CallSite.type() work in createTargetHook > ConstantCallSite selfCCS = (ConstantCallSite) this; > MethodHandle boundTarget = (MethodHandle) > createTargetHook.invokeWithArguments(selfCCS); > setTargetNormal(boundTarget); // ConstantCallSite doesn't publish > CallSite.target > UNSAFE.storeStoreFence(); // barrier between target and isFrozen > updates > } > > When we do type filtering, `(ConstantCallSite) this` causes the 'this' > reference to be discarded so no leak is reported on the next line for > `invokeWithArguments(selfCCS)`. Just a side note, there is also a leak on the > next line because final method `setTargetNormal()` invokes > `MethodHandleNatives.setCallSiteTargetNormal(this, newTarget)`, so a leak > does get reported anyway even with type filtering. > > When type filtering is disabled, we report a leak at > `invokeWithArguments(selfCCS)` - which is accurate. > > So what did we learn? > > First it looks like type filtering has very minimal effect. I think this is > because it requires some version of two casts in a row in and out of type > compatibility, and this is probably very rare. > > But looking at the one data point we do have, the type filtering did in fact > cause a false negative. > > And when building the entire JDK, it causes zero net new false positives. > > So to me this is evidence that we should just remove the type filtering > altogether... > > @vicente-romero-oracle your thoughts? I believe the conclusion you reached is sound - it doesn't look like type filtering adds much, and it can be actively harmful. ------------- PR: https://git.openjdk.org/jdk/pull/11874