Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8266074: Vtable-based CHA implementation #3727

Closed
wants to merge 8 commits into from

Conversation

@iwanowww
Copy link

@iwanowww iwanowww commented Apr 27, 2021

As of now, Class Hierarchy Analysis (CHA) employs an approximate algorithm to enumerate all non-abstract methods in a class hierarchy.

It served quite well for many years, but it accumulated significant complexity
to support different corner cases over time and inevitable evolution of the JVM
stretched the whole approach way too much (to the point where it become almost
impossible to extend the analysis any further).

It turns out the root problem is the decision to reimplement method resolution
and method selection logic from scratch and to perform it on JVM internal
representation. It makes it very hard to reason about correctness and the
implementation becomes sensitive to changes in internal representation.

So, the main motivation for the redesign is twofold:

  • reduce maintenance burden and increase confidence in the code;
  • unlock some long-awaited enhancements.

Though I did experiment with relaxing existing constraints (e.g., enable default method support),
any possible enhancements are deliberately kept out of scope for the current PR.
(It does deliver a bit of minor enhancements front as the changes in
compiler/cha/StrengthReduceInterfaceCall.java manifest, but it's a side effect
of the other changes and was not the goal of the current work.)

Proposed implementation (LinkedConcreteMethodFinder) mimics method invocation
and relies on vtable/itable information to detect target method for every
subclass it visits. It removes all the complexity associated with method
resolution and method selection logic and leaves only essential logic to prepare for method selection.

Vtables are filled during class linkage, so new logic doesn't work on not yet linked classed.
Instead of supporting not yet linked case, it is simply ignored. It is safe to
skip them (treat as "effectively non-concrete") since it is guaranteed there
are no instances created yet. But it requires VM to check dependencies once a
class is linked.

I ended up with 2 separate dependency validation passes (when class is loaded
and when it is linked). To avoid duplicated work, only dependencies
which may be affected by class initialization state change
(unique_concrete_method_4) are visited.

(I experimented with merging passes into a single pass (delay the pass until
linkage is over), but it severely affected other class-related dependencies and
relevant optimizations.code.)

Compiler Interface (CI) is changed to require users to provide complete information about the call site being analyzed.

Old implementation is kept intact for now (will be removed later) to:

  • JVMCI hasn't been migrated to the new implementation yet;
  • enable verification that 2 implementations (old and new) agree on the results;
  • temporarily keep an option to revert to the original implementation in case any regressions show up.

Testing:

  • hs-tier1 - hs-tier9
  • hs-tier1 - hs-tier4 w/ -XX:-UseVtableBasedCHA
  • performance testing

Thanks!


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3727/head:pull/3727
$ git checkout pull/3727

Update a local copy of the PR:
$ git checkout pull/3727
$ git pull https://git.openjdk.java.net/jdk pull/3727/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3727

View PR using the GUI difftool:
$ git pr show -t 3727

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3727.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 27, 2021

👋 Welcome back vlivanov! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 27, 2021

@iwanowww The following label will be automatically applied to this pull request:

  • hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

Loading

@openjdk openjdk bot added the hotspot label Apr 27, 2021
@iwanowww
Copy link
Author

@iwanowww iwanowww commented Apr 27, 2021

/label add hotspot-compiler

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 27, 2021

@iwanowww
The hotspot-compiler label was successfully added.

Loading

@iwanowww iwanowww force-pushed the 8266074.cha_vtable branch 2 times, most recently from 6e0836c to 7d94997 Apr 28, 2021
@iwanowww iwanowww force-pushed the 8266074.cha_vtable branch from 7d94997 to 3063f97 Apr 29, 2021
@iwanowww iwanowww marked this pull request as ready for review Apr 29, 2021
@openjdk openjdk bot added the rfr label Apr 29, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 29, 2021

Loading

Copy link
Contributor

@vnkozlov vnkozlov left a comment

In general looks good. I have few comments.

Loading

@@ -32,13 +32,15 @@
* @run driver jdk.test.lib.helpers.ClassFileInstaller sun.hotspot.WhiteBox
*
* @run main/othervm -Xbootclasspath/a:. -XX:+IgnoreUnrecognizedVMOptions -XX:+UnlockDiagnosticVMOptions
* -XX:+UseVtableBasedCHA
Copy link
Contributor

