8301578: Perform output outside synchronization in Module.class #12193

minborg · 2023-01-25T12:26:26Z

This PR proposed to reduce contention in synchronized methods mainly by doing I/O operations outside synch blocks.

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-8301578: Perform output outside synchronization in Module.class

Reviewers

Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12193

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

Using diff file

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

bridgekeeper · 2023-01-25T12:28:12Z

👋 Welcome back pminborg! 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.

openjdk · 2023-01-25T12:30:19Z

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

core-libs

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.

minborg · 2023-01-31T17:18:47Z

The potential performance improvement in determining if native access is enabled is a small part of the overall benchmarks in the LoopOverOfAddress benchmarks:

Baseline
Benchmark                                         Mode  Cnt  Score   Error  Units
LoopOverOfAddress.segment_loop_addr_size          avgt   30  0.563 ± 0.005  ms/op


PR
Benchmark                                         Mode  Cnt  Score   Error  Units
LoopOverOfAddress.segment_loop_addr_size          avgt   30  0.558 ± 0.002  ms/op

mlbridge · 2023-01-31T17:53:48Z

Webrevs

AlanBateman · 2023-01-31T18:03:27Z

src/java.base/share/classes/java/lang/Module.java

                }
            }
        }
    }

+    private static boolean casEnableNativeAccess(Module target) {


The change looks okay but the naming is a bit misleading, maybe trySetEnableNativeAccess would be better, as this isn't an Unsafe/VarHandler compareAndSet. Also would be possible to add a comment to the method so it's consistent with the existing methods.

AlanBateman · 2023-01-31T18:19:35Z

BTW: Is this PR meant by openjdk/jdk20 or openjdk/jdk? Does it really address a startup issue or not?

dholmes-ora · 2023-02-01T06:17:37Z

I can't quite see the effect of the change here. You went from a synchronized instance method, to a non-synchronized static method which then immediately synchronizes on the passed in instance. ??

That aside I'm not sure what the synchronization issues are here - isn't setting the native access bit an idempotent operation?

AlanBateman · 2023-02-01T07:12:03Z

I can't quite see the effect of the change here.

@minborg Have you used the wrong JBS issue by any chance? There is no information in the JBS issue, or here, that the proposed change fixes the reported startup regression after the JEP 434 integration and maybe this is a drive-by cleanup while investigating the startup issue?

viktorklang-ora · 2023-02-01T07:21:10Z

src/java.base/share/classes/java/lang/Module.java

+    }
+
+    private static boolean isNativeAccessEnabled(Module target) {
+        if (target.enableNativeAccess)


@minborg It'd seem a bit easier to implement using VarHandle and not having to use synchronized? Is the reason that you don't want to init VarHandle in this class? 🤔

It is a bootstrap problem. We cannot use AtomicBoolean, VarHandle or Unsafe here.

Shouldn’t Unsafe already be loaded at this point?

minborg · 2023-02-01T08:34:55Z

Let me take a step back here and explain the reason why I think this PR is worth looking at. Firstly, I think the argument that this might not solve https://bugs.openjdk.org/browse/JDK-8300917 is perfectly valid. Therefore, I have filed a separate issue and linked this PR to this. Thanks for pointing this out. This PR is more like a drive-by cleanup as mentioned.

Back to the contents of the PR. My fear (which may or may not be substantiated) is that if we perform operations via the relatively slow System.err.printf, while being in a synchronization block, we are exposed to a number of potential problems:

Another thread might be performing System.err operations which will be under a separate lock/sync
Other threads are calling ensureNativeAccess for the same module
A "storm of calls" during bootstrap
Combinations of any of the above

Again, this might not be the case in reality but I think removing the risk is worthwhile.

ExE-Boss · 2023-02-05T18:42:05Z

src/java.base/share/classes/java/lang/Module.java

+            } else {
+                if (trySetEnableNativeAccess(target)) {
                    // warn and set flag, so that only one warning is reported per module
                    String cls = owner.getName();
                    String mtd = cls + "::" + methodName;
                    String mod = isNamed() ? "module " + getName() : "the unnamed module";
                    String modflag = isNamed() ? getName() : "ALL-UNNAMED";
                    System.err.printf("""
-                        WARNING: A restricted method in %s has been called
-                        WARNING: %s has been called by %s
-                        WARNING: Use --enable-native-access=%s to avoid a warning for this module
-                        %n""", cls, mtd, mod, modflag);
-
-                    // set the flag
-                    target.enableNativeAccess = true;
+                            WARNING: A restricted method in %s has been called
+                            WARNING: %s has been called by %s
+                            WARNING: Use --enable-native-access=%s to avoid a warning for this module
+                            %n""", cls, mtd, mod, modflag);
                }
            }


The else and if blocks can be merged:

Suggested change

} else {

if (trySetEnableNativeAccess(target)) {

// warn and set flag, so that only one warning is reported per module

String cls = owner.getName();

String mtd = cls + "::" + methodName;

String mod = isNamed() ? "module " + getName() : "the unnamed module";

String modflag = isNamed() ? getName() : "ALL-UNNAMED";

System.err.printf("""

WARNING: A restricted method in %s has been called

WARNING: %s has been called by %s

WARNING: Use --enable-native-access=%s to avoid a warning for this module

%n""", cls, mtd, mod, modflag);

// set the flag

target.enableNativeAccess = true;

WARNING: A restricted method in %s has been called

WARNING: %s has been called by %s

WARNING: Use --enable-native-access=%s to avoid a warning for this module

%n""", cls, mtd, mod, modflag);

}

}

} else if (trySetEnableNativeAccess(target)) {

// warn and set flag, so that only one warning is reported per module

String cls = owner.getName();

String mtd = cls + "::" + methodName;

String mod = isNamed() ? "module " + getName() : "the unnamed module";

String modflag = isNamed() ? getName() : "ALL-UNNAMED";

System.err.printf("""

WARNING: A restricted method in %s has been called

WARNING: %s has been called by %s

WARNING: Use --enable-native-access=%s to avoid a warning for this module

%n""", cls, mtd, mod, modflag);

}

