8309390: [JVMCI] improve copying system properties into libgraal #14291

dougxc · 2023-06-02T20:32:14Z

This PR reduces the amount of code executed during libgraal initialization. This not only improves VM startup overall but all fixes a number of JDK test failures that are caused by Java code executing "too early". For example, java/util/Locale/CompatWarning.java can fail if sun.util.locale.provider.LocaleProviderAdapter is initialized before the CheckWarning handler is registered.

Instead of serializing VM.savedProps via VMSupport.serializeSavedPropertiesToByteArray, the jdk.vm.ci.services.Services class now directly reads Arguments::system_properties() using Unsafe. Furthermore, the value of a system property is lazily converted to a String from a C string pointer.

Times

The basic benchmarking below shows that this change brings the time for a nop Java app with eager libgraal initialization (2) down to almost the same time as lazy libgraal initialization (1). The latter typically means no libgraal initialization happens as a top tier JIT compilation is never scheduled in such a short running app.

public class Nop {
    public static void main(String[] args) {}
}

(1) Baseline (no options):

> for i in (seq 10); java Nop; end
        0.05 real         0.04 user         0.01 sys
        0.04 real         0.03 user         0.01 sys
        0.04 real         0.03 user         0.01 sys
        0.04 real         0.03 user         0.01 sys
        0.03 real         0.03 user         0.00 sys
        0.04 real         0.03 user         0.01 sys
        0.04 real         0.03 user         0.00 sys
        0.03 real         0.03 user         0.00 sys
        0.04 real         0.03 user         0.01 sys
        0.03 real         0.03 user         0.00 sys

(2) Eagerly initialize libgraal (with PR):

> for i in (seq 10); /usr/bin/time java -XX:+EagerJVMCI Nop; end
        0.06 real         0.04 user         0.01 sys
        0.05 real         0.03 user         0.01 sys
        0.05 real         0.03 user         0.01 sys
        0.05 real         0.03 user         0.01 sys
        0.05 real         0.03 user         0.01 sys
        0.05 real         0.03 user         0.01 sys
        0.05 real         0.03 user         0.01 sys
        0.05 real         0.03 user         0.01 sys
        0.05 real         0.03 user         0.01 sys
        0.05 real         0.03 user         0.01 sys

(3) Eagerly initialize libgraal (without PR):

> for i in (seq 10); /usr/bin/time java -XX:+EagerJVMCI Nop; end
        0.11 real         0.08 user         0.02 sys
        0.08 real         0.06 user         0.01 sys
        0.08 real         0.07 user         0.01 sys
        0.10 real         0.07 user         0.01 sys
        0.08 real         0.06 user         0.01 sys
        0.10 real         0.07 user         0.01 sys
        0.08 real         0.07 user         0.01 sys
        0.08 real         0.07 user         0.01 sys
        0.08 real         0.06 user         0.01 sys
        0.08 real         0.06 user         0.01 sys

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-8309390: [JVMCI] improve copying system properties into libgraal (Enhancement - P4)

Reviewers

Tom Rodriguez (@tkrodriguez - Reviewer) ⚠️ Review applies to 6c346d35
Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to 6c346d35

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14291

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-06-02T20:33:26Z

👋 Welcome back dnsimon! 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-06-02T20:35:30Z

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

core-libs
graal
hotspot

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.

dougxc · 2023-06-03T11:36:18Z

src/java.base/share/classes/jdk/internal/util/SystemProps.java