@vnkozlov vnkozlov Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UseVtableBasedCHA is true by default. May be duplicate these runs for both cases: on and off.

Loading

Copy link
Author

@iwanowww iwanowww Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test assumes UseVtableBasedCHA is on and some of the assertions it makes fail with -XX:-UseVtableBasedCHA. So, it would require all the changes in the test logic have to be guarded by a check whether UseVtableBasedCHA is on or off.

Considering UseVtableBasedCHA is turned on by default in the PR, I don't see much value in complicating the test.

Loading

Copy link
Contributor

@vnkozlov vnkozlov Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So only new implementation is tested with these changes in test. Got it. May be use:
@requires vm.opt.final.UseVtableBasedCHA == true

Loading

Copy link
Author

@iwanowww iwanowww Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Contributor

@vnkozlov vnkozlov Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let hear @iignatev opinion.

Loading

Copy link
Member

@iignatev iignatev May 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

-- Igor

Loading

src/hotspot/share/code/dependencies.hpp Show resolved Hide resolved
Loading
rose00
rose00 approved these changes Apr 30, 2021
Copy link
Contributor

@rose00 rose00 left a comment

This is a major bit of stewardship. Here's a bit of old history: The approximate dependencies walk code was hacked in the earliest days, for devirtualization—a new and cool thing back then. At the same time the vtable code was hacked in, and both sets of logic were hacked until the bugs went away. It took a while for all of us to fully understand the VM we were building. (This is why we had to add ACC_SUPER and class loader dependencies constraints, for example.) A couple engineers cleaned up the vtable stuff to its present state, and I and others cleaned up the CHA logic to its present state, before you touched it. It’s great to see those two bodies of code converging; thank you very very much.

Reviewed!

One question, which the review doesn't depend on: Are get getting closer to being able to apply CHA to interfaces? Unique-implementor and unique-method optimizations on interfaces are important. I ask because of some comments that throw shade on interfaces in the unit tests.

Loading

bool implements_interface; // initialized by method_at_itable_or_null()
selected_method = recv_klass->method_at_itable_or_null(_declaring_klass, _vtable_index,
implements_interface); // out parameter
assert(implements_interface, "not implemented");
Copy link
Contributor

@rose00 rose00 Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at recv_klass->method_at_itable_or_null, I wonder if there can be “holes” in the itable for missing methods. They would lead to AME if called. They might also trigger your assert here: assert(implements_interface, "not implemented"). Is there some reason that select_method cannot possibly encounter a missing method?

Answer to self: I don't remember whether the JVM creates itable methods on the fly, but I suppose it does, so the code would see an synthetic abstract method. (Decades ago we named those Miranda Methods because if you don't have a responding method "one will be provided for you".) And itables are just aliases of vtable slices, so the miranda placed in the vtable will be seen also in the itable.

(Overall comment on this area of the code: It looks great, much better than when I touched it last. Thanks!)

Loading

Copy link
Author

