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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 11 commits into from
Closed

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

Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
cdd3ac4
Reduce contention
minborg Jan 25, 2023
47d902b
Remove warning
minborg Jan 31, 2023
9af658b
Update after comments
minborg Feb 1, 2023
21d1122
Use another type of comment
minborg Feb 1, 2023
b9e42f7
Fix typo
minborg Feb 1, 2023
b0aeb64
Revert benchmark change
minborg Feb 1, 2023
11db7d1
Use Unsafe instead of synchronized
minborg Feb 9, 2023
5b78467
Undo unintended changes
minborg Feb 9, 2023
344d47e
Fix things from PR comments
minborg Feb 9, 2023
dfa2422
Undo more unintended edits
minborg Feb 9, 2023
29afac8
Remove comment and rename class
minborg Feb 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
85 changes: 45 additions & 40 deletions src/java.base/share/classes/java/lang/Module.java
View file
Open in desktop
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2014, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2014, 2023, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -57,6 +57,7 @@
import jdk.internal.loader.BootLoader;
import jdk.internal.loader.ClassLoaders;
import jdk.internal.misc.CDS;
import jdk.internal.misc.Unsafe;
import jdk.internal.module.ModuleBootstrap;
import jdk.internal.module.ModuleLoaderMap;
import jdk.internal.module.ServicesCatalog;
Expand Down Expand Up @@ -113,6 +114,10 @@ public final class Module implements AnnotatedElement {
private final ModuleDescriptor descriptor;

// true, if this module allows restricted native access
// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume L119-120 can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@Stable
private boolean enableNativeAccess;

Expand Down Expand Up @@ -176,9 +181,8 @@ public final class Module implements AnnotatedElement {
* Returns {@code true} if this module is a named module.
*
* @return {@code true} if this is a named module
*
* @see ClassLoader#getUnnamedModule()
* @jls 7.7.5 Unnamed Modules
* @see ClassLoader#getUnnamedModule()
*/
public boolean isNamed() {
return name != null;
Expand All @@ -203,7 +207,6 @@ public String getName() {
* class loader. </p>
*
* @return The class loader for this module
*
* @throws SecurityException
* If denied by the security manager
*/
Expand Down Expand Up @@ -238,7 +241,6 @@ public ModuleDescriptor getDescriptor() {
* not be in a module layer. </p>
*
* @return The module layer that contains this module
*
* @see java.lang.reflect.Proxy
*/
public ModuleLayer getLayer() {
Expand All @@ -258,24 +260,43 @@ public ModuleLayer getLayer() {
/**
* Update this module to allow access to restricted methods.
*/
synchronized Module implAddEnableNativeAccess() {
enableNativeAccess = true;
Module implAddEnableNativeAccess() {
EnableNativeAccessHandler.trySetEnableNativeAccess(this);
return this;
}

/**
* Returns {@code true} if this module can access
* <a href="foreign/package-summary.html#restricted"><em>restricted</em></a> methods.
*
* @since 20
*
* @return {@code true} if this module can access <em>restricted</em> methods.
* @since 20
*/
@PreviewFeature(feature=PreviewFeature.Feature.FOREIGN)
@PreviewFeature(feature = PreviewFeature.Feature.FOREIGN)
public boolean isNativeAccessEnabled() {
Module target = moduleForNativeAccess();
synchronized(target) {
return target.enableNativeAccess;
return EnableNativeAccessHandler.isNativeAccessEnabled(target);
}

/**
* 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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


private EnableNativeAccessHandler() {}

private static final Unsafe UNSAFE = Unsafe.getUnsafe();
private static final long FIELD_OFFSET = UNSAFE.objectFieldOffset(Module.class, "enableNativeAccess");
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


private static boolean isNativeAccessEnabled(Module target) {
return UNSAFE.getBooleanVolatile(target, FIELD_OFFSET);
}

// Atomically sets enableNativeAccess if not already set
// returning if the value was updated
private static boolean trySetEnableNativeAccess(Module target) {
return UNSAFE.compareAndSetBoolean(target, FIELD_OFFSET, false, true);
}
}

Expand All @@ -289,46 +310,30 @@ private Module moduleForNativeAccess() {
void ensureNativeAccess(Class<?> owner, String methodName) {
// The target module whose enableNativeAccess flag is ensured
Module target = moduleForNativeAccess();
// racy read of the enable native access flag
boolean isNativeAccessEnabled = target.enableNativeAccess;
if (!isNativeAccessEnabled) {
synchronized (target) {
// safe read of the enableNativeAccess of the target module
isNativeAccessEnabled = target.enableNativeAccess;

// check again with the safely read flag
if (isNativeAccessEnabled) {
// another thread beat us to it - nothing to do
return;
} else if (ModuleBootstrap.hasEnableNativeAccessFlag()) {
throw new IllegalCallerException("Illegal native access from: " + this);
} else {
// 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("""
if (!EnableNativeAccessHandler.isNativeAccessEnabled(target)) {
if (ModuleBootstrap.hasEnableNativeAccessFlag()) {
throw new IllegalCallerException("Illegal native access from: " + this);
}
if (EnableNativeAccessHandler.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;
}
}
}
}


/**
* Update all unnamed modules to allow access to restricted methods.
*/
static void implAddEnableNativeAccessToAllUnnamed() {
synchronized (ALL_UNNAMED_MODULE) {
ALL_UNNAMED_MODULE.enableNativeAccess = true;
}
EnableNativeAccessHandler.trySetEnableNativeAccess(ALL_UNNAMED_MODULE);
}

// --
Expand Down