ExE-Boss · 2023-02-05T18:44:56Z

src/java.base/share/classes/java/lang/Module.java

+    }
+
+    private static boolean isNativeAccessEnabled(Module target) {
+        if (target.enableNativeAccess)


Shouldn’t Unsafe already be loaded at this point?

minborg · 2023-02-09T15:03:17Z

By putting Unsafe in an inner class, we are able to avoid circular initializer dependencies and I have been able to rewrite enableNativeAccess access so it is completely lock-free.

AlanBateman · 2023-02-09T15:19:38Z

src/java.base/share/classes/java/lang/Module.java

+    // Accessing this variable is made through Unsafe in order to use the
+    // memory semantics that preserves ordering and visibility across threads.
+    //
+    // Used reflectively via Unsafe


I assume L119-120 can be removed.

The reson for the L119-120 comments is to make it easier to understand why we are declaring a variable that does not appear to be used. But maybe there is a better way to do that?

The comment at L117-118 already explains that it's accessed with Unsafe so I don't think we need any more than that. The IDE may suggest the field is unused but if anyone removes it then it will break the JDK build/startup. Same thing many other areas where Unsafe or VarHandles are used. So I don't think we should be too concerned about and the comments at L116-118 are more than enough.

AlanBateman · 2023-02-09T15:20:11Z

src/java.base/share/classes/java/lang/Module.java

@@ -124,8 +129,7 @@ public final class Module implements AnnotatedElement {
    Module(ModuleLayer layer,
           ClassLoader loader,
           ModuleDescriptor descriptor,
-           URI uri)
-    {
+           URI uri) {


This seems a spurious edit, not sure if you meant to change it.

AlanBateman · 2023-02-09T15:21:28Z

src/java.base/share/classes/java/lang/Module.java

-     *
-     * @throws SecurityException
-     *         If denied by the security manager
+     * @throws SecurityException If denied by the security manager


The exception messages are aligned under the exceptions in this class so I assume you didn't mean to change this.

AlanBateman · 2023-02-09T15:25:06Z

src/java.base/share/classes/java/lang/Module.java

@@ -229,7 +230,7 @@ public ModuleDescriptor getDescriptor() {
    /**
     * Returns the module layer that contains this module or {@code null} if
     * this module is not in a module layer.
-     *
+     * <p>


This will break up the first paragraph, I don't think this PR should be changing that.

AlanBateman · 2023-02-09T15:25:47Z

src/java.base/share/classes/java/lang/Module.java

+        private AccessHolder() {}
+
+        private static final Unsafe UNSAFE = Unsafe.getUnsafe();
+        private static final long FIELD_OFFSET = UNSAFE.objectFieldOffset(Module.class, "enableNativeAccess");


Using Unsafe and CAS'ing the enableNativeAccess field looks okay. The name "AccessHolder" looks too general here as there is a lot of access going on in this class, maybe NativeAccessHolder will work.

It would be good if you could try to keep changes to this code consistent with the existing style if possible. In this case, the other inner class uses /** .. */ style comment for the class description. I think would I put the constants at the top of the this class.

AlanBateman

I think this is a good cleanup overall.

AlanBateman · 2023-02-10T08:50:27Z

src/java.base/share/classes/java/lang/Module.java

+    // Accessing this variable is made through Unsafe in order to use the
+    // memory semantics that preserves ordering and visibility across threads.
+    //
+    // Used reflectively via Unsafe


The comment at L117-118 already explains that it's accessed with Unsafe so I don't think we need any more than that. The IDE may suggest the field is unused but if anyone removes it then it will break the JDK build/startup. Same thing many other areas where Unsafe or VarHandles are used. So I don't think we should be too concerned about and the comments at L116-118 are more than enough.

AlanBateman · 2023-02-10T08:52:47Z

src/java.base/share/classes/java/lang/Module.java

+     * This class is used to be able to bootstrap without using Unsafe
+     * in the outer Module class as that would create a circular initializer dependency.
+     */
+    private static final class EnableNativeAccessHandler {


I see this has been renamed EnableNativeAccessHandler, maybe drop "Handler" as it just sets the enableNativeAccess bit, nothing more.

openjdk · 2023-02-10T09:29:26Z

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

8301578: Perform output outside synchronization in Module.class

Reviewed-by: alanb

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

1c7b09b: 8302069: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java update
837d464: 8302125: Make G1 full gc abort the VM after failing VerifyDuringGC verification
723433d: 8302117: IgnoreUnrecognizedVMOptions flag causes failure in ArchiveHeapTestClass
e245620: 8293198: [vectorapi] Improve the implementation of VectorMask.indexInRange()
b814cfc: 8178806: Better exception logging in crypto code
8c87a67: 8245654: Add Certigna Root CA
97d0c87: 8302109: Trivial fixes to btree tests
0aeebee: 8301988: VerifyLiveClosure::verify_liveness asserts on bad pointers outside heap
4815566: 8228604: StackMapFrames are missing from redefined class bytes of retransformed classes
5147969: 8272288: Funky multiresolution image breaks graphics context
... and 260 more: https://git.openjdk.org/jdk/compare/a5d8e12872d9de399fa97b33896635d101b71372...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.

minborg · 2023-02-10T13:45:11Z

/integrate

openjdk · 2023-02-10T13:46:55Z

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

5830c03: 8302004: InlineTree should consult replay file in release build
c8ace48: 8301072: Replace NULL with nullptr in share/oops/
1c7b09b: 8302069: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java update
837d464: 8302125: Make G1 full gc abort the VM after failing VerifyDuringGC verification
723433d: 8302117: IgnoreUnrecognizedVMOptions flag causes failure in ArchiveHeapTestClass
e245620: 8293198: [vectorapi] Improve the implementation of VectorMask.indexInRange()
b814cfc: 8178806: Better exception logging in crypto code
8c87a67: 8245654: Add Certigna Root CA
97d0c87: 8302109: Trivial fixes to btree tests
0aeebee: 8301988: VerifyLiveClosure::verify_liveness asserts on bad pointers outside heap
... and 262 more: https://git.openjdk.org/jdk/compare/a5d8e12872d9de399fa97b33896635d101b71372...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-02-10T13:47:10Z

@minborg Pushed as commit c25b4f4.

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

Reduce contention

cdd3ac4

openjdk bot added the core-libs core-libs-dev@openjdk.org label Jan 25, 2023

Remove warning

47d902b

minborg marked this pull request as ready for review January 31, 2023 17:30

openjdk bot added the rfr Pull request is ready for review label Jan 31, 2023

AlanBateman reviewed Jan 31, 2023

View reviewed changes

viktorklang-ora reviewed Feb 1, 2023

View reviewed changes

minborg changed the title ~~8300917: Regression 2x and bimodal startup on Mac aarch64 in b27~~ 8301578: Perform output outside synchronization in Module.class Feb 1, 2023

minborg added 4 commits February 1, 2023 09:41

Update after comments

9af658b

Use another type of comment

21d1122

Fix typo

b9e42f7

Revert benchmark change

b0aeb64

ExE-Boss reviewed Feb 5, 2023

View reviewed changes

Use Unsafe instead of synchronized

11db7d1

AlanBateman reviewed Feb 9, 2023

View reviewed changes

minborg added 3 commits February 9, 2023 17:30

Undo unintended changes

5b78467

Fix things from PR comments

344d47e

Undo more unintended edits

dfa2422

AlanBateman reviewed Feb 10, 2023

View reviewed changes

Remove comment and rename class

29afac8

AlanBateman approved these changes Feb 10, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 10, 2023

openjdk bot added the integrated Pull request has been integrated label Feb 10, 2023

openjdk bot closed this Feb 10, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8301578: Perform output outside synchronization in Module.class #12193

8301578: Perform output outside synchronization in Module.class #12193

minborg commented Jan 25, 2023 •

edited by openjdk bot

bridgekeeper bot commented Jan 25, 2023

openjdk bot commented Jan 25, 2023

minborg commented Jan 31, 2023 •

edited

mlbridge bot commented Jan 31, 2023 •

edited

AlanBateman Jan 31, 2023

AlanBateman commented Jan 31, 2023 •

edited

dholmes-ora commented Feb 1, 2023

AlanBateman commented Feb 1, 2023

viktorklang-ora Feb 1, 2023

minborg Feb 1, 2023

ExE-Boss Feb 5, 2023

minborg commented Feb 1, 2023 •

edited

ExE-Boss Feb 5, 2023

ExE-Boss Feb 5, 2023

minborg commented Feb 9, 2023

AlanBateman Feb 9, 2023

minborg Feb 9, 2023

AlanBateman Feb 10, 2023

AlanBateman Feb 9, 2023

AlanBateman Feb 9, 2023

AlanBateman Feb 9, 2023

AlanBateman Feb 9, 2023

AlanBateman left a comment

AlanBateman Feb 10, 2023

AlanBateman Feb 10, 2023

openjdk bot commented Feb 10, 2023

minborg commented Feb 10, 2023

openjdk bot commented Feb 10, 2023

openjdk bot commented Feb 10, 2023

8301578: Perform output outside synchronization in Module.class #12193

8301578: Perform output outside synchronization in Module.class #12193

Conversation

minborg commented Jan 25, 2023 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 25, 2023

openjdk bot commented Jan 25, 2023

minborg commented Jan 31, 2023 • edited

mlbridge bot commented Jan 31, 2023 • edited

Webrevs

Choose a reason for hiding this comment

AlanBateman commented Jan 31, 2023 • edited

dholmes-ora commented Feb 1, 2023

AlanBateman commented Feb 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minborg commented Feb 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minborg commented Feb 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanBateman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Feb 10, 2023

minborg commented Feb 10, 2023

openjdk bot commented Feb 10, 2023

openjdk bot commented Feb 10, 2023

minborg commented Jan 25, 2023 •

edited by openjdk bot

minborg commented Jan 31, 2023 •

edited

mlbridge bot commented Jan 31, 2023 •

edited

AlanBateman commented Jan 31, 2023 •

edited

minborg commented Feb 1, 2023 •

edited