@iwanowww iwanowww Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding "holes" in itables: yes, it happens in practice. select_method is allowed to return a NULL and it is used by LinkedConcreteMethodFinder as a sentinel value for AME (when placed in _found_methods array). That's why Dependencies::find_unique_concrete_method() check participant(0):

   Method* fm = wf.found_method(0);  // Will be NULL if num_parts == 0.
   Klass*   p = wf.participant(0);   // Will be NULL if num_parts == 0.
   ...
   if (Dependencies::is_concrete_method(m, ctxk)) {
     if (fm == NULL && p == NULL) {
       // It turns out that m was always the only implementation.
       fm = m;
     }

Now I think that it's not clear enough from the code. I'll elaborate on it in the additional comments.

Loading

Copy link
Author

@iwanowww iwanowww Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the assert: the itable should always be found. First, because only implementors are enumerated during the analysis (currently, subclasses of the unique direct implementor). Second, because itable stubs rely on itable contents when performing receiver subtype checks. So, all superinterfaces should have a corresponding itable present (even an empty one).

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 30, 2021

@iwanowww This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8266074: Vtable-based CHA implementation

Reviewed-by: kvn, jrose, dlong

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 257 new commits pushed to the master branch:

  • 2568d18: 8267047: Put serviceability/sa/TestJmapCoreMetaspace.java back on ZGC problem list due to JDK-8267045
  • accbfea: 8226216: parameter modifiers are not visible to javac plugins across compilation boundaries
  • 69daedf: 8266845: Shenandoah: Simplify SBS::load_reference_barrier implementation
  • 7433821: 8250658: Performance of ClipFlatOval Renderperf test is very low
  • 4727187: 8266567: Fix javadoc tag references in sun.management.jmxremote.ConnectorBootstrap
  • 11759bf: 8266798: C1: More types of instruction can also apply LoopInvariantCodeMotion
  • dcf250d: 8233378: CHT: Fast reset
  • f3b510b: 8266923: [JVMCI] expose StackOverflow::_stack_overflow_limit to JVMCI
  • 548899d: 8266189: Remove C1 "IfInstanceOf" instruction
  • b46086d: 8266874: Clean up C1 canonicalizer for TableSwitch/LookupSwitch
  • ... and 247 more: https://git.openjdk.java.net/jdk/compare/f560b8923378042b1cb162de4e1c65ea1c875af0...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added the ready label Apr 30, 2021
@iwanowww
Copy link
Author

@iwanowww iwanowww commented Apr 30, 2021

Are get getting closer to being able to apply CHA to interfaces?

Yes! Though it is always the case now that any interface method case can be strength-reduced to a virtual call (due to unique implementor constraint), I deliberately kept interface dispatch support in place. Once there's a reliable way to enumerate all implementors, it becomes straightforwad to apply CHA when interface type information can be trusted.

And, as a first step, it would be very interesting to experiment with sealed interface support .

Loading

Copy link
Member

@dean-long dean-long left a comment

Looks good.

Loading

@iwanowww
Copy link
Author

@iwanowww iwanowww commented May 13, 2021

Thanks for the reviews, John, Vladimir, and Dean.

/integrate

Loading

@openjdk openjdk bot closed this May 13, 2021
@openjdk openjdk bot added the integrated label May 13, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 13, 2021

@iwanowww Since your change was applied there have been 264 commits pushed to the master branch:

  • 347d41d: 8164804: sun/security/ssl/SSLSocketImpl/CloseSocket.java makes not reliable time assumption
  • 17ceef9: 8266819: Separate the stop policies from the compile policies completely
  • a270cbe: 8267043: IntelliJ project doesn't handle generated sources correctly
  • 08a5a5c: 8263382: java/util/logging/ParentLoggersTest.java failed with "checkLoggers: getLoggerNames() returned unexpected loggers"
  • b50fc5f: 8265528: Specification of BasicSplitPaneDivider::getMinimumSize,getPreferredSize doesn't match with its behavior.
  • d215743: 8231031: runtime/ReservedStack/ReservedStackTest.java fails after jsr166 refresh
  • ab17be2: 8252530: Fix inconsistencies in hotspot whitebox
  • 2568d18: 8267047: Put serviceability/sa/TestJmapCoreMetaspace.java back on ZGC problem list due to JDK-8267045
  • accbfea: 8226216: parameter modifiers are not visible to javac plugins across compilation boundaries
  • 69daedf: 8266845: Shenandoah: Simplify SBS::load_reference_barrier implementation
  • ... and 254 more: https://git.openjdk.java.net/jdk/compare/f560b8923378042b1cb162de4e1c65ea1c875af0...master

Your commit was automatically rebased without conflicts.

Pushed as commit 127bfe4.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

Mailing list message from David Holmes on hotspot-dev:

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> 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
-----

Loading

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

Mailing list message from David Holmes on hotspot-dev:

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> 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
-----

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

Mailing list message from Igor Ignatev on hotspot-dev:

Hi 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.

-- Igor

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

Loading

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

Mailing list message from Igor Ignatev on hotspot-dev:

Hi 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.

-- Igor

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

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

Mailing list message from David Holmes on hotspot-dev:

On 2/05/2021 2:00 pm, Igor Ignatev wrote:

Hi 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.

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.

Cheers,
David

-- Igor

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
-----

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

Mailing list message from Vladimir Ivanov on hotspot-dev:

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
-----

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

Mailing list message from David Holmes on hotspot-dev:

On 2/05/2021 2:00 pm, Igor Ignatev wrote:

Hi 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.

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.

Cheers,
David

-- Igor

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
-----

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 14, 2021

Mailing list message from Vladimir Ivanov on hotspot-dev:

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
-----

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants