8189685: need PerfMemory class update and a volatile_static_field support in VMStructs #15373

plummercj · 2023-08-21T21:47:14Z

During JDK-8151815 it was noted that the PerfMemory _initialized and _destroyed fields should be volatile, but VMStructs didn't have the needed support for doing that, so it was left as a future task. @YaSuenag provided a patch at the time to take care of the VMStructs support. I've integrated it, although it was far from clean due to some changes in VMStructs, and also moving OrderAccess::release_store to Atomic::release_store.

One other change I made to the patch had to do with consistency with using "volatile static" vs "static volatile". We already have volatile_nonstatic_field. The patch renamed static_ptr_volatile_field to static_volatile_field to make it more general purpose, but this was inconsistent with the name of volatile_nonstatic_field, so I chose the name volatile_static_field instead. This carried over into some other areas like the names of the GENERATE_VOLATILE_STATIC_VM_STRUCT_ENTRY and CHECK_VOLATILE_STATIC_VM_STRUCT_ENTRY macros.

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-8189685: need PerfMemory class update and a volatile_static_field support in VMStructs (Bug - P4)

Reviewers

Yasumasa Suenaga (@YaSuenag - Reviewer)
David Holmes (@dholmes-ora - Reviewer)
Serguei Spitsyn (@sspitsyn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15373

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

Using diff file

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

Webrev

Link to Webrev Comment

…port for volatile static.

bridgekeeper · 2023-08-21T21:48:06Z

👋 Welcome back cjplummer! 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-08-21T21:49:16Z

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

hotspot

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.

mlbridge · 2023-08-21T21:53:45Z

Webrevs

plummercj · 2023-08-21T21:54:19Z

/label remove hotspot
/label add serviceability
/label add runtime

openjdk · 2023-08-21T21:55:00Z

@plummercj
The hotspot label was successfully removed.

openjdk · 2023-08-21T21:55:36Z

@plummercj
The serviceability label was successfully added.

openjdk · 2023-08-21T21:56:15Z

@plummercj
The label runtime is not a valid label.
These labels are valid:

graal
serviceability
hotspot
hotspot-compiler
ide-support
kulla
i18n
shenandoah
jdk
javadoc
security
hotspot-runtime
jmx
build
nio
client
core-libs
compiler
net
hotspot-gc
hotspot-jfr

plummercj · 2023-08-21T22:18:33Z

/label add hotspot-runtime

openjdk · 2023-08-21T22:19:00Z

@plummercj
The hotspot-runtime label was successfully added.

YaSuenag

LGTM.
Thank you for heading up for this issue!

openjdk · 2023-08-22T04:30:57Z

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

8189685: need PerfMemory class update and a volatile_static_field support in VMStructs

Reviewed-by: ysuenaga, dholmes, sspitsyn

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

acd9310: 8313430: [JVMCI] fatal error: Never compilable: in JVMCI shutdown
8a5db6b: 8237542: JMapHeapConfigTest.java doesn't work with negative jlong values
d0cc043: 8314550: [macosx-aarch64] serviceability/sa/TestJmapCore.java fails with "sun.jvm.hotspot.debugger.UnmappedAddressException: 801000800"
837d2e1: 8314483: Optionally override copyright header in generated source
3201623: 8310454: Introduce static-libs-graal bundle
e9ba8d5: 8314960: Add Certigna Root CA - 2
837cf85: 8312547: Max/Min nodes Value implementation could be improved
7342f5a: 8314333: Update com/sun/jdi/ProcessAttachTest.java to use ProcessTools.createTestJvm(..)
f139f30: 8315033: Problemlist java/lang/template/StringTemplateTest.java
f2383b3: 8314063: The socket is not closed in Connection::createSocket when the handshake failed for LDAP connection
... and 51 more: https://git.openjdk.org/jdk/compare/78f74bc8ff1e673991d91a55ef70880d9fb3f6b5...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.

dholmes-ora · 2023-08-22T07:07:57Z

src/hotspot/share/runtime/perfMemory.cpp

@@ -197,7 +197,7 @@ void PerfMemory::destroy() {
    delete_memory_region();
  }

-  _destroyed = true;
+  Atomic::release_store(&_destroyed, 1);


As discussed in the final review thread for JDK-8151815 a release_store is not actually needed here as there is no data that we access based on _destroyed being set to 1. Hence also no need for load_acquire in is_destroyed().

Ok. I've updated it.

YaSuenag · 2023-08-23T00:49:18Z

src/hotspot/share/runtime/perfMemory.hpp

@@ -134,7 +134,7 @@ class PerfMemory : AllStatic {
    static size_t used() { return (size_t) (_top - _start); }
    static size_t capacity() { return _capacity; }
    static bool is_initialized();
-    static bool is_destroyed() { return _destroyed; }
+    static bool is_destroyed();


So don't we need to make changes here? I think we can keep current implementation ( is_destroyed() )

Agreed - I assume the change to int was for the release_store etc but that is not needed so we can keep the bool type and existing code.

I was also partly going for consistency with _initialized and _destroyed w.r.t. int vs bool and 0/1 vs false/true. I can go back to making _destroyed all bool.

Update done. I should also point out that _destroyed is not exported from VMStructs, so could have been made volatile long ago without needing to add any VMStructs support. SA only looks at _initialized, but I suppose you could argue that it also should be checking _destroyed before accessing PerfMemory.

SA only looks at _initialized, but I suppose you could argue that it also should be checking _destroyed before accessing PerfMemory.

In JDK-8151815, we do not clear fields in PerfMemory because they are used in SA even if they are freed, so we can ignore _destroyed in SA. We've discussed about this in review thread of JDK-8151815.

dholmes-ora · 2023-08-23T01:24:24Z

src/hotspot/share/runtime/perfMemory.cpp

 PerfDataPrologue*        PerfMemory::_prologue = nullptr;
-bool                     PerfMemory::_destroyed = false;
+volatile int             PerfMemory::_destroyed = 0;


Why was this changed to int?

In my original patch (pasted into JBS), I changed it to int because atomic operation only supports integer type only (maybe). So we can keep bool if we don't need atomic operations here.

sspitsyn · 2023-08-23T06:45:25Z

src/hotspot/share/runtime/vmStructs.hpp

@@ -207,7 +207,7 @@ class VMStructs {

 // This macro checks the type of a static pointer volatile VMStructEntry by comparing pointer types,
 // e.g.: "static ObjectMonitor * volatile g_block_list;"
-#define CHECK_STATIC_PTR_VOLATILE_VM_STRUCT_ENTRY(typeName, fieldName, type)       \
+#define CHECK_VOLATILE_STATIC_VM_STRUCT_ENTRY(typeName, fieldName, type)       \
 {type volatile * dummy = &typeName::fieldName; }


It is not clear why the PTR_ suffix is removed from the name.

The intent was to generalize it but the comments also need changing, and now I'm not clear what this is actually doing.

This change wasn't in the original patch, but the GENERATE_STATIC_PTR_VOLATILE_VM_STRUCT_ENTRY change above it was. I found it when I noticed the use of GENERATE_STATIC_PTR_VOLATILE_VM_STRUCT_ENTRY and thought it should be renamed too. I was also doing "static volatile" -> "volatile stack" at the time so that is also part of this macros rename.

As David points out, the purpose is to make it more general purpose, although for this macro only the name change is needed (`GENERATE_STATIC_PTR_VOLATILE_VM_STRUCT_ENTRY also has a minor implementation change that was needed).

The comments for both of these macros references "static pointer volatile" entries. That should changed to "volatile static". The type no longer needs to be a pointer type.

I'm not certain what this CHECK macro does, but it was pre-existing, and there is also the CHECK_STATIC_VM_STRUCT_ENTRY macro above it which is identical except of the absence of "volatile" in the name and implementation. I think it verifies that the type specified in the vmstructs definition of the field is actually the same as the field itself. The macro will generate a compiler error if a cast was needed. I think this type of check can only be done with static fields. You would need an instance of the class/struct to do a similar check on an instance field. Thus CHECK_NONSTATIC_VM_STRUCT_ENTRY is much more complex and seems to be a runtime check that will assert if it fails.

I can't convince myself that volatile is in the right place in type volatile * dummy. But volatile * dummy means that the pointer is volatile as opposed to the type being pointed to, which would match with the original macro name's use of PTR_VOLATILE.

But in that case the use of the macro doesn't make much sense. Regardless of the field type, vmstructs doesn't deal with volatile pointers to fields. It deals with volatile fields (that can be pointers or scalar). I think if anything the macro has always been wrong and should contain volatile type * dummy. It probably never fails because it is taking a non-volatile type and assigning it to a volatile type, which always works if the types are otherwise the same.

Well of course that change blows up

invalid conversion from 'ClassLoaderData* volatile*' to 'volatile ClassLoaderData**'

There's something I'm not understanding here. I use to understand the volatile ordering syntax and how to read it properly, but that was long long ago.

I take back the following statement:

It probably never fails because it is taking a non-volatile type and assigning it to a volatile type, which always works if the types are otherwise the same.

Since the field type is volatile, the current approach must be correct. Otherwise there would be cast errors if the LHS did not have volatile in the right place.

@dholmes-ora I'm not sure what is needed to move forward with this. Are you OK with the change?

dholmes-ora

I'm not quite certain about the intent with the old naming for the macros, but the new names certainly don't hurt anything. So this seems okay.

Thanks.

sspitsyn

Looks good.
Thanks,
Serguei

YaSuenag

I agree with this change.

plummercj · 2023-08-29T20:53:48Z

Thanks for the reviews Yasumasa, David, and Serguei!

/integrate

openjdk · 2023-08-29T20:55:14Z

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

e22762c: 8314932: G1: Fix -Wconversion warnings for simple cases inside g1 folder
762b652: 8314573: G1: Heap resizing at Remark does not take existing eden regions into account
a4e97aa: 8314753: Remove support for @beaninfo, @todo, @since.unbundled, and @note
5cc64cc: 8309463: IGV: Dynamic graph layout algorithm
1d02507: 8314476: TestJstatdPortAndServer.java failed with "java.rmi.NoSuchObjectException: no such object in table"
93188bd: 8314569: (fs) Improve normalization of UnixPath for input with trailing slashes
8b8182d: 8315087: G1: Use uint for G1 flags indicating percentage
3dc266c: 8315070: RISC-V: Clean up platform dependent inline headers
25f5df2: 8315073: Zero build on macOS fails after JDK-8303852
a03954e: 8309697: [TESTBUG] Remove "@requires vm.flagless" from jtreg vectorization tests
... and 81 more: https://git.openjdk.org/jdk/compare/78f74bc8ff1e673991d91a55ef70880d9fb3f6b5...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-08-29T20:55:21Z

@plummercj Pushed as commit b2728cb.

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

Make needed perfMemory fields volatile and added needed vmstructs sup…

08c1b6a

…port for volatile static.

openjdk bot changed the title ~~8189685~~ 8189685: need PerfMemory class update and a volatile_static_field support in VMStructs Aug 21, 2023

openjdk bot added the rfr Pull request is ready for review label Aug 21, 2023

openjdk bot added the hotspot hotspot-dev@openjdk.org label Aug 21, 2023

openjdk bot removed the hotspot hotspot-dev@openjdk.org label Aug 21, 2023

openjdk bot added the serviceability serviceability-dev@openjdk.org label Aug 21, 2023

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Aug 21, 2023

YaSuenag approved these changes Aug 22, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 22, 2023

dholmes-ora reviewed Aug 22, 2023

View reviewed changes

release_store() and load_acquire() not needed for _destroyed

8f62584

YaSuenag reviewed Aug 23, 2023

View reviewed changes

dholmes-ora reviewed Aug 23, 2023

View reviewed changes

Revert _destroyed back to bool

c0e1798

sspitsyn reviewed Aug 23, 2023

View reviewed changes

minor comment fix

49b41f9

dholmes-ora approved these changes Aug 25, 2023

View reviewed changes

sspitsyn approved these changes Aug 26, 2023

View reviewed changes

YaSuenag approved these changes Aug 26, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Aug 29, 2023

openjdk bot closed this Aug 29, 2023

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

8189685: need PerfMemory class update and a volatile_static_field support in VMStructs #15373

8189685: need PerfMemory class update and a volatile_static_field support in VMStructs #15373

Uh oh!

Conversation

plummercj commented Aug 21, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Aug 21, 2023

Uh oh!

openjdk bot commented Aug 21, 2023

Uh oh!

mlbridge bot commented Aug 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

plummercj commented Aug 21, 2023

Uh oh!

openjdk bot commented Aug 21, 2023

Uh oh!

openjdk bot commented Aug 21, 2023

Uh oh!

openjdk bot commented Aug 21, 2023

Uh oh!

plummercj commented Aug 21, 2023

Uh oh!

openjdk bot commented Aug 21, 2023

Uh oh!

YaSuenag left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

sspitsyn left a comment

Choose a reason for hiding this comment

Uh oh!

YaSuenag left a comment

Choose a reason for hiding this comment

plummercj commented Aug 21, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 21, 2023 •

edited

Loading

openjdk bot commented Aug 22, 2023 •

edited

Loading