jdk icon indicating copy to clipboard operation
jdk copied to clipboard

8333891: Method excluded with directive is not compiled after removal of directive

Open eastig opened this issue 1 year ago • 6 comments
trafficstars

A Java method can become non-compilable if there are issues with its compilation or if its compiled version causes problems. Additionally, a method can be marked as non-compilable using a compile command or a compiler directive. Since compiler directives can be updated, a directive that disables a method's compilation can be changed or removed.

Currently, when a Java method is marked as non-compilable, the reason for this status is unknown. If a change in a compiler directive makes the method compilable again, we cannot clear the non-compilable status because we don't know if the directive initially caused the method to become non-compilable.

To resolve the issue two method flags are introduced: is_c1_exclude and is_c2_excluded. They mean a Java method is excluded from compilation by a directive. With these flags we can find out a Java method has been excluded with a directive. If the directive changes and allows compilation of the method we can detect this and clear the non-compilable status.

As accesses to flags must be race free we have to remove getting a directive from CompileBroker::compile_method. We combine two CompileBroker::compile_method into one. We also move calculation whether compilation is blocking into CompileBroker::compile_method_base. The directive used for that calculation is passed to a compile task, so a compile task does not need to get it again.

A regression test is added.

Tested fastdebug build on Linux AArch64, Linux x86_64 and Windows Server 2019 x86_64:

  • Tier1/2/3 passed.

Progress

  • [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • [x] Change must not contain extraneous whitespace
  • [x] Commit message must refer to an issue

Issue

  • JDK-8333891: Method excluded with directive is not compiled after removal of directive (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19637/head:pull/19637
$ git checkout pull/19637

Update a local copy of the PR:
$ git checkout pull/19637
$ git pull https://git.openjdk.org/jdk.git pull/19637/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19637

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19637.diff

Webrev

Link to Webrev Comment

eastig avatar Jun 10 '24 20:06 eastig

:wave: Welcome back eastigeevich! 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.

bridgekeeper[bot] avatar Jun 10 '24 20:06 bridgekeeper[bot]

❗ This change is not yet ready to be integrated. See the Progress checklist in the description for automated requirements.

openjdk[bot] avatar Jun 10 '24 20:06 openjdk[bot]

@eastig The following labels will be automatically applied to this pull request:

  • hotspot-compiler
  • serviceability

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

openjdk[bot] avatar Jun 10 '24 20:06 openjdk[bot]

Webrevs

mlbridge[bot] avatar Jun 10 '24 20:06 mlbridge[bot]

Test runtime/BootstrapMethod/BSMCalledTwice.java might have failed on Windows x64 because of the change.

eastig avatar Jun 12 '24 20:06 eastig

Test runtime/BootstrapMethod/BSMCalledTwice.java might have failed on Windows x64 because of the change.

I managed to reproduce the failure. The test fails because of my change. There is a data race:

Thread1: cleaning_flag ... setting_flag ... assert
Thread2: cleaning_flag ... setting_flag ... assert

Thread2 can clean the flag between Thread1 setting the flag and checking the assert.

eastig avatar Jun 17 '24 18:06 eastig

@eastig This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper[bot] avatar Jul 29 '24 18:07 bridgekeeper[bot]

@eastig This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

bridgekeeper[bot] avatar Aug 26 '24 22:08 bridgekeeper[bot]

@eastig are you still working on this? Do you want to reopen it?

dafedafe avatar Sep 02 '24 11:09 dafedafe

Hi @dafedafe, IMO as the fix is not simple it might not be worth to merge. The fix uses method flags. I have not seen compiler directives are used a lot, especially the case: add a directive and remove the directive. This can be waste of method flags. This PR got linked to the JBS issue. When more users complain of the issue, we can reconsider the fix.

eastig avatar Sep 02 '24 20:09 eastig

You're right: the use-case seems quite a peculiar one. Thanks anyway @eastig.

dafedafe avatar Sep 03 '24 07:09 dafedafe