On Sat, 14 Jan 2023 01:54:23 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.
>>
>> 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
