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

8332111: [BACKOUT] A way to align already compiled methods with compiler directives #19215

Closed
wants to merge 1 commit into from

Conversation

eastig
Copy link
Member

@eastig eastig commented May 13, 2024

Backout of JDK-8309271 which has known bugs, possible bugs and performance issues. REDO work is tracked by JDK-8331749.

Found bugs:

  • When refreshing CompilerDirectivesAddDCmd::execute will call DirectivesStack::hasMatchingDirectives(mh, true) which only considers the compiler directive which is on the top of the directives stack. As more than one directive can be added, CompilerDirectivesAddDCmd::execute will not behave as expected.
  • A Java method with old directives might be in the compilation queue. A request to recompile it with new directives will be ignored.

There are other concerns: bugs and performance issues.

Possible bugs:

  • has_matching_directives might not be cleared. A nmethod might get into the unloading state before CodeCache::recompile_marked_directives_matches. If the nmethod has been used to mark a Java method and it is the only nmethod, there will be no nmethod in CodeCache to reach the Java method to clear the mark.
  • A Java method might have been compiled with new directives before CodeCache::recompile_marked_directives_matches. CodeCache::recompile_marked_directives_matches will recompile it again.
  • JIT compiler might be compiling a Java method with old directives. A request to recompile it with new directives will be ignored.

Performance issues:

  • Usually directives are updated for a small number of Java methods. If CodeCache has thousands of nmethods, CodeCache::recompile_marked_directives_matches will be traversing nmethods most of which don't need recompilation.

The backout is not clean because of removal of CompiledMethod.

Tested with release and fastdebug builds: tier1 and tier2 passed.


Progress

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

Issue

  • JDK-8332111: [BACKOUT] A way to align already compiled methods with compiler directives (Sub-task - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19215

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented May 13, 2024

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

@eastig
Copy link
Member Author

eastig commented May 13, 2024

/cc hotspot-compiler

@openjdk
Copy link

openjdk bot commented May 13, 2024

@eastig 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:

8332111: [BACKOUT] A way to align already compiled methods with compiler directives

Reviewed-by: shade, kvn

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 7 new commits pushed to the master branch:

  • ea5eb74: 8326404: Assertion error when trying to compile switch with fallthrough with pattern
  • beea530: 8331907: BigInteger and BigDecimal should use optimized division
  • 440782e: 8331466: Problemlist serviceability/dcmd/gc/RunFinalizationTest.java on generic-all
  • 5ded8da: 8332085: Remove 10 year old transition check in GenerateCurrencyData tool
  • 7c2c24f: 8261433: Better pkcs11 performance for libpkcs11:C_EncryptInit/libpkcs11:C_DecryptInit
  • ff4bf1c: 8332102: Add @since to package-info of jdk.security.jarsigner
  • abf54bb: 8332100: Add missing @since to KeyValue::EC_TYPE in java.xml.crypto

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

@openjdk openjdk bot added rfr Pull request is ready for review hotspot-compiler hotspot-compiler-dev@openjdk.org labels May 13, 2024
@openjdk
Copy link

openjdk bot commented May 13, 2024

@eastig
The hotspot-compiler label was successfully added.

@eastig
Copy link
Member Author

eastig commented May 13, 2024

/label hotspot

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label May 13, 2024
@openjdk
Copy link

openjdk bot commented May 13, 2024

@eastig
The hotspot label was successfully added.

@eastig
Copy link
Member Author

eastig commented May 13, 2024

/label serviceability

@openjdk openjdk bot added the serviceability serviceability-dev@openjdk.org label May 13, 2024
@openjdk
Copy link

openjdk bot commented May 13, 2024

@eastig
The serviceability label was successfully added.

@mlbridge
Copy link

mlbridge bot commented May 13, 2024

Webrevs

Copy link
Member

@shipilev shipilev left a comment

Choose a reason for hiding this comment

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

The reversal looks fine.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label May 13, 2024
@dchuyko
Copy link
Member

dchuyko commented May 13, 2024

Are there any high severity problems caused by the original PR? Especially not in the new functionality. Minor issues could be probably addressed without backing out the entire functionality.

@eastig
Copy link
Member Author

eastig commented May 13, 2024

Are there any high severity problems caused by the original PR? Especially not in the new functionality. Minor issues could be probably addressed without backing out the entire functionality.

Yes, there are:

  1. Usually directives are updated for a small number of Java methods. If CodeCache has thousands of nmethods, CodeCache::recompile_marked_directives_matches will be traversing nmethods most of which don't need recompilation.
  2. has_matching_directives might not be cleared.
  3. A Java method is not recompiled as requested.

@dchuyko
Copy link
Member

dchuyko commented May 13, 2024

Are there any high severity problems caused by the original PR? Especially not in the new functionality. Minor issues could be probably addressed without backing out the entire functionality.

Yes, there are:

  1. Usually directives are updated for a small number of Java methods. If CodeCache has thousands of nmethods, CodeCache::recompile_marked_directives_matches will be traversing nmethods most of which don't need recompilation.
  1. has_matching_directives might not be cleared.
  1. A Java method is not recompiled as requested.

So there are cases when new functionality doesn't work as expected (I don't see any other users impacted). Why not file bugs for those cases and estimate their impact?

