RFR: 8266074: Vtable-based CHA implementation

[Vladimir Ivanov]

Thanks for your feedback, Igor and David.

>> I meant both: in a generic sense, meaning we won't properly document, 
>> advertise it or claim it as supported; and changing its type to 
>> EXPERIMENTAL, so it will be somewhat harder for people to switch it.
> 
> Both forms are as hard to switch to as both must be unlocked. But 
> semantically this is not an experimental flag IMO.

I agree.

> Regardless, if the intent is to allow the flag to be used to restore 
> legacy behaviour, then that legacy behaviour must be tested. We don't 
> have to run thousands of tests with the flag disabled, just something 
> representative enough to give us confidence that the code has not 
> bit-rotted.

Yes, it makes perfect sense to test that -XX:-UseVtableBasedCHA is usable.

I'd like to point out that new implementation exercise old 
implementation [1] to verify that there's an agreement on the result.

So, maybe just a smoke test is enough here.

Best regards,
Vladimir Ivanov

[1] src/hotspot/share/code/dependencies.cpp:

Method* Dependencies::find_unique_concrete_method(InstanceKlass* ctxk, 
Method* m, Klass* resolved_klass, Method* resolved_method) {
...
   // Old CHA conservatively reports concrete methods in abstract classes
   // irrespective of whether they have concrete subclasses or not.
#ifdef ASSERT
   Klass*  uniqp = NULL;
   Method* uniqm = Dependencies::find_unique_concrete_method(ctxk, m, 
&uniqp);
   assert(uniqm == NULL || uniqm == fm ||
          uniqm->method_holder()->is_abstract() ||
          (fm == NULL && uniqm != NULL && uniqp != NULL && 
!InstanceKlass::cast(uniqp)->is_linked()),
          "sanity");
#endif // ASSERT


>>> On May 1, 2021, at 3:03 PM, David Holmes <david.holmes at oracle.com 
>>> <mailto:david.holmes at oracle.com>> wrote:
>>>
>>> On 2/05/2021 1:39 am, Igor Ignatyev wrote:
>>>> On Fri, 30 Apr 2021 22:10:02 GMT, Vladimir Kozlov <kvn at openjdk.org 
>>>> <mailto:kvn at openjdk.org>> wrote:
>>>>>> I'm fine with both approaches.
>>>>>>
>>>>>> Explicitly setting the flag looked to me more robust and clearer 
>>>>>> communicating the intent. But if you prefer `@requires`, I'll use it.
>>>>>
>>>>> Let hear @iignatev opinion.
>>>> from my point of view, `@requires` is clearer and also eliminates 
>>>> "wasted" execution (if someone tries to run this test w/ 
>>>> `-XX:-UseVtableBasedCHA`), so I'd prefer if we use it.
>>>> I have a more generic comment about `UseVtableBasedCHA`. I 
>>>> understand the desire to introduce a flag to switch back to the old 
>>>> implementation, but I'm somewhat concern that it adds a new 
>>>> dimension into configuration space that won't be covered by our 
>>>> existing tests (w/ the test which exercises interesting parts of the 
>>>> related code is inapplicable) and isn't part of our regular test 
>>>> configurations. Can we make it an experimental flag (w/ vtable-based 
>>>> CHA still being enabled by default)? this way, the quality bar for 
>>>> the old implementation will be somewhat lower, yet the end-users 
>>>> will still be able to return to the old implementation if it, for 
>>>> some reason, works better in their use-cases.
>>>
>>> Did you mean "experimental" in a generic sense or actually change it 
>>> from DIAGNOSTIC to EXPERIMENTAL? If the latter then I don't agree 
>>> this is an experimental flag, it is diagnostic. But either way the 
>>> testing requirements are the same if we expect to tell end users to 
>>> try this flag if they hit an problem - the flag has to be known to be 
>>> functional, so we will have to expand the test coverage.
>>>
>>> Cheers,
>>> David
>>> -----
>>>
>>>> -- Igor
>>>> -------------
>>>> PR:https://git.openjdk.java.net/jdk/pull/3727 
>>>> <https://git.openjdk.java.net/jdk/pull/3727>
>>