jdk 8314480: Memory ordering spec updates in java.lang.ref

Classes in the java.lang.ref package would benefit from an update to bring the spec in line with how the VM already behaves. The changes would focus on happens-before edges at some key points during reference processing.

A couple key things we want to be able to say are:

Reference.reachabilityFence(x) happens-before reference processing occurs for 'x'.
Cleaner.register() happens-before the Cleaner thread runs the registered cleaning action.

This will bring Cleaner in line (or close) with the memory visibility guarantees made for finalizers in JLS 17.4.5: "There is a happens-before edge from the end of a constructor of an object to the start of a finalizer (§12.6) for that object."

Progress

[x] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
[x] Change must not contain extraneous whitespace
[ ] Change requires CSR request JDK-8327707 to be approved
[x] Commit message must refer to an issue

Issues

JDK-8314480: Memory ordering spec updates in java.lang.ref (Bug - P4)
JDK-8327707: Memory ordering spec updates in java.lang.ref (CSR)

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 0f949d3c
Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16644

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

Using diff file

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

Webrev

Link to Webrev Comment

Nov 13 '23 22:11 bchristi-git

:wave: Welcome back bchristi! 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.

Nov 13 '23 22:11 bridgekeeper[bot]

⚠️ @bchristi-git This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

Nov 13 '23 22:11 openjdk[bot]

@bchristi-git 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.

Nov 13 '23 22:11 openjdk[bot]

Webrevs

Nov 13 '23 22:11 mlbridge[bot]

/label add hotspot-gc

Nov 29 '23 18:11 bchristi-git

@bchristi-git The hotspot-gc label was successfully added.

Nov 29 '23 18:11 openjdk[bot]

@bchristi-git This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Jan 10 '24 07:01 bridgekeeper[bot]

@bchristi-git 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:

8314480: Memory ordering spec updates in java.lang.ref

Reviewed-by: dholmes, alanb, darcy

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

8aeada105acd143b38b02123377ef86513eee266: 8331159: VM build without C2 fails after JDK-8180450
e99f6a65a8307e6b31a08a677914dfd20d46687f: 8333236: Test java/foreign/TestAccessModes.java is timing out after passing
e650bdf4654a0459bb2af95f08ba42ca870642d4: 8332507: compilation result depends on compilation order
e4fbb15c6a7b18f1ec66176080404818d3871194: 8320215: HeapDumper can use DumpWriter buffer during merge
681137cad2b1de8a0af1dfea949439bcaf5e7500: 8333131: Source launcher should work with service loader SPI
914423e3b7162ad934fa4edc46ee37e0f401d27b: 8332899: RISC-V: add comment and make the code more readable (if possible) in MacroAssembler::movptr
5abc02927b480a85fadecf8d03850604510276e4: 8331877: JFR: Remove JIInliner framework
d9e7b7e7da98a0170d26301a4bbd61aad0127c6e: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater
1e04ee6d57d5fe84e1d202b16e8d13dc13c002ff: 8331579: Reference to primitive type fails without error or warning
32ee252c455d3ddcb5954698b546ac39a40515e8: 8333169: javac NullPointerException record.type
... and 94 more: https://git.openjdk.org/jdk/compare/7bf1989f59695c3d08b4bd116fb4c022cf9661f4...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.

Feb 22 '24 01:02 openjdk[bot]

@bchristi-git Made a suggestion, but beyond that this looks great to me. Thanks for putting this together!

Feb 22 '24 09:02 viktorklang-ora

/csr

Feb 23 '24 14:02 AlanBateman

@AlanBateman has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@bchristi-git please create a CSR request for issue JDK-8314480 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

Feb 23 '24 14:02 openjdk[bot]

Mailing list message from Hans Boehm on core-libs-dev:

Is it worth keeping the discussion starting with "It is sometimes possible to better encapsulate ..." and the associated example code? I find this example extremely unconvincing. It's very hard to construct a case in which you can safely use the result of getExternalResource(). And I don't want to encourage writing getters for things like this; if you use them from a static method, or somebody decides to make it public, things get really messy really quickly.

This example kind of gives the impression that we have a solution to reachabilityFence() proliferation. I don't think we do. There may be a difference of opinion about whether it's worth fixing, but I don't think we should deny the existence of the problem.

On Thu, Mar 21, 2024 at 7:11?PM David Holmes <dholmes at openjdk.org> wrote:

-------------- next part -------------- An HTML attachment was scrubbed... URL: <https://mail.openjdk.org/pipermail/core-libs-dev/attachments/20240321/c956dca0/attachment-0001.htm>

Mar 22 '24 15:03 mlbridge[bot]

AFAICT, all review feedback on this change has been addressed. I would love to have some reviewers take another look. Thanks!

Apr 17 '24 17:04 bchristi-git

Thanks Brent, I have no objections. Good job!

Apr 20 '24 09:04 viktorklang-ora

Mailing list message from Hans Boehm on core-libs-dev:

I'm not convinced this helps.

The isAlive() spec says:

"A thread is alive if it has been started and has not yet terminated."

Clearly an object is reachable if it can be accessed by a thread that will be started in the future. Is that part of a "potential continuing computation from any live thread"?

I think the JLS wording is a bit sloppy about what "live thread" means here. And that sloppiness is probably better than pointing at a precise definition that I'm not sure really applies. "in any potential continuing computation from any live thread" really seems to mean something like "in any continuing computation in which no finalizers are run"?

Even if the object is later accessed from an existing "live" thread, it should not be considered reachable if that happens only after a finalizer later makes it reachable again. So I don't see why the thread from which the access happens matters at all.

Hans

On Thu, May 9, 2024 at 11:44?AM Brent Christian <bchristi at openjdk.org> wrote:

-------------- next part -------------- An HTML attachment was scrubbed... URL: <https://mail.openjdk.org/pipermail/core-libs-dev/attachments/20240510/59fbaa88/attachment.htm>

May 11 '24 03:05 mlbridge[bot]

Mailing list message from Brent Christian on core-libs-dev:

On 5/10/24 10:54 AM, Hans Boehm wrote:

I'm not convinced this helps.

The isAlive() spec says:

"A thread is alive if it has been started and has not yet terminated."

Clearly an object is reachable if it can be accessed by a thread that will be started in the future. Is that part of a "potential continuing computation from any live thread"?

I would think; one "computation" a live thread can perform is to start another thread, which in turn might access the object.

I think the JLS wording is a bit sloppy about what "live thread" means here. And that sloppiness is probably better than pointing at a precise definition that I'm not sure really applies.

"in any potential continuing computation from any live thread" really seems to mean something like "in any continuing computation in which no finalizers are run"?

Once an object becomes finalizer-reachable, it can only be reached via a running finalizer. (JLS 12.6.1: "A finalizer-reachable object can be reached from some finalizable object through some chain of references, but not from any live thread.")

So maybe finalizer threads should not be considered "live" threads.

Even if the object is later accessed from an existing "live" thread, it should not be considered reachable if that happens only after a finalizer later makes it reachable again.

Agreed - if an object can (*and only can*) be accessed again after a finalizer resurrects it, that doesn't count as "reachable". In fact, at that point, the object must have transitioned from reachable to finalizer-reachable.

If an object gets resurrected by a finalizer, it does become reachable again and can again be accessed by the program. (And of course if the object's own finalizer ran, it won't run again if the object again stops being reachable.)

So I don't see why the thread from which the access happens matters at all.

I think it would only matter for accesses from a finalizer thread.

-Brent

May 13 '24 19:05 mlbridge[bot]

Mailing list message from Hans Boehm on core-libs-dev:

On Mon, May 13, 2024 at 12:16?PM Brent Christian <brent.christian at oracle.com> wrote:

On 5/10/24 10:54 AM, Hans Boehm wrote:

I'm not convinced this helps.

The isAlive() spec says:

"A thread is alive if it has been started and has not yet terminated."

Clearly an object is reachable if it can be accessed by a thread that will be started in the future. Is that part of a "potential continuing computation from any live thread"?

I would think; one "computation" a live thread can perform is to start another thread, which in turn might access the object.

I think the JLS wording is a bit sloppy about what "live thread" means here. And that sloppiness is probably better than pointing at a precise definition that I'm not sure really applies.

"in any potential continuing computation from any live thread" really seems to mean something like "in any continuing computation in which no finalizers are run"?

Once an object becomes finalizer-reachable, it can only be reached via a running finalizer. (JLS 12.6.1: "A finalizer-reachable object can be reached from some finalizable object through some chain of references, but not from any live thread.")

