On Fri, 13 Jan 2023 00:57:14 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. 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. > >> 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? ------------- PR: https://git.openjdk.org/jdk/pull/11874