@@ -59,7 +59,7 @@ public static boolean isBadIoTmpdir() {
     * Note:  Build-defined properties such as versions and vendor information
     * are initialized by VersionProps.java-template.
     *
-     * @return a Properties instance initialized with all of the properties


I took the liberty of correcting this javadoc to reflect the code.

mlbridge · 2023-06-03T11:40:32Z

Webrevs

tkrodriguez · 2023-06-05T18:58:36Z

I don't really love the hard code parsing of the HashMap. What properties are actually required for JVMCI? It seems to me that the contents of Arguments::system_properties() should contain all the properties we want to advertise to JVMCI. That would have avoid having to decode them after they've been converted into Java objects.

dougxc · 2023-06-06T23:02:08Z

I don't really love the hard code parsing of the HashMap. What properties are actually required for JVMCI? It seems to me that the contents of Arguments::system_properties() should contain all the properties we want to advertise to JVMCI. That would have avoid having to decode them after they've been converted into Java objects.

I tired this but unfortunately, Graal relies on some properties that are only initialized in Java:

Caused by: java.lang.NullPointerException: Cannot invoke "String.compareTo(String)" because "this.javaSpecVersion" is null
	at jdk.internal.vm.compiler/org.graalvm.compiler.hotspot.JVMCIVersionCheck.run(JVMCIVersionCheck.java:181)
	at jdk.internal.vm.compiler/org.graalvm.compiler.hotspot.JVMCIVersionCheck.check(JVMCIVersionCheck.java:166)
	at jdk.internal.vm.compiler/org.graalvm.compiler.hotspot.HotSpotGraalCompilerFactory.initialize(HotSpotGraalCompilerFactory.java:117)
	at jdk.internal.vm.compiler/org.graalvm.compiler.hotspot.HotSpotGraalCompilerFactory.ensureInitialized(HotSpotGraalCompilerFactory.java:90)
	at jdk.internal.vm.compiler/org.graalvm.compiler.hotspot.HotSpotGraalCompilerFactory.createCompiler(HotSpotGraalCompilerFactory.java:180)
	at jdk.internal.vm.compiler/org.graalvm.compiler.hotspot.HotSpotGraalCompilerFactory.createCompiler(HotSpotGraalCompilerFactory.java:53)
	at jdk.internal.vm.ci/jdk.vm.ci.hotspot.HotSpotJVMCIRuntime.getCompiler(HotSpotJVMCIRuntime.java:810)
	at org.graalvm.nativeimage.pointsto/com.oracle.graal.pointsto.util.GraalAccess.<clinit>(GraalAccess.java:50)

That code is reading the "java.specification.version" property which is initialized in java.lang.VersionProps#init.

tkrodriguez · 2023-06-07T00:13:02Z

HotSpot sets java.vm.specification.version which has the same value so we could change Graal to read that instead.

openjdk · 2023-06-07T22:48:14Z

@dougxc Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

dougxc · 2023-06-07T22:50:12Z

Ok, I've pushed a change with your suggestion Tom and it seems to work. It assumes Arguments::system_properties() is fully initialized and effectively read-only by the time JVMCIEnv::init_saved_properties is called.

dougxc · 2023-06-07T22:51:47Z

src/hotspot/share/jvmci/jvmciJavaClasses.hpp

@@ -57,7 +57,7 @@
                         jvmci_method, \
                         jvmci_constructor) \
  start_class(Services, jdk_vm_ci_services_Services)                                                          \
-    jvmci_method(CallStaticVoidMethod, GetStaticMethodID, call_static, void, Services, initializeSavedProperties, byte_array_void_signature, (JVMCIObject serializedProperties)) \


The final arg for the jvmci_method macro is never used so I removed it everywhere.

openjdk · 2023-06-11T16:48:47Z

@dougxc Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

tkrodriguez · 2023-06-12T03:00:23Z

src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/services/Services.java

