On Fri, 13 Jan 2023 00:57:14 GMT, Archie L. Cobbs <[email protected]> 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