@eastig
Copy link
Member Author

eastig commented May 13, 2024

So there are cases when new functionality doesn't work as expected (I don't see any other users impacted). Why not file bugs for those cases and estimate their impact?

Do you know any users using the new functionality?

@eastig
Copy link
Member Author

eastig commented May 13, 2024

IMO if nobody uses it and the amount of code is small, it is better to back out it and to reimplement it.

@vnkozlov
Copy link
Contributor

@eastig do you have tests which shows issues you listed in description? I don't see any reference to them in this sub-task and in [REDO] bug JDK-8331749.
How you found these issues?

@eastig
Copy link
Member Author

eastig commented May 13, 2024

do you have tests which shows issues you listed in description?

Here is a jtreg test:

  • refresh_control.02.txt
[
  {
    match: "serviceability.dcmd.compiler.DirectivesRefreshTest::callable",
    c2: {
      PrintOptoAssembly: true
    }
  }
]
  • DirectivesRefreshTest02.java
/**
 * @test DirectivesRefreshTest02
 * @summary Test of forced recompile after compiler directives changes by diagnostic command
 * @requires vm.compiler1.enabled & vm.compiler2.enabled
 * @library /test/lib /
 * @modules java.base/jdk.internal.misc
 *
 * @build jdk.test.whitebox.WhiteBox
 * @run driver jdk.test.lib.helpers.ClassFileInstaller jdk.test.whitebox.WhiteBox
 *
 * @run main/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI
 *                   -XX:+BackgroundCompilation -Xlog:codecache=trace -XX:-Inline -XX:+TieredCompilation -XX:CICompilerCount=2
 *                   -XX:+UnlockDiagnosticVMOptions
 *                   serviceability.dcmd.compiler.DirectivesRefreshTest02
 */

package serviceability.dcmd.compiler;

import jdk.test.whitebox.WhiteBox;

import jdk.test.lib.process.OutputAnalyzer;
import jdk.test.lib.dcmd.CommandExecutor;
import jdk.test.lib.dcmd.JMXExecutor;

import java.nio.file.Path;
import java.nio.file.Paths;
import java.lang.reflect.Method;
import java.util.Random;

import static jdk.test.lib.Asserts.assertEQ;

import static compiler.whitebox.CompilerWhiteBoxTest.COMP_LEVEL_NONE;
import static compiler.whitebox.CompilerWhiteBoxTest.COMP_LEVEL_SIMPLE;
import static compiler.whitebox.CompilerWhiteBoxTest.COMP_LEVEL_FULL_OPTIMIZATION;

public class DirectivesRefreshTest02 {

    static Path cmdPath = Paths.get(System.getProperty("test.src", "."), "refresh_control.02.txt");
    static WhiteBox wb = WhiteBox.getWhiteBox();
    static Random random = new Random();

    static Method method;
    static CommandExecutor executor;

    static int callable() {
        int result = 0;
        for (int i = 0; i < 100; i++) {
            result += random.nextInt(100);
        }
        return result;
    }

    static void setup() throws Exception {
        method = DirectivesRefreshTest.class.getDeclaredMethod("callable");
        executor = new JMXExecutor();

        wb.enqueueMethodForCompilation(method, COMP_LEVEL_SIMPLE);
        while (wb.isMethodQueuedForCompilation(method)) {
            Thread.onSpinWait();
        }
        wb.lockCompilation();
        boolean r = wb.enqueueMethodForCompilation(method, COMP_LEVEL_FULL_OPTIMIZATION);
        System.out.println("Method enqueued: " + r);
    }

