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.

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

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

Closed
wants to merge 72 commits into from

Conversation

bchristi-git
Copy link
Member

@bchristi-git bchristi-git commented Nov 13, 2023

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

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Change requires CSR request JDK-8327707 to be approved
  • 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

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

@mlbridge
Copy link

mlbridge bot commented May 11, 2024

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>

Copy link
Contributor

@AlanBateman AlanBateman left a comment

Choose a reason for hiding this comment

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

You've addressed my comments, I don't have anything else.

@mlbridge
Copy link

mlbridge bot commented May 13, 2024

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

@mlbridge
Copy link

mlbridge bot commented May 14, 2024

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>

@viktorklang-ora
Copy link
Contributor

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

@bchristi-git
Copy link
Member Author

@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. 😉

@AlanBateman
Copy link
Contributor

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.

@openjdk openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels May 30, 2024
@viktorklang-ora
Copy link
Contributor

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

@bchristi-git
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented May 31, 2024

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

  • 9fd0e73: 8332110: [macos] jpackage tries to sign added files without the --mac-sign option
  • 8aeada1: 8331159: VM build without C2 fails after JDK-8180450
  • e99f6a6: 8333236: Test java/foreign/TestAccessModes.java is timing out after passing
  • e650bdf: 8332507: compilation result depends on compilation order
  • e4fbb15: 8320215: HeapDumper can use DumpWriter buffer during merge
  • 681137c: 8333131: Source launcher should work with service loader SPI
  • 914423e: 8332899: RISC-V: add comment and make the code more readable (if possible) in MacroAssembler::movptr
  • 5abc029: 8331877: JFR: Remove JIInliner framework
  • d9e7b7e: 8210471: GZIPInputStream constructor could leak an un-end()ed Inflater
  • 1e04ee6: 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.

@openjdk openjdk bot added the integrated Pull request has been integrated label May 31, 2024
@openjdk openjdk bot closed this May 31, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 31, 2024
@openjdk
Copy link

openjdk bot commented May 31, 2024

@bchristi-git Pushed as commit 2cae9a0.

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

* correctness.
* triggering garbage collection. This method is applicable only
* when reclamation may have visible effects,
* such as for objects that use finalizers or {@link Cleaner}, or code that
Copy link

@lukellmann lukellmann Oct 10, 2024

Choose a reason for hiding this comment

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

This @link tag does not produce a link in the Javadoc because this file imports jdk.internal.ref.Cleaner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org hotspot-gc hotspot-gc-dev@openjdk.org integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.