+        for (long prop = systemProperties; prop != 0; prop = unsafe.getLong(prop + nextOffset)) {
+            String key = toJavaString(unsafe, unsafe.getLong(prop + keyOffset));
+            long valueAddress = unsafe.getLong(prop + valueOffset);
+            if (valueAddress != 0) {


Is the lazy value construction really necessary? It seems like it requires quite a bit of extra machinery.

In big apps, I image there can be quite some large system property values (e.g. java.class.path) so we don't want to pay memory overhead per libgraal isolate for such properties as they're ignored by libgraal.

I realized I can reduce the extra machinery based on the fact that SystemProperties is unmodifiable: 6c346d3
The only method that still eager converts the string is SystemProperties.values() but that's fine as it's not used by JVMCI or Graal.
I think the trade off is now acceptable?

Yes I like this trade off a bit better.

openjdk · 2023-06-12T15:34:29Z

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

8309390: [JVMCI] improve copying system properties into libgraal

Reviewed-by: never, 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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

vnkozlov

VM changes seems fine. They are JVMCI specific.

…8309390

dougxc · 2023-06-13T14:44:53Z

Thanks for the reviews!

/integrate

openjdk · 2023-06-13T14:45:39Z

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

63843b1: 8309907: Remove unused _print_gc_overhead_limit_would_be_exceeded
6d05360: 8304403: Remove unused methods in RangeCheckElimination::Bound
9b0baa1: 8306281: function isWsl() returns false on WSL2
c884862: 8309468: Remove jvmti Allocate locker test case
05f896a: 8309862: Unsafe list operations in JfrStringPool
4f23fc1: 8309671: Avoid using jvmci.Compiler property to determine if Graal is enabled
1a9edb8: 8309838: Classfile API Util.toBinaryName and other cleanup
f7de726: 8295555: Primitive wrapper caches could be @Stable
5d71612: 8309852: G1: Remove unnecessary assert_empty in G1ParScanThreadStateSet destructor
23a54f3: 8309538: G1: Move total collection increment from Cleanup to Remark
... and 5 more: https://git.openjdk.org/jdk/compare/3ce1240ca1b4139980444c171e317f4bfeff9314...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-06-13T14:45:47Z

@dougxc Pushed as commit c0aa6bf.

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

openjdk bot added graal graal-dev@openjdk.org hotspot hotspot-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Jun 2, 2023

dougxc force-pushed the JDK-8309390 branch 2 times, most recently from cf1be09 to d636421 Compare June 3, 2023 11:25

dougxc marked this pull request as ready for review June 3, 2023 11:35

openjdk bot added the rfr Pull request is ready for review label Jun 3, 2023

dougxc commented Jun 3, 2023

View reviewed changes

dougxc force-pushed the JDK-8309390 branch from d636421 to c3c0474 Compare June 7, 2023 22:46

dougxc commented Jun 7, 2023

View reviewed changes

dougxc closed this Jun 10, 2023

copy system properties into libgraal more efficiently

7a82c24

dougxc reopened this Jun 11, 2023

dougxc force-pushed the JDK-8309390 branch from c3c0474 to 7a82c24 Compare June 11, 2023 16:38

tkrodriguez reviewed Jun 12, 2023

View reviewed changes

reduced complexity of SystemProperties based on it being unmodifiable

6c346d3

tkrodriguez approved these changes Jun 12, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 12, 2023

vnkozlov approved these changes Jun 12, 2023

View reviewed changes

fix reference in comments

59e3dbf

dougxc added 3 commits June 12, 2023 20:45

fix copyright dates

cf8f03e

[skip-ci] remove debug code

0918653

[skip-ci] Merge remote-tracking branch 'openjdk-jdk/master' into JDK-…

15a07e9

…8309390

openjdk bot added the integrated Pull request has been integrated label Jun 13, 2023

openjdk bot closed this Jun 13, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8309390: [JVMCI] improve copying system properties into libgraal #14291

8309390: [JVMCI] improve copying system properties into libgraal #14291

dougxc commented Jun 2, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jun 2, 2023

openjdk bot commented Jun 2, 2023

dougxc Jun 3, 2023

mlbridge bot commented Jun 3, 2023 •

edited

Loading

tkrodriguez commented Jun 5, 2023

dougxc commented Jun 6, 2023

tkrodriguez commented Jun 7, 2023

openjdk bot commented Jun 7, 2023

dougxc commented Jun 7, 2023

dougxc Jun 7, 2023

openjdk bot commented Jun 11, 2023

tkrodriguez Jun 12, 2023

dougxc Jun 12, 2023

tkrodriguez Jun 12, 2023

openjdk bot commented Jun 12, 2023 •

edited

Loading

vnkozlov left a comment

dougxc commented Jun 13, 2023

openjdk bot commented Jun 13, 2023

openjdk bot commented Jun 13, 2023

8309390: [JVMCI] improve copying system properties into libgraal #14291

8309390: [JVMCI] improve copying system properties into libgraal #14291

Conversation

dougxc commented Jun 2, 2023 • edited by openjdk bot Loading

Times

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jun 2, 2023

openjdk bot commented Jun 2, 2023

dougxc Jun 3, 2023

Choose a reason for hiding this comment

mlbridge bot commented Jun 3, 2023 • edited Loading

Webrevs

tkrodriguez commented Jun 5, 2023

dougxc commented Jun 6, 2023

tkrodriguez commented Jun 7, 2023

openjdk bot commented Jun 7, 2023

dougxc commented Jun 7, 2023

dougxc Jun 7, 2023

Choose a reason for hiding this comment

openjdk bot commented Jun 11, 2023

tkrodriguez Jun 12, 2023

Choose a reason for hiding this comment

dougxc Jun 12, 2023

Choose a reason for hiding this comment

tkrodriguez Jun 12, 2023

Choose a reason for hiding this comment

openjdk bot commented Jun 12, 2023 • edited Loading

vnkozlov left a comment

Choose a reason for hiding this comment

dougxc commented Jun 13, 2023

openjdk bot commented Jun 13, 2023

openjdk bot commented Jun 13, 2023

dougxc commented Jun 2, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 3, 2023 •

edited

Loading

openjdk bot commented Jun 12, 2023 •

edited

Loading