So maybe finalizer threads should not be considered "live" threads.

Even if the object is later accessed from an existing "live" thread, it should not be considered reachable if that happens only after a finalizer later makes it reachable again.

Agreed - if an object can (*and only can*) be accessed again after a finalizer resurrects it, that doesn't count as "reachable". In fact, at that point, the object must have transitioned from reachable to finalizer-reachable.

If an object gets resurrected by a finalizer, it does become reachable again and can again be accessed by the program. (And of course if the object's own finalizer ran, it won't run again if the object again stops being reachable.)

So I don't see why the thread from which the access happens matters at all.

I think it would only matter for accesses from a finalizer thread.

I'm arguing that even that doesn't matter. Let's say B is reachable from A, and A has a finalizer. Whether B is accessed directly by the finalizer from the finalizer thread, or the finalizer invokes some parallel algorithm on A, so that B is accessed by a helper "live thread" shouldn't matter. What does matter is that neither A nor B are reachable before the finalizer runs, because they can only be accessed as the result of a finalizer running.

I now wonder whether "A <em>reachable</em> object is any object that can be accessed in any potential continuing computation before any additional finalizers are started." wouldn't actually be a much better way to state this. (Officially, this wording is presumably nearly obsolete, since I don't think Cleaners or References have this particular issue. OTOH, that would also make it much clearer that this is so, and post finalizers, there is no issue here.)

Hans

-Brent

-------------- next part -------------- An HTML attachment was scrubbed... URL: <https://mail.openjdk.org/pipermail/core-libs-dev/attachments/20240513/874c0d98/attachment.htm>

May 14 '24 04:05 mlbridge[bot]

@bchristi-git Just checking in—we're waiting for CSR-approval here before integrating? 🤔

May 21 '24 11:05 viktorklang-ora

@bchristi-git Just checking in—we're waiting for CSR-approval here before integrating? 🤔

Indeed - can't move forward without a CSR. Also wouldn't mind more reviewer ✔️s. 😉

May 21 '24 16:05 bchristi-git

Indeed - can't move forward without a CSR. Also wouldn't mind more reviewer ✔️s. 😉

I can do that. One other thing to do is to rebase the changes, it looks like this branch is 6 months behind main line.

May 22 '24 07:05 AlanBateman

@AlanBateman @stuart-marks Any final words before @bchristi-git gets to do the honors of integrating? :)

May 31 '24 13:05 viktorklang-ora

/integrate

May 31 '24 21:05 bchristi-git

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

9fd0e7349ebf4a49b5c0c7a16c866b5b8e626b53: 8332110: [macos] jpackage tries to sign added files without the --mac-sign option
8aeada105acd143b38b02123377ef86513eee266: 8331159: VM build without C2 fails after JDK-8180450
e99f6a65a8307e6b31a08a677914dfd20d46687f: 8333236: Test java/foreign/TestAccessModes.java is timing out after passing
e650bdf4654a0459bb2af95f08ba42ca870642d4: 8332507: compilation result depends on compilation order
e4fbb15c6a7b18f1ec66176080404818d3871194: 8320215: HeapDumper can use DumpWriter buffer during merge
681137cad2b1de8a0af1dfea949439bcaf5e7500: 8333131: Source launcher should work with service loader SPI
914423e3b7162ad934fa4edc46ee37e0f401d27b: 8332899: RISC-V: add comment and make the code more readable (if possible) in MacroAssembler::movptr
5abc02927b480a85fadecf8d03850604510276e4: 8331877: JFR: Remove JIInliner framework
d9e7b7e7da98a0170d26301a4bbd61aad0127c6e: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater
1e04ee6d57d5fe84e1d202b16e8d13dc13c002ff: 8331579: Reference to primitive type fails without error or warning
... and 95 more: https://git.openjdk.org/jdk/compare/7bf1989f59695c3d08b4bd116fb4c022cf9661f4...master

Your commit was automatically rebased without conflicts.

May 31 '24 21:05 openjdk[bot]

@bchristi-git Pushed as commit 2cae9a0397f4e46c6faec0a998ecad1c7015564d.

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

May 31 '24 21:05 openjdk[bot]

jdk jdk copied to clipboard

8314480: Memory ordering spec updates in java.lang.ref

Progress

Issues

Reviewers

Reviewing

Webrev

Webrevs

jdk
jdk copied to clipboard