    static void testDirectivesAddRefresh() {
        var output = executor.execute("Compiler.directives_add -r " + cmdPath.toString());
        output.stderrShouldBeEmpty().shouldContain("1 compiler directives added");
        System.out.println("Method enqueued: " + wb.isMethodQueuedForCompilation(method));
        wb.unlockCompilation();

        wb.enqueueMethodForCompilation(method, COMP_LEVEL_FULL_OPTIMIZATION);
        while (wb.isMethodQueuedForCompilation(method)) {
            Thread.onSpinWait();
        }
        System.out.println("Method compilation level: " + wb.getMethodCompilationLevel(method));

        assertEQ(true, false, "Stop here");
    }

    public static void main(String[] args) throws Exception {
        setup();
        testDirectivesAddRefresh();
    }
}

@eastig
Copy link
Member Author

eastig commented May 13, 2024

There is no PrintOptoAssembly in output.
I use lockCompilation()/unlockCompilation() to simulate:

A Java method with old directives might be in the compilation queue. A request to recompile it with new directives will be ignored.

I think using them we can also simulate, though it would not be easy to write a test:

JIT compiler might be compiling a Java method with old directives. A request to recompile it with new directives will be ignored.

@eastig
Copy link
Member Author

eastig commented May 13, 2024

How you found these issues?

I've been backporting JDK-8309271 to downstream 17 and 21. As compilations happens in background but a test from JDK-8309271 runs with background compilation off, I asked myself what might happen with background compilation. I have a patch fixing the test above. I don't think it is a complete fix. There is a race among a thread updating directives, compiler threads and CodeCache cleaning threads. We don't properly lock the directives stack, the compile queue and CodeCache to manage the race.

@eastig
Copy link
Member Author

eastig commented May 13, 2024

What if instead of backing out we will use an experimental JVM flag: XX:+CompilerDirectivesRefreshSupport?

@vnkozlov
Copy link
Contributor

There is a race among a thread updating directives, compiler threads and CodeCache cleaning threads. We don't properly lock the directives stack, the compile queue and CodeCache to manage the race.

This is indeed concerning.

@vnkozlov
Copy link
Contributor

What if instead of backing out we will use an experimental JVM flag: XX:+CompilerDirectivesRefreshSupport?

I don't think this is correct way to fix the bug.

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

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

I agree with this backout. Thank you @eastig for explaining your point.
We have about 3 weeks before RDP1 and it is better we have less issues before that.
Let redo implementation in next release taking into account the issues you found and have more time for testing.

@dchuyko
Copy link
Member

dchuyko commented May 14, 2024

I agree with this backout. Thank you @eastig for explaining your point. We have about 3 weeks before RDP1 and it is better we have less issues before that. Let redo implementation in next release taking into account the issues you found and have more time for testing.

OK. I hope it takes less time to get back into the source tree than it did initially.

@eastig
Copy link
Member Author

eastig commented May 15, 2024

Thank you, @vnkozlov @dchuyko @shipilev

@eastig
Copy link
Member Author

eastig commented May 15, 2024

/integrate

@openjdk
Copy link

openjdk bot commented May 15, 2024

Going to push as commit 1a94447.
Since your change was applied there have been 20 commits pushed to the master branch:

  • 957eb61: 8331429: [JVMCI] Cleanup JVMCIRuntime allocation routines
  • 2f10a31: 8302850: Implement C1 clone intrinsic that reuses arraycopy code for primitive arrays
  • c642f44: 8329839: Cleanup ZPhysicalMemoryBacking trace logging
  • d04ac14: 8332236: javac crashes with module imports and implicitly declared class
  • 4e77cf8: 8330795: C2: assert((uint)type <= T_CONFLICT && _zero_type[type] != nullptr) failed: bad type with -XX:-UseCompressedClassPointers
  • 7b4ba7f: 8325932: Replace ATTRIBUTE_NORETURN with direct [[noreturn]]
  • 0bb5ae6: 8332248: (fc) java/nio/channels/FileChannel/BlockDeviceSize.java failed with RuntimeException
  • 4d32c60: 8322008: Exclude some CDS tests from running with -Xshare:off
  • e91492a: 8313674: (fc) java/nio/channels/FileChannel/BlockDeviceSize.java should test for more block devices
  • 95a6013: 8332042: Move MEMFLAGS to its own include file
  • ... and 10 more: https://git.openjdk.org/jdk/compare/1484153c1a092cefc20270b35aa1e508280843a4...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label May 15, 2024
@openjdk openjdk bot closed this May 15, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 15, 2024
@openjdk
Copy link

openjdk bot commented May 15, 2024

@eastig Pushed as commit 1a94447.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated serviceability serviceability-dev@openjdk.org
Development

Successfully merging this pull request may close these issues.

4 participants