Re: RFR: 8333396: Performance regression of DecimalFormat.format [v4]
On Tue, 4 Jun 2024 09:07:44 GMT, lingjun-cg wrote:
>> ### Performance regression of DecimalFormat.format
>> From the output of perf, we can see the hottest regions contain atomic
>> instructions. But when run with JDK 11, there is no such problem. The
>> reason is the removed biased locking.
>> The DecimalFormat uses StringBuffer everywhere, and StringBuffer itself
>> contains many synchronized methods.
>> So I added support for some new methods that accept StringBuilder which is
>> lock-free.
>>
>> ### Benchmark testcase
>>
>> @BenchmarkMode(Mode.AverageTime)
>> @Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
>> @State(Scope.Thread)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> public class JmhDecimalFormat {
>>
>> private DecimalFormat format;
>>
>> @Setup(Level.Trial)
>> public void setup() {
>> format = new DecimalFormat("#0.0");
>> }
>>
>> @Benchmark
>> public void testNewAndFormat() throws InterruptedException {
>> new DecimalFormat("#0.0").format(9524234.1236457);
>> }
>>
>> @Benchmark
>> public void testNewOnly() throws InterruptedException {
>> new DecimalFormat("#0.0");
>> }
>>
>> @Benchmark
>> public void testFormatOnly() throws InterruptedException {
>> format.format(9524234.1236457);
>> }
>> }
>>
>>
>> ### Test result
>> Current JDK before optimize
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnly avgt 50 642.099 ? 1.253 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 989.307 ? 3.676 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 303.381 ? 5.252 ns/op
>>
>>
>>
>> Current JDK after optimize
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 50 351.499 ? 0.761 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 615.145 ? 2.478 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 209.874 ? 9.951 ns/op
>>
>>
>> ### JDK 11
>>
>> Benchmark Mode CntScore Error Units
>> JmhDecimalFormat.testFormatOnlyavgt 50 364.214 ? 1.191 ns/op
>> JmhDecimalFormat.testNewAndFormat avgt 50 658.699 ? 2.311 ns/op
>> JmhDecimalFormat.testNewOnly avgt 50 248.300 ? 5.158 ns/op
>
> lingjun-cg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 896: Performance regression of DecimalFormat.format
(Same comment as on the [other
PR](https://github.com/openjdk/jdk/pull/19534#issuecomment-2148568212).)
Would it be better if the benchmarks returned the created DecimalFormats ?
Just thinking about dead code elimination:
https://github.com/openjdk/jmh/blob/master/jmh-samples/src/main/java/org/openjdk/jmh/samples/JMHSample_08_DeadCode.java
-
PR Comment: https://git.openjdk.org/jdk/pull/19513#issuecomment-2148572775
Re: RFR: 8333462: Performance regression of new DecimalFormat() when compare to jdk11
On Tue, 4 Jun 2024 02:32:28 GMT, lingjun-cg wrote:
> Run the below benchmark test ,it show the average time of new
> DecimalFormat() increase 18% when compare to jdk 11.
>
> the result with jdk 11:
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testNewOnly avgt 50 248.300 ? 5.158 ns/op
>
>
> the result with current jdk:
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testNewOnly avgt 50 303.381 ? 5.252 ns/op
>
>
>
> @BenchmarkMode(Mode.AverageTime)
> @Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
> @Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
> @State(Scope.Thread)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> public class JmhDecimalFormat {
>
> @Setup(Level.Trial)
> public void setup() {
> }
>
> @Benchmark
> public void testNewOnly() throws InterruptedException {
> new DecimalFormat("#0.0");
> }
> }
>
>
> Compare the flame graph it shows the
> java.text.DecimalFormatSymbols#findNonFormatChar takes a significant time.
> After replacing the lambda implementation with a simple loop , it shows
> nearly the same performance as jdk 11.
>
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testNewOnly avgt 50 209.874 ? 9.951 ns/op
>
>
> [flame-graph-jdk11-jdk21.zip](https://github.com/user-attachments/files/15541764/flame-graph-jdk11-jdk21.zip)
Would it be better if the benchmark returned the created `DecimalFormat` ?
Just thinking about dead code elimination:
https://github.com/openjdk/jmh/blob/master/jmh-samples/src/main/java/org/openjdk/jmh/samples/JMHSample_08_DeadCode.java
-
PR Comment: https://git.openjdk.org/jdk/pull/19534#issuecomment-2148568212
Integrated: 8314480: Memory ordering spec updates in java.lang.ref
On Mon, 13 Nov 2023 22:31:16 GMT, Brent Christian wrote: > 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ This pull request has now been integrated. Changeset: 2cae9a03 Author:Brent Christian URL: https://git.openjdk.org/jdk/commit/2cae9a0397f4e46c6faec0a998ecad1c7015564d Stats: 229 lines in 4 files changed: 132 ins; 19 del; 78 mod 8314480: Memory ordering spec updates in java.lang.ref Reviewed-by: dholmes, alanb, darcy - PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v35]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: another package doc link - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/e02bf98e..cf0dc3b8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=34 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=33-34 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v34]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: simplify some java.lang.ref package doc links - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/4d06..e02bf98e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=33 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=32-33 Stats: 6 lines in 2 files changed: 0 ins; 1 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v32]
On Thu, 30 May 2024 05:14:30 GMT, Joe Darcy wrote: >> Brent Christian has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 69 commits: >> >> - Merge branch 'master' into refDocs2 >> - add link to Thread.isAlive() >> - small review tweaks; shorten MemoryConsistency links >> - small grammar fixes >> - new section for finalizer memviz >> - add memviz bullet for finalization >> - remove quotes from dequeue >> - package spec updates, mostly about reference queues and dequeueing >> - move reachability section before notification; update section header >> - add details on use of reference queues; swap reachability/memviz sections >> - ... and 59 more: https://git.openjdk.org/jdk/compare/7bf1989f...d7cbf0d3 > > src/java.base/share/classes/java/lang/ref/Reference.java line 491: > >> 489: * method is unsuccessful and returns false. >> 490: * >> 491: * Memory >> consistency effects: > > Note the `https://git.openjdk.org/jdk/pull/16644#discussion_r1621570783
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v33]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: convert sample to snippet; add 'JLS' label to jls links - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/d7cbf0d3..4d06 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=32 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=31-32 Stats: 7 lines in 2 files changed: 1 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v32]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 69 commits: - Merge branch 'master' into refDocs2 - add link to Thread.isAlive() - small review tweaks; shorten MemoryConsistency links - small grammar fixes - new section for finalizer memviz - add memviz bullet for finalization - remove quotes from dequeue - package spec updates, mostly about reference queues and dequeueing - move reachability section before notification; update section header - add details on use of reference queues; swap reachability/memviz sections - ... and 59 more: https://git.openjdk.org/jdk/compare/7bf1989f...d7cbf0d3 - Changes: https://git.openjdk.org/jdk/pull/16644/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16644=31 Stats: 226 lines in 4 files changed: 132 ins; 19 del; 75 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v22]
On Wed, 17 Apr 2024 17:12:28 GMT, Brent Christian wrote: >> Brent Christian has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Another update to reachabilityFence() @apiNote > > AFAICT, all review feedback on this change has been addressed. I would love > to have some reviewers take another look. Thanks! > @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. - PR Comment: https://git.openjdk.org/jdk/pull/16644#issuecomment-2123059255
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v31]
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
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v31]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: add link to Thread.isAlive() - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/5db47889..4efa5d18 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=30 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=29-30 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v30]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: small review tweaks; shorten MemoryConsistency links - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/a41e6fc2..5db47889 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=29 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=28-29 Stats: 8 lines in 3 files changed: 0 ins; 1 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v29]
On Wed, 8 May 2024 08:33:31 GMT, Alan Bateman wrote:
>> Brent Christian has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> small grammar fixes
>
> src/java.base/share/classes/java/lang/ref/Cleaner.java line 224:
>
>> 222: * Actions in a thread prior to calling {@code Cleaner.register()}
>> 223: * > href="{@docRoot}/java.base/java/util/concurrent/package-summary.html#MemoryVisibility">happen-before
>> 224: * the cleaning action is run by the Cleaner's thread.
>
> In passing, you could reduces the html refs to
> "package-summary.html#MemoryConsistency" and
> "package-summary.html#MemoryVisibility" as it's the same package/directory.
MemoryVisibility is in java.util.concurrent
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1594785182
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v10]
On Fri, 23 Feb 2024 14:37:20 GMT, Alan Bateman wrote: >> Brent Christian has updated the pull request incrementally with one >> additional commit since the last revision: >> >> VM -> virtual machine; also fix misspelling > > src/java.base/share/classes/java/lang/ref/Cleaner.java line 218: > >> 216: * cautions about the behavior of cleaning actions. >> 217: * >> 218: * The object being registered is kept strongly reachable (and >> therefore not eligible > > I would be tempted to drop "being registered" from this sentence. The > previous paragraph and the parameter descriptions use "the object". That > would make it "The given object is kept strongly reachable ..." which I > think is a bit easier to read. If it's clear which object is meant, then yes, that reads better. - PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1594756233
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v28]
On Sat, 27 Apr 2024 11:58:52 GMT, Viktor Klang wrote:
>> Brent Christian has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> new section for finalizer memviz
>
> src/java.base/share/classes/java/lang/ref/package-info.java line 157:
>
>> 155: * The dequeuing of a reference to a
>> 156: * {@linkplain Cleaner#register(Object object, Runnable action)
>> registered}
>> 157: * object, by a Cleaner thread, happens-before that Cleaner
>> thread runs
>
> Are we committing to this? I.e. that there will always be a single Cleaner
> thread? (i.e. one could imagine a situation where you have one polling thread
> which executes the cleaning action on some other thread?)
I'll revert this back to "_the_ Cleaner thread".
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1583750541
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v29]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: small grammar fixes - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/5f90b5d8..a41e6fc2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=28 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=27-28 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v28]
On Sat, 27 Apr 2024 11:57:23 GMT, Viktor Klang wrote: >> Brent Christian has updated the pull request incrementally with one >> additional commit since the last revision: >> >> new section for finalizer memviz > > src/java.base/share/classes/java/lang/ref/package-info.java line 137: > >> 135: * >> 136: * Memory Consistency Properties >> 137: * Certain interactions between references, references queues, and the >> garbage > > Suggestion: > > * Certain interactions between references, reference queues, and the garbage Yes, thank you - PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1583736191
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v28]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: new section for finalizer memviz - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/77d53818..5f90b5d8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=27 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=26-27 Stats: 22 lines in 1 file changed: 14 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v27]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: add memviz bullet for finalization - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/0b9d3efc..77d53818 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=26 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=25-26 Stats: 7 lines in 1 file changed: 3 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v26]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: remove quotes from dequeue - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/16fcc764..0b9d3efc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=25 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=24-25 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v25]
On Thu, 25 Apr 2024 08:25:39 GMT, Viktor Klang wrote:
>> Brent Christian has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> package spec updates, mostly about reference queues and dequeueing
>
> src/java.base/share/classes/java/lang/ref/package-info.java line 153:
>
>> 151: * The enqueueing of a reference (by the garbage collector, or
>> 152: * by a successful call to {@link Reference#enqueue})
>> happens-before
>> 153: * the reference is removed from the queue (dequeued).
>
> @bchristi-git I wonder if it makes sense to use the same style when referring
> to "dequeue", so either always within quotes or only using em (see
> line 108)
Sure; I'll remove the quotes.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1580238810
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v25]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: package spec updates, mostly about reference queues and dequeueing - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/cc21d296..16fcc764 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=24 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=23-24 Stats: 15 lines in 1 file changed: 0 ins; 3 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v24]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: move reachability section before notification; update section header - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/27d0c249..cc21d296 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=23 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=22-23 Stats: 67 lines in 1 file changed: 33 ins; 33 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v23]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: add details on use of reference queues; swap reachability/memviz sections - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/91d4db48..27d0c249 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=22 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=21-22 Stats: 81 lines in 1 file changed: 41 ins; 33 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v22]
On Fri, 5 Apr 2024 23:13:39 GMT, Brent Christian wrote: >> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > Another update to reachabilityFence() @apiNote AFAICT, all review feedback on this change has been addressed. I would love to have some reviewers take another look. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/16644#issuecomment-2061795930
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v22]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Another update to reachabilityFence() @apiNote - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/bdac5cce..91d4db48 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=21 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=20-21 Stats: 19 lines in 1 file changed: 0 ins; 5 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v21]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: small reformat - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/170c9814..bdac5cce Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=20 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=19-20 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v20]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Stray blank line - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/4648d064..170c9814 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=19 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=18-19 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v19]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Re-remove paragraph break - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/0748ed04..4648d064 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=18 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=17-18 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v18]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: elaborate on VM optimizations, field values outliving their object, etc - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/54a44414..0748ed04 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=17 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=16-17 Stats: 14 lines in 1 file changed: 10 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v17]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Remove section about 'encapsulating' reachabilityFence() - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/15ae0f25..54a44414 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=16 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=15-16 Stats: 17 lines in 1 file changed: 0 ins; 17 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v16]
On Mon, 25 Mar 2024 19:26:41 GMT, Brent Christian wrote:
>> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_
>
> Brent Christian has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Small updates to reachabilityFence, per review
src/java.base/share/classes/java/lang/ref/Reference.java line 638:
> 636: *
> 637: * It is sometimes possible to better encapsulate use of
> 638: * {@code reachabilityFence}. Continuing the above example, if it
> were
[Per Hans in
email,](https://mail.openjdk.org/pipermail/core-libs-dev/2024-March/120747.html)
I agree that this is not a good example, and is misleading.
Even if `update()` could be called without harm if the finalizer has already
executed:
the user has called `action()` expecting some action (involving the
`externalResource`) to take place. With the `reachabilityFence()` call
"refactored" in this way, it is unclear whether that action actually takes
place.
Better to stick with the try/finally/reachabilityFence() pattern, and have
`action()` behave consistently.
This passage/example should be removed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1538183404
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v15]
On Fri, 22 Mar 2024 09:28:02 GMT, Viktor Klang wrote:
>> Brent Christian has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Elaborate on 'surprising and undesirable effects' in reachabilityFence()
>
> src/java.base/share/classes/java/lang/ref/Reference.java line 632:
>
>> 630: * object. This might be the case if, for example, a usage in a
>> user program
>> 631: * had the form {@code new Resource().action();} which retains no
>> other
>> 632: * reference to this {@code Resource}. The
>
> Reformat this line?
Suggestions?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1538120558
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v16]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Small updates to reachabilityFence, per review - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/3df288a5..15ae0f25 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=15 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=14-15 Stats: 5 lines in 1 file changed: 0 ins; 1 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v15]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Elaborate on 'surprising and undesirable effects' in reachabilityFence() - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/80a3973a..3df288a5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=14 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=13-14 Stats: 29 lines in 1 file changed: 11 ins; 0 del; 18 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v13]
On Sat, 16 Mar 2024 04:20:44 GMT, Stuart Marks wrote:
>> Brent Christian has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> further tweaks to reachability
>
> src/java.base/share/classes/java/lang/ref/Reference.java line 402:
>
>> 400: * method is called, the garbage collector may already be in the
>> process of
>> 401: * (or already completed) clearing and/or enqueueing this reference.
>> 402: *
>
> Either this is an extra blank line, or you need a `` here.
Removed the blank line; I thought it looked better for the API note to be a
single paragraph.
> src/java.base/share/classes/java/lang/ref/Reference.java line 496:
>
>> 494: * Actions in a thread prior to calling
>> 495: * {@code enqueue} successfully
>> 496: * > href="{@docRoot}/java.base/java/util/concurrent/package-summary.html#MemoryVisibility">happen-before
>
> Editorial. The text here says
>
>> Actions in a thread prior to calling `enqueue` successfully _happen-before_
>> the reference is removed...
>
> This could be confusing, because "successfully" might be read to modify
> "happen-before". This raises questions such as "Is it possible for something
> to happen-before unsuccessfully?" Of course you want "successfully" to modify
> "enqueue" because you're relying on the definition of "successful" given
> previously. Suggest rewording:
>
>> Actions in a thread prior to successful calls to `enqueue` _happen-before_
>> the reference is removed...
Updated, though I made it singular ("a successful call to enqueue"), since we
talk about "the" reference being removed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1529365850
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1529364758
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v13]
On Sat, 16 Mar 2024 04:21:55 GMT, Stuart Marks wrote:
>> Brent Christian has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> further tweaks to reachability
>
> src/java.base/share/classes/java/lang/ref/package-info.java line 127:
>
>> 125: * thread prior to a {@link Reference#reachabilityFence
>> Reference.reachabilityFence(x)}
>> 126: * happen-before cleanup code for {@code x} runs on the cleaner
>> thread.
>> 127: *
>
> Extra blank line or need ``.
Removed
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1529361892
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v14]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Updates per review comments - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/4d6f1dce..80a3973a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=13 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=12-13 Stats: 8 lines in 2 files changed: 0 ins; 3 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v13]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: further tweaks to reachability - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/603ff4dc..4d6f1dce Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=12 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=11-12 Stats: 7 lines in 1 file changed: 3 ins; 1 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v12]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: update definition of strongly reachable - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/855b13a8..603ff4dc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=11 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=10-11 Stats: 4 lines in 1 file changed: 0 ins; 1 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v11]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Tiny clarification to 'strongly reachable' definition; add link to Reference class - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/adb1fbe3..855b13a8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=10 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=09-10 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v10]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: VM -> virtual machine; also fix misspelling - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/0f949d3c..adb1fbe3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=09 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=08-09 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v9]
On Thu, 22 Feb 2024 23:14:37 GMT, Brent Christian wrote:
>> I note that the adjective(s) (un)successful and the adverb(s)
>> (un)successfully are used at several places in these comments, it might
>> makes sense to use those terms here as well such that the documentation in
>> internally consistent in its use of success or failure of actions. In
>> particular, if this terminology is consistent with precedent in the official
>> JLS spec.
>>
>> However, I note that there are places where these terms are italicized and
>> places where they aren't. I am not sure I follow the convention for
>> italicization. In general, the first use (i.e. introduction) of a term that
>> the reader might want to pay attention to calls for italicization when
>> documents are read sequentially, such as in research papers. These javadoc
>> specs will usually not be read in sequentially. But considering that someone
>> does read them in order, I'd suggest italicizing only the first use of the
>> term or, if not, then perhaps none. Alternatively, you might want to
>> italicize all uses (but why?).
>
> Thanks for finding my misspelling, djelinski.
The use of "(un)successful(ly)" in relation to `Reference.enqueue()` is quite
deliberate (and builds on the previous wording, "successful").
The intention was to use it consistently (is that not the case somewhere?). For
example, it's also used in the new **Memory Consistency Properties** section of
the `java.lang.ref` package docs ("The enqueueing of a reference...by a
successful call to `Reference.enqueue()`...").
A "successful call to `enqueue()`" is meant to be shorthand for:
"the reference has been enqueued, and the enqueuing was performed by the
`enqueue()` method (rather than by the garbage collector). Therefore there is a
_happens-before_ edge between the `enqueue()` method call and the dequeuing of
the Reference (whereas there would not be this _happens-before_ if the GC had
already enqueued the Reference at the time of the `enqueue()` call)."
The text emphasis with italics is to indicate this added significance of the
result of the `enqueue()` call -- ala `happens-before`.
I'm not aware of a similar scenario covered in the JLS, so AFAIK there is not
precedent to be consistent with in that regard.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1500073141
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v9]
On Thu, 22 Feb 2024 18:24:39 GMT, Y. Srinivas Ramakrishna wrote: >> or, better yet, `fails` > > I note that the adjective(s) (un)successful and the adverb(s) > (un)successfully are used at several places in these comments, it might makes > sense to use those terms here as well such that the documentation in > internally consistent in its use of success or failure of actions. In > particular, if this terminology is consistent with precedent in the official > JLS spec. > > However, I note that there are places where these terms are italicized and > places where they aren't. I am not sure I follow the convention for > italicization. In general, the first use (i.e. introduction) of a term that > the reader might want to pay attention to calls for italicization when > documents are read sequentially, such as in research papers. These javadoc > specs will usually not be read in sequentially. But considering that someone > does read them in order, I'd suggest italicizing only the first use of the > term or, if not, then perhaps none. Alternatively, you might want to > italicize all uses (but why?). Thanks for finding my misspelling, djelinski. - PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1500054507
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v9]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Cleaner thread dequeue happens-before running cleaning action - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/1ca169ec..0f949d3c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=08 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=07-08 Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v8]
On Wed, 21 Feb 2024 01:52:57 GMT, David Holmes wrote: >> Brent Christian has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Updates to reachabilityFence() > > src/java.base/share/classes/java/lang/ref/package-info.java line 118: > >> 116: * >> 117: * The Cleaner thread dequeues a reference to a registered object >> before >> 118: * running the cleaning action for that object. > > Do you want to rephrase this so that it too mentions `happens-before`? e.g. > > Dequeuing of a reference to a registered object, by the Cleaner thread, > happens-before the execution of the cleaning action for that object. I hesitated to state it that way because there's not a synchronization action _per se_ between the Cleaning thread dequeuing, and then running the cleaning action. However, I see that JLS 17.4.5 does state: _"If x and y are actions of the same thread and x comes before y in program order, then hb(x, y)."_ (That is, x _happens-before_ y). So yes, we can say _happens-before_ here. Thanks! - PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1498493650
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v8]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Updates to reachabilityFence() - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/7eb3397c..1ca169ec Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=07 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=06-07 Stats: 6 lines in 1 file changed: 2 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: CFV: New Core Libraries Group Member: Viktor Klang
Vote: Yes On 2/19/24 3:40 PM, Stuart Marks wrote: I hereby nominate Viktor Klang [6] to Membership in the Core Libraries Group [4].
Re: CFV: New Core Libraries Group Member: Raffaello Giulietti
Vote: Yes On 2/13/2024 12:25 PM, Brian Burkhalter wrote: I hereby nominate Raffaello Giulietti to Membership in the Core Libraries Group. Raffaello has been working in the Core Library team at Oracle since April, 2022. He has authored more than 50 contributions to OpenJDK in a number of areas including numerics, formatting, regular expressions, and random number generation. Votes are due by Wednesday, February 28, 2023. Only current Members of the Core Libraries Group [1] are eligible to vote on this nomination. Votes must be cast in the open by replying to this mailing list For Lazy Consensus voting instructions, see [2]. Brian Burkhalter [1] https://openjdk.org/census#core-libs [2] https://openjdk.org/groups/#member-vote
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v7]
On Mon, 27 Nov 2023 22:52:10 GMT, Hans Boehm wrote: >> Brent Christian has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Updates to clear() and enqueue() > > src/java.base/share/classes/java/lang/ref/package-info.java line 109: > >> 107: * >> 108: * The clearing of a reference by the garbage collector >> happens-before >> 109: * the garbage collector enqueues the reference. > > Is it worth specifying this middle step? Is there a way to tell that > something has been enqueued without removing the reference or calling the > deprecated (and very dubious) isEnqueued method? Could we just remove this > paragraph, and instead start the next one with "The clearing of a reference > by the garbage collector ..." Here, we are building a chain of _happens-befores_ that reaches from RF to dequeue (and on, to the running of the cleaning action, in the case of Cleaner). A ref can also be cleared by a call to clear(), but that has no memory consistency effects. So I think it's worth clarifying here that ref-clearing only forms a "link" in the _happens-before_ chain **_when performed by the GC_**. - PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1472189769
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v7]
On Mon, 27 Nov 2023 21:51:01 GMT, Hans Boehm wrote: >> Brent Christian has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Updates to clear() and enqueue() > > src/java.base/share/classes/java/lang/ref/package-info.java line 104: > >> 102: * >> 103: * >> 104: * Actions in a thread prior to calling > > Is line 104 needed? Can we just say that Refrence.reachaboilityFence(x) > happens-before ... This language is consistent with other parts of this fix (and elsewhere in the API spec). - PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1472172567
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v7]
On Thu, 30 Nov 2023 22:39:26 GMT, Brent Christian wrote: >> IMO, the core of the semantics here are that the reachabilityFence() call >> happens before clearing or enqueueing of any References to the argument. I >> think it's the call itself, not the end of the argument.s scope (which may >> not even make sense) that matters here. >> >> Perhaps reachabilityFence(x) should also happen before any objects that are >> definitely reachable from x (JLS 12.6.2) at that point are cleared or >> enqueued. That is not completely clear to me, since it impacts the legality >> of dead field elimination. And it would be nice to get rid of 12.6.2. > > The existing docs refer to "invocation" of this method. I've continued with > that, and in general have kept this at a bit higher level, in order to > simplify understanding. > > I'm open to more detailed wording, if it would improve understandability, (or > if what I have is not functionally accurate). > Perhaps reachabilityFence(x) should also happen before any objects that are > definitely reachable from x (JLS 12.6.2) at that point are cleared or > enqueued. I don't think we need to extend this to objects reachable from x. Consider what I consider to be the primary scenario - x has resources to cleanup after it becomes unreachable. x may refer to a variety of other objects; some will be the resources needing cleanup, others may be ordinary, non-resource objects. For proper cleanup, resource objects **_already_** must remain reachable longer than x (so their cleanup can be performed even after x is unreachable). Cleaner takes care of this for you (The Cleaner instance keeps a reference to the registered "cleaning action", which refers to the resources). Or, one must roll their own if using Reference+ReferenceQueue directly. - PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1472163987
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v7]
On Mon, 27 Nov 2023 22:33:16 GMT, Hans Boehm wrote: >> Brent Christian has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Updates to clear() and enqueue() > > src/java.base/share/classes/java/lang/ref/Reference.java line 546: > >> 544: * strongly >> reachable, >> 545: * regardless of any other actions of the program that might >> otherwise cause >> 546: * the object to become unreachable; thus, the object is not > > Delete "regardless of any other actions of the program that might otherwise > cause the object to become unreachable" ? I have no idea what actions would > qualify there. And it's more misleading thann informative, since IMO the real > concern here is compiler dead variable elimination. The only action I can think of would be nulling out the last strong reference to the object in question. - PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1472074132
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v7]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Updates to clear() and enqueue() - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/3647f64a..7eb3397c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=06 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=05-06 Stats: 7 lines in 1 file changed: 1 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v6]
On Tue, 23 Jan 2024 11:40:00 GMT, Kim Barrett wrote:
>> Brent Christian has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Tweak Reference.enqueue memory consistency effects wording
>
> src/java.base/share/classes/java/lang/ref/Reference.java line 508:
>
>> 506: * Use of this method allows the registered queue's
>> 507: * {@link ReferenceQueue#poll} and {@link ReferenceQueue#remove}
>> methods
>> 508: * to return this reference even though the referent is still
>> strongly
>
> Perhaps s/is still/may still be/
Sure, sounds good
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1467059185
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v6]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Tweak Reference.enqueue memory consistency effects wording - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/9b4b1150..3647f64a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=05 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=04-05 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v4]
On Wed, 10 Jan 2024 16:09:14 GMT, Viktor Klang wrote:
>> Brent Christian has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add links to new Memory Consistency section in package doc
>
> src/java.base/share/classes/java/lang/ref/Reference.java line 499:
>
>> 497: * the reference is removed from the queue by {@link
>> ReferenceQueue#poll}
>> 498: * or {@link ReferenceQueue#remove}. An unsuccessful
>> 499: * {@code enqueue} call has no memory consistency effects.
>
> Would it help to say that unsuccessful enqueue calls have no *reliable* or
> *specified* memory consistency effects?
Yes, I like that phrasing better, thanks
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1448025551
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v5]
On Sat, 18 Nov 2023 01:56:11 GMT, Mandy Chung wrote: >> Brent Christian has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove hyphen in 'strongly reachable' in Cleaner > > src/java.base/share/classes/java/lang/ref/Reference.java line 399: > >> 397: * (or already completed) clearing and/or enqueueing this reference. >> 398: * >> 399: * Avoid this race by ensuring the referent remains >> strongly-reachable until > > `s/strongly-reachable/strongly reachable/` no dash as in other occurrences. Changed in Cleaner, too - PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1448025318
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v5]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Remove hyphen in 'strongly reachable' in Cleaner - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/c09db360..9b4b1150 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=04 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8320919: Clarify Locale related system properties [v6]
On Thu, 14 Dec 2023 21:50:01 GMT, Naoto Sato wrote:
>> This is a doc change to clarify what the `Default Locale` is, and how it is
>> established during the system startup using the system properties. Those
>> locale-related system properties have existed since the early days of Java,
>> but have never been publicly documented before. It is also the intention of
>> this PR to clarify those system properties and how they are overridden. A
>> corresponding CSR has been drafted.
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reflects review comments
src/java.base/share/classes/java/util/Locale.java line 327:
> 325: * the value of the {@code user.language.display} system property will
> be used in the
> 326: * {@code language} part of the default Locale for {@link
> Locale.Category#DISPLAY}
> 327: * category. In the absence of category specific system properties, the
> "category-less"
"...part of the default Locale for **_the_** Locale.Category#DISPLAY category."
?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17065#discussion_r1428459997
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v4]
On Wed, 6 Dec 2023 01:24:51 GMT, Brent Christian wrote: >> Perhaps in each of these places add a link to #Memory Consistency Properties > > I can flesh out the new Memory Consistency Properties section in the package > info to cover the whole chain. Adding links to it sounds like a good idea. I've converted "Memory consistency effects:" to be a link to the new/updated section in the package doc. Maybe that's fine, or maybe we want to do a little more to direct the user to that section. - PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1424682474
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v4]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Add links to new Memory Consistency section in package doc - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/05477522..c09db360 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=03 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=02-03 Stats: 10 lines in 3 files changed: 2 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v3]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Describe whole chain of hb relationships - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/84d8b61f..05477522 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=01-02 Stats: 14 lines in 1 file changed: 13 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v2]
On Tue, 21 Nov 2023 09:16:17 GMT, Viktor Klang wrote:
>> src/java.base/share/classes/java/lang/ref/Reference.java line 564:
>>
>>> 562: * {@code reachabilityFence(x)}
>>> 563: * >> href="{@docRoot}/java.base/java/util/concurrent/package-summary.html#MemoryVisibility">happen-before
>>> 564: * the garbage collector clears any reference to {code x}.
>>
>> This is a fairly low-level specification, so the relationship described here
>> is "reachabilityFence **hb** clearing". The relationships "clearing **hb**
>> enqueue" and "enqueue **hb** dequeue" are described elsewhere. Thus the
>> mention of Cleaner above seems misplaced.
>>
>> However, the whole chain of relationships
>>
>>> RF **hb** clearing **hb** enqueue **hb** dequeue **hb** cleaning-action
>>
>> is important. Should this be described somewhere?
>
> Perhaps in each of these places add a link to #Memory Consistency Properties
I can flesh out the new Memory Consistency Properties section in the package
info to cover the whole chain. Adding links to it sounds like a good idea.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1416512195
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v2]
> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ Brent Christian has updated the pull request incrementally with one additional commit since the last revision: Address a number of PR comments - Changes: - all: https://git.openjdk.org/jdk/pull/16644/files - new: https://git.openjdk.org/jdk/pull/16644/files/dfce0dd5..84d8b61f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16644=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16644=00-01 Stats: 54 lines in 2 files changed: 15 ins; 25 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref
On Wed, 15 Nov 2023 22:45:46 GMT, Stuart Marks wrote:
>> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_
>
> src/java.base/share/classes/java/lang/ref/ReferenceQueue.java line 187:
>
>> 185: * {@link Reference#enqueue()} instead of by the garbage collector,
>> its
>> 186: * former referent (which has since been cleared) could still be
>> strongly
>> 187: * reachable.
>
> See my comments on the corresponding Reference.enqueue() apiNote. I'm not
> sure we want to have this issue sprinkled around in the apiNotes for several
> methods, including others here on ReferenceQueue.
>
> If we do want to address this issue -- and it's not clear to me that we do --
> then perhaps we should do it in a central location?
We can cover this issue just in `Reference.enqueue()`. I'll take out these
`@apiNote`s, and put in `@see Reference.enqueue()`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1411395204
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref
On Tue, 21 Nov 2023 22:58:09 GMT, Roger Riggs wrote: >> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ > > src/java.base/share/classes/java/lang/ref/Reference.java line 552: > >> 550: * of this method. >> 551: * Invocation of this method does not itself initiate reference >> processing, >> 552: * garbage collection, or finalization. > > My understanding was that it is not the object instance that is being > guarded, only that the reference holding the object is considered a strong > root and is only used to delimit a range of bytecodes for which the reference > is considered to be strong. > In particular, the invocation of the method itself has no semantics, only > that a control flow could reach that statement and the reference was > considered strong as long as the reference was in a scope that included the > reachability fence. The existing docs refer to "invocation" of this method. I've continued with that, and in general have kept this at a bit higher level, in order to simplify understanding. I'm open to more detailed wording, if it would improve understandability, (or if what I have is not functionally accurate). - PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1411372474
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref
On Tue, 21 Nov 2023 09:13:59 GMT, Viktor Klang wrote: >> src/java.base/share/classes/java/lang/ref/Reference.java line 505: >> >>> 503: * to return this reference even though the referent is still >>> strongly >>> 504: * reachable. That is, before the referent has reached the expected >>> 505: * reachability level. >> >> The wording here is kind of awkward. We could try to wordsmith it, but there >> is also the possibility that there are valid use cases for using enqueue(), >> which makes this caution seem misplaced. An alternative would simply be to >> omit this paragraph. > > Might be better to leave out the last sencence? I'll take out the last sentence, and we can continue discussing whether to keep this note at all. - PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1411305306
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref
On Tue, 21 Nov 2023 22:46:32 GMT, Roger Riggs wrote:
>> src/java.base/share/classes/java/lang/ref/Reference.java line 489:
>>
>>> 487: * If this reference was already enqueued (by the garbage
>>> collector, or a
>>> 488: * previous call to {@code enqueue}), this method is not
>>> successful,
>>> 489: * and returns false.
>>
>> If we're going to talk about successful and unsuccessful calls, we should
>> define what success is first. I guess that would be something like: if this
>> ref is registered with a queue and not already enqueued, it is enqueued
>> successfully and the method returns true. Otherwise, not successful, and
>> returns false.
>
> This could be worded as a post condition, after the call the ref is enqueued
> with the queue; the return is true iff the ref was not previously enqueued.
> The enqueuing is not conditional (assuming the queue is non-null).
I'll give that some thought. The enqueuing is not conditional, but the
_happens-before_ is...
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1410031119
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref
On Tue, 21 Nov 2023 09:12:49 GMT, Viktor Klang wrote:
>> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_
>
> src/java.base/share/classes/java/lang/ref/Reference.java line 497:
>
>> 495: * or {@link ReferenceQueue#remove}.
>> 496: *
>> 497: * This method is invoked only by Java code; when the garbage
>> collector
>
> Perhaps something along the lines of:
>
> Suggestion:
>
> * This method is only invoked explicitly; when the garbage collector
I think the important part to convey is that the GC won't call this method. So
how about _just_:
When the garbage collector enqueues references it does so directly, without
invoking this method.
?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1409993103
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref
On Sat, 18 Nov 2023 01:55:00 GMT, Mandy Chung wrote: >> Perhaps it's sufficient to say simply that, _"the garbage collector will no >> longer enqueue this object."_ > > Is this sentence needed? Excluding the race scenario as described in the > javadoc, it would be equivalent as if the Reference with null referent which > will never get enqueued? I personally think it's worth clearly stating that calling `clear()` opts you out of getting the GC notification that you signed up for when you registering this Reference with a RefQ. - PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1408648376
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref
On Tue, 21 Nov 2023 09:08:06 GMT, Viktor Klang wrote:
>> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_
>
> src/java.base/share/classes/java/lang/ref/Reference.java line 405:
>
>> 403: * This method is invoked only by Java code; when the garbage
>> collector
>> 404: * clears references it does so directly, without invoking this
>> method. The
>> 405: * {@link #enqueue} method also clears references directly, without
>> invoking
>
> The last sentence might make sense to have as an apiNote? 樂
I will remove that sentence here, and instead update the `enqueue()` doc to say
that the GC clears and enqueues references without calling `clear()` or
`enqueue()`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1408644358
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref
On Tue, 14 Nov 2023 14:06:29 GMT, Alan Bateman wrote: >> 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ > > src/java.base/share/classes/java/lang/ref/Reference.java line 393: > >> 391: * Clears this reference object. Invoking this method does not >> enqueue this >> 392: * object, and the garbage collector will no longer enqueue this >> object once >> 393: * the referent reaches the designated reachability level. > > I'm wondering if "designated reachability level" is the right words to use > here. The Notification section in the package description speaks of when the > reachability of the referent has changed to the value corresponding to the > type of the reference and I wonder if it might be better to use wording > consistent with that. Minimally it could link to the package description > where the notion of reachability level is introduced. Perhaps it's sufficient to say simply that, _"the garbage collector will no longer enqueue this object."_ - PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1394969775
Re: RFR: 8319928: Exceptions thrown by cleanup actions should be handled correctly
On Mon, 13 Nov 2023 11:08:13 GMT, Maurizio Cimadamore wrote: > Silently ignoring exceptions seems bad, but something that is forced when > using a Cleaner I also don't like that Cleaner ignores exceptions. One idea I have: [8305979 : UncaughtExceptionHandler for Cleaner ](https://bugs.openjdk.org/browse/JDK-8305979). - PR Comment: https://git.openjdk.org/jdk/pull/16619#issuecomment-1813163393
RFR: 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](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#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."_ - Commit messages: - Merge - Update enqueue() docs, add warnings about still-reachable referent - small tweak to enqueue() - Updates to clear() and enqueue() - updates re: reference clearing - Per slack, 'on a queue' is inconsistent with existing wording - reworked - Update 'remains reachable', per Slack - tweak again - tweak - consistency tweaks - ... and 28 more: https://git.openjdk.org/jdk/compare/cc4b0d92...dfce0dd5 Changes: https://git.openjdk.org/jdk/pull/16644/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16644=00 Issue: https://bugs.openjdk.org/browse/JDK-8314480 Stats: 108 lines in 4 files changed: 87 ins; 0 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/16644.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16644/head:pull/16644 PR: https://git.openjdk.org/jdk/pull/16644
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v16]
On Fri, 22 Jul 2022 20:51:59 GMT, Brent Christian wrote: >> Please review this change to replace the finalizer in >> `AbstractLdapNamingEnumeration` with a Cleaner. >> >> The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult >> res`, and `LdapClient enumClnt`) are moved to a static inner class . From >> there, the change is fairly mechanical. >> >> Details of note: >> 1. Some operations need to change the state values (the update() method is >> probably the most interesting). >> 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read >> `homeCtx` from the superclass's `state`. >> >> The test case is based on a copy of >> `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test >> case might be possible, but this was done for expediency. >> >> The test only confirms that the new Cleaner use does not keep the object >> reachable. It only tests `LdapSearchEnumeration` (not >> `LdapNamingEnumeration` or `LdapBindingEnumeration`, though all are >> subclasses of `AbstractLdapNamingEnumeration`). >> >> Thanks. > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > remove some more tabs Your persistence finally paid off, **bridgekeeper** - well played. - PR Comment: https://git.openjdk.org/jdk/pull/8311#issuecomment-1736077085
Re: RFR: 8314896: additional clarifications to reversed() default methods' implementation requirements
On Mon, 18 Sep 2023 21:12:44 GMT, Stuart Marks wrote: > Wording changes to make clear that the scenarios described are merely > examples and are not normative requirements. Looks good. - Marked as reviewed by bchristi (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15799#pullrequestreview-1640769251
Re: RFR: 8316305: Initial buffer size of StackWalker is too small caused by JDK-8285447 [v4]
On Fri, 22 Sep 2023 17:32:32 GMT, Mandy Chung wrote: >> A trivial fix. [JDK-8285447](https://bugs.openjdk.org/browse/JDK-8285447) >> intends to change the initial batch size only for a stack walker with an >> estimated stack depth. For stack walkers without user-supplied estimated >> stack depth, the initial batch size is changed to 3 which is a bug. This >> causes the stack walker to fetch the second batch after walking 2 frames. > > Mandy Chung has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains six commits: > > - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8316305 > - Revert this change. PR 15876 > - Fix a warning caused by 8316456 > - Merge > - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8316305 > - 8316305: Initial buffer size of StackWalker is too small caused by > JDK-8285447 Marked as reviewed by bchristi (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15749#pullrequestreview-1640462720
Re: RFR: 8316305: Initial buffer size of StackWalker is too small caused by JDK-8285447
On Thu, 14 Sep 2023 21:15:48 GMT, Mandy Chung wrote: >> src/java.base/share/classes/java/lang/StackStreamFactory.java line 544: >> >>> 542: return walker.estimateDepth() == 0 >>> 543: ? SMALL_BATCH >>> 544: : Math.min(walker.estimateDepth() + >>> RESERVED_ELEMENTS, LARGE_BATCH_SIZE); >> >> Without the >> `Math.max(walker.estimateDepth()+RESERVED_ELEMENTS, MIN_BATCH_SIZE)` >> for estimateDepth = 1, I believe this will now return 2, where previously it >> returned 3. >> Is that OK? > > yes as it's asked by the user. It will fetch the second batch if it walks > more than 1 frame. Sounds good, thanks - PR Review Comment: https://git.openjdk.org/jdk/pull/15749#discussion_r1326532469
Re: RFR: 8316305: Initial buffer size of StackWalker is too small caused by JDK-8285447
On Thu, 14 Sep 2023 17:06:53 GMT, Mandy Chung wrote: > A trivial fix. [JDK-8285447](https://bugs.openjdk.org/browse/JDK-8285447) > intends to change the initial batch size only for a stack walker with an > estimated stack depth. For stack walkers without user-supplied estimated > stack depth, the initial batch size is changed to 3 which is a bug. This > causes the stack walker to fetch the second batch after walking 2 frames. Marked as reviewed by bchristi (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15749#pullrequestreview-1627775967
Re: RFR: 8316305: Initial buffer size of StackWalker is too small caused by JDK-8285447
On Thu, 14 Sep 2023 17:06:53 GMT, Mandy Chung wrote: > A trivial fix. [JDK-8285447](https://bugs.openjdk.org/browse/JDK-8285447) > intends to change the initial batch size only for a stack walker with an > estimated stack depth. For stack walkers without user-supplied estimated > stack depth, the initial batch size is changed to 3 which is a bug. This > causes the stack walker to fetch the second batch after walking 2 frames. src/java.base/share/classes/java/lang/StackStreamFactory.java line 544: > 542: return walker.estimateDepth() == 0 > 543: ? SMALL_BATCH > 544: : Math.min(walker.estimateDepth() + > RESERVED_ELEMENTS, LARGE_BATCH_SIZE); Without the `Math.max(walker.estimateDepth()+RESERVED_ELEMENTS, MIN_BATCH_SIZE)` for estimateDepth = 1, I believe this will now return 2, where previously it returned 3. Is that OK? - PR Review Comment: https://git.openjdk.org/jdk/pull/15749#discussion_r1326495665
Re: RFR: 8285447: StackWalker minimal batch size should be optimized for getCallerClass [v4]
On Tue, 12 Sep 2023 21:09:25 GMT, Mandy Chung wrote:
>> test/micro/org/openjdk/bench/java/lang/CallerClassBench.java line 45:
>>
>>> 43: public class CallerClassBench {
>>> 44: static final StackWalker INST =
>>> StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE);
>>> 45:
>>
>> Could `DROP_METHOD_INFO` also be used here?
>
> yes but it's irrelevant in this benchmark as `getCallerClass` should be
> independent to `DROP_METHOD_INFO` option. The implementation always drops
> method info in the implementation. It does not affect the result.
Ah - understood, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15642#discussion_r1323600936
Re: RFR: 8285447: StackWalker minimal batch size should be optimized for getCallerClass [v4]
On Mon, 11 Sep 2023 17:52:56 GMT, Mandy Chung wrote:
>> Typically it will find the caller class at the second stack frame from the
>> frame of executing `StackWalker::getCallerClass`. The initial size of the
>> buffer can be changed from 8 to 4 (the top 2 elements are reserved for
>> implementation use). If it fetches another batch, `getCallerClass` may be
>> invoked via core reflection, so subsequent batches can be increased to a
>> larger size. This PR also moves the benchmark for `getCallerClass` in its
>> own file because it does not need to test with different depth and can be
>> separated from StackWalkBench.
>
> Mandy Chung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> cleanup
test/micro/org/openjdk/bench/java/lang/CallerClassBench.java line 45:
> 43: public class CallerClassBench {
> 44: static final StackWalker INST =
> StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE);
> 45:
Could `DROP_METHOD_INFO` also be used here?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15642#discussion_r1323574924
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock. [v2]
On Fri, 8 Sep 2023 08:03:51 GMT, Alan Bateman wrote: > > We could do this mid-term in some follow up action. > > It shouldn't be hard to try it. If it works out then it would mean this old > crufty code goes away and the JDK is in a better place. If it doesn't work > out then the a plan B would be to add to have lockFile0 throw with a useful > exception. I would also like to see what a FileChannel implementation looks like. Am I right that this would allow the `prefs` native library to be removed entirely? - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1712087144
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v11]
On Thu, 7 Sep 2023 18:22:40 GMT, Mandy Chung wrote: >> 8268829: Provide an optimized way to walk the stack with Class object only >> >> `StackWalker::walk` creates one `StackFrame` per frame and the current >> implementation >> allocates one `StackFrameInfo` and one `MemberName` objects per frame. Some >> frameworks >> like logging may only interest in the Class object but not the method name >> nor the BCI, >> for example, filters out its implementation classes to find the caller >> class. It's >> similar to `StackWalker::getCallerClass` but allows a predicate to filter >> out the element. >> >> This PR proposes to add `Option::DROP_METHOD_INFO` enum that requests to >> drop the method information. If no method information is needed, a >> `StackWalker` with `DROP_METHOD_INFO` >> can be used instead and such stack walker will save the overhead of >> extracting the method information >> and the memory used for the stack walking. >> >> New factory methods to take a parameter to specify the kind of stack walker >> to be created are defined. >> This provides a simple way for existing code, for example logging >> frameworks, to take advantage of >> this enhancement with the least change as it can keep the existing function >> for traversing >> `StackFrame`s. >> >> For example: to find the first caller filtering a known list of >> implementation class, >> existing code can create a stack walker instance with `DROP_METHOD_INFO` >> option: >> >> >> StackWalker walker = StackWalker.getInstance(Option.DROP_METHOD_INFO, >> Option.RETAIN_CLASS_REFERENCE); >> Optional> callerClass = walker.walk(s -> >> s.map(StackFrame::getDeclaringClass) >> .filter(Predicate.not(implClasses::contains)) >> .findFirst()); >> >> >> If method information is accessed on the `StackFrame`s produced by this >> stack walker such as >> `StackFrame::getMethodName`, then `UnsupportedOperationException` will be >> thrown. >> >> Javadoc & specdiff >> >> https://cr.openjdk.org/~mchung/api/java.base/java/lang/StackWalker.html >> https://cr.openjdk.org/~mchung/jdk22/specdiff/overview-summary.html >> >> Alternatives Considered >> One alternative is to provide a new API: >> ` T walkClass(Function, ? extends T> function)` >> >> In this case, the caller would need to pass a function that takes a stream >> of `Class` object instead of `StackFrame`. Existing code would have to >> modify calls to the `walk` method to `walkClass` and the function body. >> >> ### Implementation Details >> >> A `StackWalker` configured with `DROP_METHOD_INFO` ... > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > review feedback Looks great - thanks! - Marked as reviewed by bchristi (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15370#pullrequestreview-1616029850
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v9]
On Tue, 5 Sep 2023 17:52:44 GMT, Mandy Chung wrote: >> test/micro/org/openjdk/bench/java/lang/StackWalkBench.java line 64: >> >>> 62: default -> throw new IllegalArgumentException(name); >>> 63: }; >>> 64: } >> >> The previous `WALKER_DEFAULT` would not have retained the Class reference, >> but the new `default` will? > > Some benchmarks need the Class reference but some do not. For simplicity, > use only walkers that retain Class reference so that all benchmarks can run > with the default walker. In my mind, a "default" StackWalker (obtained from no-arg`StackWalker.getInstance()`) does not retain the Class instance. I think this will be confusing when the "default" Param value is reported in JMH results. I like running the benchmarks with both sets of StackWalker options, but I think the `default` Param value should be changed to something like, `class+methods`. - PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1317859222
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v9]
On Thu, 31 Aug 2023 17:09:40 GMT, Mandy Chung wrote:
>> 8268829: Provide an optimized way to walk the stack with Class object only
>>
>> `StackWalker::walk` creates one `StackFrame` per frame and the current
>> implementation
>> allocates one `StackFrameInfo` and one `MemberName` objects per frame. Some
>> frameworks
>> like logging may only interest in the Class object but not the method name
>> nor the BCI,
>> for example, filters out its implementation classes to find the caller
>> class. It's
>> similar to `StackWalker::getCallerClass` but allows a predicate to filter
>> out the element.
>>
>> This PR proposes to add `Option::DROP_METHOD_INFO` enum that requests to
>> drop the method information. If no method information is needed, a
>> `StackWalker` with `DROP_METHOD_INFO`
>> can be used instead and such stack walker will save the overhead of
>> extracting the method information
>> and the memory used for the stack walking.
>>
>> New factory methods to take a parameter to specify the kind of stack walker
>> to be created are defined.
>> This provides a simple way for existing code, for example logging
>> frameworks, to take advantage of
>> this enhancement with the least change as it can keep the existing function
>> for traversing
>> `StackFrame`s.
>>
>> For example: to find the first caller filtering a known list of
>> implementation class,
>> existing code can create a stack walker instance with `DROP_METHOD_INFO`
>> option:
>>
>>
>> StackWalker walker = StackWalker.getInstance(Option.DROP_METHOD_INFO,
>> Option.RETAIN_CLASS_REFERENCE);
>> Optional> callerClass = walker.walk(s ->
>> s.map(StackFrame::getDeclaringClass)
>> .filter(Predicate.not(implClasses::contains))
>> .findFirst());
>>
>>
>> If method information is accessed on the `StackFrame`s produced by this
>> stack walker such as
>> `StackFrame::getMethodName`, then `UnsupportedOperationException` will be
>> thrown.
>>
>> Javadoc & specdiff
>>
>> https://cr.openjdk.org/~mchung/api/java.base/java/lang/StackWalker.html
>> https://cr.openjdk.org/~mchung/jdk22/specdiff/overview-summary.html
>>
>> Alternatives Considered
>> One alternative is to provide a new API:
>> ` T walkClass(Function, ? extends T> function)`
>>
>> In this case, the caller would need to pass a function that takes a stream
>> of `Class` object instead of `StackFrame`. Existing code would have to
>> modify calls to the `walk` method to `walkClass` and the function body.
>>
>> ### Implementation Details
>>
>> A `StackWalker` configured with `DROP_METHOD_INFO` ...
>
> Mandy Chung has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 29 commits:
>
> - Merge
> - Remove the new getInstance method taking varargs
> - update mode to be int rather than long
> - update tests
> - Review feedback on javadoc
> - Revised the API change. Add Option::DROP_METHOD_INFO
> - Review feedback from Remi
> - fixup javadoc
> - Review feedback: move JLIA to ClassFrameInfo
> - review feedback and javadoc clean up
> - ... and 19 more: https://git.openjdk.org/jdk/compare/c8acab1d...111661bc
Looks good. I like `DROP_METHOD_INFO`. I have just a few minor comments.
src/java.base/share/classes/java/lang/StackStreamFactory.java line 657:
> 655: static final class ClassFrameBuffer extends
> FrameBuffer {
> 656: final StackWalker walker;
> 657: ClassFrameInfo[] classFrames; // caller class for fast path
Maybe I missed it, but I don't see any differences between `ClassFramesBuffer`
and `StackFrameBuffer` other than the `ClassFrameInfo`/`StackFrameInfo` types.
Could a single, generified Buffer class serve for both?
test/micro/org/openjdk/bench/java/lang/StackWalkBench.java line 64:
> 62: default -> throw new IllegalArgumentException(name);
> 63: };
> 64: }
The previous `WALKER_DEFAULT` would not have retained the Class reference, but
the new `default` will?
test/micro/org/openjdk/bench/java/lang/StackWalkBench.java line 360:
> 358: }
> 359: }
> 360:
I'm fine having this benchmark active by default, though offline we had
discussed adding it commented out.
-
PR Review: https://git.openjdk.org/jdk/pull/15370#pullrequestreview-1603548954
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1312375997
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1316143011
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1316205105
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v8]
On Tue, 29 Aug 2023 20:51:56 GMT, Mandy Chung wrote:
>> 8268829: Provide an optimized way to walk the stack with Class object only
>>
>> `StackWalker::walk` creates one `StackFrame` per frame and the current
>> implementation
>> allocates one `StackFrameInfo` and one `MemberName` objects per frame. Some
>> frameworks
>> like logging may only interest in the Class object but not the method name
>> nor the BCI,
>> for example, filters out its implementation classes to find the caller
>> class. It's
>> similar to `StackWalker::getCallerClass` but allows a predicate to filter
>> out the element.
>>
>> This PR proposes to add `Option::DROP_METHOD_INFO` enum that requests to
>> drop the method information. If no method information is needed, a
>> `StackWalker` with `DROP_METHOD_INFO`
>> can be used instead and such stack walker will save the overhead of
>> extracting the method information
>> and the memory used for the stack walking.
>>
>> New factory methods to take a parameter to specify the kind of stack walker
>> to be created are defined.
>> This provides a simple way for existing code, for example logging
>> frameworks, to take advantage of
>> this enhancement with the least change as it can keep the existing function
>> for traversing
>> `StackFrame`s.
>>
>> For example: to find the first caller filtering a known list of
>> implementation class,
>> existing code can create a stack walker instance with `DROP_METHOD_INFO`
>> option:
>>
>>
>> StackWalker walker = StackWalker.getInstance(Option.DROP_METHOD_INFO,
>> Option.RETAIN_CLASS_REFERENCE);
>> Optional> callerClass = walker.walk(s ->
>> s.map(StackFrame::getDeclaringClass)
>> .filter(Predicate.not(implClasses::contains))
>> .findFirst());
>>
>>
>> If method information is accessed on the `StackFrame`s produced by this
>> stack walker such as
>> `StackFrame::getMethodName`, then `UnsupportedOperationException` will be
>> thrown.
>>
>> Javadoc & specdiff
>>
>> https://cr.openjdk.org/~mchung/api/java.base/java/lang/StackWalker.html
>> https://cr.openjdk.org/~mchung/jdk22/specdiff/overview-summary.html
>>
>> Alternatives Considered
>> One alternative is to provide a new API:
>> ` T walkClass(Function, ? extends T> function)`
>>
>> In this case, the caller would need to pass a function that takes a stream
>> of `Class` object instead of `StackFrame`. Existing code would have to
>> modify calls to the `walk` method to `walkClass` and the function body.
>>
>> ### Implementation Details
>>
>> A `StackWalker` configured with `DROP_METHOD_INFO` ...
>
> Mandy Chung has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - update mode to be int rather than long
> - update tests
> - Review feedback on javadoc
src/java.base/share/classes/java/lang/ClassFrameInfo.java line 31:
> 29: import java.lang.StackWalker.StackFrame;
> 30:
> 31: class ClassFrameInfo implements StackFrame {
Maybe a minimal comment on when/how this class is used?
src/java.base/share/classes/java/lang/ClassFrameInfo.java line 37:
> 35: int flags; // updated by VM to set hidden and
> caller-sensitive bits
> 36:
> 37: ClassFrameInfo(StackWalker walker) {
The StackFrameInfo constructor has comment. Maybe add one here, too?
src/java.base/share/classes/java/lang/StackFrameInfo.java line 32:
> 30: import java.lang.reflect.Modifier;
> 31:
> 32: class StackFrameInfo extends ClassFrameInfo {
Maybe a minimal comment on when/how this class is used?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1310866414
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1310876832
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1310866565
Re: CFV: New Core Libraries Group Member: Michael McMahon
Vote: Yes On 8/25/23 8:24 AM, Roger Riggs wrote: I hereby nominate Michael McMahon to Membership in the Core Libraries Group
Re: CFV: New Core Libraries Group Member: Lance Andersen
Vote: Yes On 8/25/23 8:23 AM, Roger Riggs wrote: I hereby nominate Lance Andersen to Membership in the Core Libraries Group
Re: CFV: New Core Libraries Group Member: Daniel Fuchs
Vote: Yes On 8/25/23 8:23 AM, Roger Riggs wrote: I hereby nominate Daniel Fuchs to Membership in the Core Libraries Group
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v5]
On Thu, 24 Aug 2023 22:03:53 GMT, Mandy Chung wrote: > I like separating this from `Option` as it specifies what information to be > collected from each frame whereas `Option` controls everything else such as > what frames to be filtered In my mind, `Option` _already_ can control what information is to be collected: `RETAIN_CLASS_REFERENCE` as well as what frames to filter: `SHOW_HIDDEN_FRAMES` `SHOW_REFLECT_FRAMES` - PR Comment: https://git.openjdk.org/jdk/pull/15370#issuecomment-1692537575
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v5]
On Thu, 24 Aug 2023 18:44:14 GMT, Mandy Chung wrote:
>> 8268829: Provide an optimized way to walk the stack with Class object only
>>
>> `StackWalker::walk` creates one `StackFrame` per frame and the current
>> implementation
>> allocates one `StackFrameInfo` and one `MemberName` objects per frame. Some
>> frameworks
>> like logging may only interest in the Class object but not the method name
>> nor the BCI,
>> for example, filters out its implementation classes to find the caller
>> class. It's
>> similar to `StackWalker::getCallerClass` but allows a predicate to filter
>> out the element.
>>
>> This PR proposes to add `StackWalker.Kind` enum to specify the information
>> that a stack walker
>> collects. If no method information is needed, a `StackWalker` of
>> `CLASS_INFO` can be used
>> instead and such stack walker will save the overhead (1) to extract the
>> method information
>> and (2) the memory used for the stack walking. In addition, this can also
>> fix
>>
>> - [8311500](https://bugs.openjdk.org/browse/JDK-8311500):
>> StackWalker.getCallerClass() throws UOE if invoked reflectively
>>
>> New factory methods to take a parameter to specify the kind of stack walker
>> to be created are defined.
>> This provides a simple way for existing code, for example logging
>> frameworks, to take advantage of
>> this enhancement with the least change as it can keep the existing function
>> for traversing
>> `StackFrame`s.
>>
>> For example: to find the first caller filtering a known list of
>> implementation class,
>> existing code can call `StackWalker::getInstance(CLASS_INFO, ...)` to create
>> a stack walker instance:
>>
>>
>> StackWalker walker = StackWalker.getInstance(Kind.CLASS_INFO,
>> Option.RETAIN_CLASS_REFERENCE);
>> Optional> callerClass = walker.walk(s ->
>> s.map(StackFrame::getDeclaringClass)
>> .filter(interestingClasses::contains)
>> .findFirst());
>>
>>
>> If method information is accessed on the `StackFrame`s produced by this
>> stack walker such as
>> `StackFrame::getMethodName`, then `UnsupportedOperationException` will be
>> thrown.
>>
>> Alternatives Considered
>> One alternative is to provide a new API:
>> ` T walkClass(Function, ? extends T> function)`
>>
>> In this case, the caller would need to pass a function that takes a stream
>> of `Class` object instead of `StackFrame`. Existing code would have to
>> modify calls to the `walk` method to `walkClass` and the function body.
>>
>> Another alternative is to add a new `NO_METHOD_INFO` option. Similar to the
>> proposed API,
>>...
>
> Mandy Chung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> review feedback and javadoc clean up
src/java.base/share/classes/java/lang/StackWalker.java line 47:
> 45: * from the stack frames by default. If the method information is not
> needed,
> 46: * a {@code StackWalker} collecting {@linkplain Kind#CLASS_INFO class
> only information}
> 47: * can be used instead via the {@link #getInstance(Kind, Option...)}
> method.
The description of `Kind` should come after the basics of what StackWalker does:
* The {@link StackWalker#walk walk} method opens a sequential stream
* of {@link StackFrame StackFrame}s for the current thread and then applies
* the given function to walk the {@code StackFrame} stream.
It should be clarified that `METHOD_INFO` returns method information _as well
as_ class information.
Overall I would still prefer adding an `Option.OMIT_METHOD_INFO` over a new
`Kind` enum.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1304929416
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v3]
On Tue, 22 Aug 2023 19:01:53 GMT, Mandy Chung wrote: >> 8268829: Provide an optimized way to walk the stack with Class object only >> >> `StackWalker::walk` creates one `StackFrame` per frame and the current >> implementation >> allocates one `StackFrameInfo` and one `MemberName` objects per frame. Some >> frameworks >> like logging may only interest in the Class object but not the method name >> nor the BCI, >> for example, filters out its implementation classes to find the caller >> class. It's >> similar to `StackWalker::getCallerClass` but allows a predicate to filter >> out the element. >> >> This PR proposes to add `StackWalker.Kind` enum to specify the information >> that a stack walker >> collects. If no method information is needed, a `StackWalker` of >> `CLASS_INFO` can be used >> instead and such stack walker will save the overhead (1) to extract the >> method information >> and (2) the memory used for the stack walking. In addition, this can also >> fix >> >> - [8311500](https://bugs.openjdk.org/browse/JDK-8311500): >> StackWalker.getCallerClass() throws UOE if invoked reflectively >> >> New factory methods to take a parameter to specify the kind of stack walker >> to be created are defined. >> This provides a simple way for existing code, for example logging >> frameworks, to take advantage of >> this enhancement with the least change as it can keep the existing function >> for traversing >> `StackFrame`s. >> >> For example: to find the first caller filtering a known list of >> implementation class, >> existing code can call `StackWalker::getInstance(CLASS_INFO, ...)` to create >> a stack walker instance: >> >> >> StackWalker walker = StackWalker.getInstance(Kind.CLASS_INFO, >> Option.RETAIN_CLASS_REFERENCE); >> Optional> callerClass = walker.walk(s -> >> s.map(StackFrame::getDeclaringClass) >> .filter(interestingClasses::contains) >> .findFirst()); >> >> >> If method information is accessed on the `StackFrame`s produced by this >> stack walker such as >> `StackFrame::getMethodName`, then `UnsupportedOperationException` will be >> thrown. >> >> Alternatives Considered >> One alternative is to provide a new API: >> ` T walkClass(Function, ? extends T> function)` >> >> In this case, the caller would need to pass a function that takes a stream >> of `Class` object instead of `StackFrame`. Existing code would have to >> modify calls to the `walk` method to `walkClass` and the function body. >> >> Another alternative is to add a new `NO_METHOD_INFO` option. Similar to the >> proposed API, >>... > > Mandy Chung has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 16 commits: > > - Replace NO_METHOD_INFO option with StackWalker.Kind enums > - Merge branch 'master' of https://github.com/openjdk/jdk into > stackwalker-class > - clean up > - fix trailing whitespace > - Merge branch 'master' of https://github.com/openjdk/jdk into > stackwalker-class > - Update LocalLongHelper.java > - clean up > - Merge branch 'master' of https://github.com/openjdk/jdk into > stackwalker-class > - Add StackWalker.Option.NO_METHOD_INFO > - Merge branch 'master' of https://github.com/openjdk/jdk into > stackwalker-class > - ... and 6 more: https://git.openjdk.org/jdk/compare/ce1ded1a...434d90cb Sorry for not getting a look at this earlier. I don't think adding a new `Kind` enum is the best way to do this. StackWalkers would be configured along two different axes (`Kind` _and_ `Options`). It changes the mental model, in that all StackWalkers would now be divided into two `Kind`s. I feel like this bleeds the implementation into the API a bit. The existing `Option` enum already provides a way to configure which frames are walked, and what information to include in those frames. I think adding a new `Option` value fits better. It's true that compatibility dictates that the default behavior be to _include_ method info, so the new option must _omit_ method info. If the `NO_METHOD_INFO` is disliked, perhaps a better name can be found - `SKIP_METHOD_INFO` or `OMIT_METHOD_INFO`? - PR Comment: https://git.openjdk.org/jdk/pull/15370#issuecomment-1690187998
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v16]
On Fri, 22 Jul 2022 20:51:59 GMT, Brent Christian wrote: >> Please review this change to replace the finalizer in >> `AbstractLdapNamingEnumeration` with a Cleaner. >> >> The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult >> res`, and `LdapClient enumClnt`) are moved to a static inner class . From >> there, the change is fairly mechanical. >> >> Details of note: >> 1. Some operations need to change the state values (the update() method is >> probably the most interesting). >> 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read >> `homeCtx` from the superclass's `state`. >> >> The test case is based on a copy of >> `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test >> case might be possible, but this was done for expediency. >> >> The test only confirms that the new Cleaner use does not keep the object >> reachable. It only tests `LdapSearchEnumeration` (not >> `LdapNamingEnumeration` or `LdapBindingEnumeration`, though all are >> subclasses of `AbstractLdapNamingEnumeration`). >> >> Thanks. > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > remove some more tabs July already! - PR Comment: https://git.openjdk.org/jdk/pull/8311#issuecomment-1638979092
Re: RFR: JDK-8310814: Clarify the targetName parameter of Lookup::findClass [v2]
On Mon, 26 Jun 2023 23:13:19 GMT, Mandy Chung wrote: >> This PR updates the spec of `Lookup::findClass` to reflect the current >> behavior that requires a binary name or a string representing an array class >> in the form as returned by `Class::getName`. >> >> `test/jdk/java/lang/invoke/accessClassAndFindClass/TestFindClass.java` >> already covers the test cases of a nested class and an array class. > > Mandy Chung has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains one additional > commit since the last revision: > > 8310833: Clarify the targetName parameter of Lookup::findClass Looks fine. - Marked as reviewed by bchristi (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14668#pullrequestreview-1533774019
Re: RFR: 8308694: Clarify reversed() default methods' implementation requirements
On Fri, 16 Jun 2023 15:30:55 GMT, Roger Riggs wrote:
>> Can I get a preliminary review of the wording for Deque.reversed()? If the
>> text is good, I'll make corresponding changes to the implSpecs of the other
>> reversed() default methods, namely those in List, SortedMap, and SortedSet
>> and then file the CSR.
>
> src/java.base/share/classes/java/util/Deque.java line 626:
>
>> 624: * inverse-ordered method of this Deque. For example, the {@code
>> getFirst}
>> 625: * method of the returned view delegates to the {@code getLast}
>> method of
>> 626: * this Deque.
>
> The description is general and using 'For example" avoids having to be
> specific about every operation.
> The term "inverse-ordered" is new to this context and perhaps should be
> "reverse-ordered".
> "inverse" does appear in a few other class descriptions for SequencedXXX but
> its relation to "reverse" may not be clear.
Rather than describing how the methods delegate, I think we mostly want to say
that
order-sensitive operations are performed in reverse order.
I agree about, "For example", and I would keep the `getFirst` -> `getLast` part
- that's a good, straighforward example.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14504#discussion_r1232571578
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v16]
On Fri, 22 Jul 2022 20:51:59 GMT, Brent Christian wrote: >> Please review this change to replace the finalizer in >> `AbstractLdapNamingEnumeration` with a Cleaner. >> >> The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult >> res`, and `LdapClient enumClnt`) are moved to a static inner class . From >> there, the change is fairly mechanical. >> >> Details of note: >> 1. Some operations need to change the state values (the update() method is >> probably the most interesting). >> 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read >> `homeCtx` from the superclass's `state`. >> >> The test case is based on a copy of >> `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test >> case might be possible, but this was done for expediency. >> >> The test only confirms that the new Cleaner use does not keep the object >> reachable. It only tests `LdapSearchEnumeration` (not >> `LdapNamingEnumeration` or `LdapBindingEnumeration`, though all are >> subclasses of `AbstractLdapNamingEnumeration`). >> >> Thanks. > > Brent Christian has updated the pull request incrementally with one > additional commit since the last revision: > > remove some more tabs Let's hang on to this for a bit longer - PR Comment: https://git.openjdk.org/jdk/pull/8311#issuecomment-1585244767
Re: RFR: 8309630: Clean up tests that reference deploy modules
On Wed, 7 Jun 2023 19:03:31 GMT, Mandy Chung wrote: > Trivial fix. The deploy modules no longer exist. The tests are updated > not to reference them. Looks fine. - Marked as reviewed by bchristi (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14367#pullrequestreview-1468750248
Re: RFR: 8308167: SequencedMap::firstEntry throws NPE when first entry has null key or value
On Fri, 2 Jun 2023 01:12:32 GMT, Stuart Marks wrote: > Create and use new NullableKeyValueHolder class to accommodate map entries > whose key or value might be null. Changes look good. An observation: `TreeMap` implements `SequencedMap`, and I see that its `firstEntry()` and related methods use `AbstractMap.SimpleImmutableEntry` (via `exportEntry()`), despite it being serializable. However, this is long-standing code (from 1.6, perhaps?), and `TreeMap` is itself serializable. So, leaving it as is seems the right thing to do. src/java.base/share/classes/jdk/internal/util/NullableKeyValueHolder.java line 65: > 63: * Map.of might still need to reject nulls, and so would Map.ofEntries) > but allowing > 64: * a Map.Entry itself to contain nulls seems beneficial in general. If > this is done, > 65: * merging KeyValueHolder and NullableKeyValueHolder should be > reconsidered. I think having this background and explanation in the docs for this internal class is fine. IMO, this information would also be useful to have in the bug report. - Marked as reviewed by bchristi (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14278#pullrequestreview-1463441861 PR Review Comment: https://git.openjdk.org/jdk/pull/14278#discussion_r1218591121
