[8u] RFR: 8240676: Meet not symmetric failure when running lucene on jdk8

[Andrew Dinn]

On 24/07/2020 10:26, Roland Westrelin wrote:
>> The changes to Type::meet_helper and Type::check_symmetrical look fine.
>>
>> However, I don't understand what the cherry-picked change to line 3996
>> in TypeAryPtr::xmeet_helper does and why it is legitimate:
>>
>> -      return make(NotNull, NULL, tary, lazy_klass, false, off,
>> InstanceBot);
>> +      return make(NotNull, NULL, tary, lazy_klass, false, off,
>> InstanceBot, speculative, depth);
>>
>> Obviously it fixes a crash but -- for the record -- can you explain
>>
>>   1) why the crash happened and how this fixes it
> 
> The background for this patch is the following: we saw a rare crash
> during testing. The crash couldn't be reproduced. My attempts at a test
> case didn't succeed either. So instead, I made a change to the
> verification code in the type system so it stress tested some
> combinations of types that were usually rarely exercised. It was then
> easy to write a test case that triggered the failure and implement a
> fix.

Ok, understood.

> The risk with this change is not so much in the fix itself but in the
> improvement to the verification code that can uncover bugs that we were
> not aware of before. That's what happens with 8u where we hit a bug that
> was never seen with 8u before.

Ok, but all the verification code happens under #ifdef ASSERT so that is
only going to change behaviour in non-production builds right?

i.e. the important change is the one to the meet code?

> Object pointer types have 2 parts: a known type part and a speculative
> part. When the verification code triggers it verify both parts. In the
> case of this fix, the speculative parts gets accidentally dropped. The
> new verification code catches it. The previous one didn't for some
> reason.

Ah ok, I get this now. The change ensures that the speculative type of
the meet type is the meet of the respective speculative types.

That may well change behaviour for some programs as meets are computed
outside of the changed verification path. I'd like to assume the
benefits of improving type accuracy override the risk. Do you think that
is justified? (one might argue that improved type accuracy is not always
better, especially for speculative info where avoiding the erasure might
enable optimizations not previously attempted).

>>   2) why this was not needed in the upstream patch and is needed here
> 
> I cherry-picked the change from a later release (jdk 9 I think). So the
> change was not needed in the 11u patch because it was already there.
Doh! Of course. Thanks for the explanation.

Well, the change looks good to me but I'm not really in a position to
assess the risk of the xmeet change. I am reassured that it exists in
the upstream code and is not known to have caused any errors.

regards,


Andrew Dinn
-----------
Red Hat Distinguished Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill