Re: RFR: 8336384: AbstractQueuedSynchronizer.acquire should cancel acquire when failing due to a LinkageError or other errors [v2]
On Mon, 12 Aug 2024 16:11:46 GMT, Viktor Klang wrote: >> 8336384: AbstractQueuedSynchronizer.acquire should cancel acquire when >> failing due to a LinkageError or other errors > > Viktor Klang has updated the pull request incrementally with one additional > commit since the last revision: > > Catching both Error and RuntimeException Marked as reviewed by alanb (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20548#pullrequestreview-2234753657
Re: RFR: 8336384: AbstractQueuedSynchronizer.acquire should cancel acquire when failing due to a LinkageError or other errors [v2]
On Mon, 12 Aug 2024 23:28:26 GMT, David Holmes wrote: > Though I'm unclear how that compiles without the method declaring `throws > Throwable` ?? It wouldn't need that because of precise rethrow. In any case, having Error and RuntimeException are okay. - PR Review Comment: https://git.openjdk.org/jdk/pull/20548#discussion_r1714818621
Re: RFR: 8336384: AbstractQueuedSynchronizer.acquire should cancel acquire when failing due to a LinkageError or other errors [v2]
On Mon, 12 Aug 2024 23:39:02 GMT, David Holmes wrote: > It has been a while since I knew this code reasonably well so perhaps I have > just forgotten this difference between AQS and built-in monitors, but it > seems that a Condition.await can return by throwing an exception without > re-acquiring the associated synchronizer. Or is that handled at a > higher-level? The semantics are the same as monitor wait/notify so Condition.await must guarantee to hold the lock when it returns. If ConditionNode.block were to throw something like StackOverflowError then there would be an issue (it's a different park to the one changed in this PR but I think you do have a good point). - PR Comment: https://git.openjdk.org/jdk/pull/20548#issuecomment-2285609879
Re: RFR: 8336856: Efficient hidden class-based string concatenation strategy [v48]
On Tue, 13 Aug 2024 01:39:33 GMT, Shaojin Wen wrote:
>> This PR implements the same algorithm as the current generateMHInlineCopy
>> based on bytecode to improve startup performance.
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> add jtreg HiddenClassUnloading
Thanks for adding the test! Looks pretty good, but could perhaps be simplified
a bit and run fewer iterations with the same result.
Marking it as `@require vm.flagless` might avoid uninteresting test failures on
various exotic VM configurations that higher test tiers might otherwise try
out.
(Also found a few more suggestions to the code at large)
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1226:
> 1224: cl = String.class;
> 1225: }
> 1226: paramTypes[i + 1] = ConstantUtils.classDesc(cl);
Suggestion:
paramTypes[i + 1] = needString(cl) ? CD_String :
ConstantUtils.classDesc(cl);
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1269:
> 1267: clb.withSuperclass(CD_StringConcatBase)
> 1268: .withFlags(ACC_FINAL | ACC_SUPER |
> ACC_SYNTHETIC)
> 1269: .withMethodBody(INIT_NAME, MTD_INIT,
> ACC_PROTECTED, CONSTRUCTOR_BUILDER)
Suggestion:
.withMethodBody(INIT_NAME, MTD_INIT, 0,
CONSTRUCTOR_BUILDER)
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1413:
> 1411: int paramCount = concatArgs.parameterCount();
> 1412: int thisSlot= cb.receiverSlot();
> 1413: int[] stringSlots = new int[paramCount];
This array and the following loop strictly isn't needed: you can allocate the
string slots just before astore, then derive those slots again in the two loops
doing aload. They'll always start from the same slot and be in the same order.
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1417:
> 1415: var cl = concatArgs.parameterType(i);
> 1416: if (needStringOf(cl)) {
> 1417: stringSlots[i] =
> cb.allocateLocal(TypeKind.from(String.class));
Suggestion:
stringSlots[i] =
cb.allocateLocal(TypeKind.ReferenceType);
test/jdk/java/lang/String/concat/HiddenClassUnloading.java line 32:
> 30: * @test
> 31: * @summary Test whether the hidden class unloading of
> StringConcatFactory works
> 32: *
Suggestion:
*
* @requires vm.flagless
I suggest we add this (slightly controversial) flag which locks down so that
testing won't test this over and over at higher tiers with all manner of VM
flags that this test might fail at.
test/jdk/java/lang/String/concat/HiddenClassUnloading.java line 67:
> 65: new Object[0]
> 66: );
> 67: MethodHandle mh = callSite.dynamicInvoker();
Actually invoking the concats seem unnecessary for this test; even with the
rest of this method removed many thousands of classes is unloaded. We also seem
to do pretty well with fewer iterations in the outer loop.
test/jdk/java/lang/String/concat/HiddenClassUnloading.java line 89:
> 87:
> 88: long unloadedClassCount =
> ManagementFactory.getClassLoadingMXBean().getUnloadedClassCount();
> 89: if (unloadedClassCount == 0) {
Sample `getUnloadedClassCount()` before going into the loop so that we check
that there's progress.
-
PR Review: https://git.openjdk.org/jdk/pull/20273#pullrequestreview-2234482233
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1714815159
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1714657417
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1714657078
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1714654215
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1714870451
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1714854631
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1714857823
Re: RFR: 8336856: Efficient hidden class-based string concatenation strategy [v49]
> This PR implements the same algorithm as the current generateMHInlineCopy > based on bytecode to improve startup performance. Shaojin Wen has updated the pull request incrementally with four additional commits since the last revision: - Update src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java Co-authored-by: Claes Redestad - Update src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java Co-authored-by: Claes Redestad - Update test/jdk/java/lang/String/concat/HiddenClassUnloading.java Co-authored-by: Claes Redestad - Update src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java Co-authored-by: Claes Redestad - Changes: - all: https://git.openjdk.org/jdk/pull/20273/files - new: https://git.openjdk.org/jdk/pull/20273/files/68506248..32334915 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20273&range=48 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20273&range=47-48 Stats: 7 lines in 2 files changed: 1 ins; 3 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/20273.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20273/head:pull/20273 PR: https://git.openjdk.org/jdk/pull/20273
Re: RFR: 8336856: Efficient hidden class-based string concatenation strategy [v49]
On Tue, 13 Aug 2024 08:36:18 GMT, Shaojin Wen wrote:
>> This PR implements the same algorithm as the current generateMHInlineCopy
>> based on bytecode to improve startup performance.
>
> Shaojin Wen has updated the pull request incrementally with four additional
> commits since the last revision:
>
> - Update
> src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java
>
>Co-authored-by: Claes Redestad
> - Update
> src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java
>
>Co-authored-by: Claes Redestad
> - Update test/jdk/java/lang/String/concat/HiddenClassUnloading.java
>
>Co-authored-by: Claes Redestad
> - Update
> src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java
>
>Co-authored-by: Claes Redestad
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1191:
> 1189: cl = String.class;
> 1190: }
> 1191: paramTypes[i + 4] = ConstantUtils.classDesc(cl);
Suggestion:
paramTypes[i + 4] = needStringOf(cl) ? CD_String :
ConstantUtils.classDesc(cl);
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1223:
> 1221: for (int i = 0; i < parameterCount; i++) {
> 1222: var cl = concatArgs.parameterType(i);
> 1223: paramTypes[i + 1] = needString(cl) ? CD_String :
> ConstantUtils.classDesc(cl);
Suggestion:
paramTypes[i + 1] = needStringOf(cl) ? CD_String :
ConstantUtils.classDesc(cl);
Typo in my previous suggestion
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1714913889
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1714912293
Re: RFR: 8336856: Efficient hidden class-based string concatenation strategy [v50]
> This PR implements the same algorithm as the current generateMHInlineCopy > based on bytecode to improve startup performance. Shaojin Wen has updated the pull request incrementally with three additional commits since the last revision: - Simplify HiddenClassUnloading - Remove unnecessary loops - fix build error - Changes: - all: https://git.openjdk.org/jdk/pull/20273/files - new: https://git.openjdk.org/jdk/pull/20273/files/32334915..babc76e1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20273&range=49 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20273&range=48-49 Stats: 51 lines in 2 files changed: 4 ins; 35 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/20273.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20273/head:pull/20273 PR: https://git.openjdk.org/jdk/pull/20273
Re: RFR: 8336856: Efficient hidden class-based string concatenation strategy [v48]
On Tue, 13 Aug 2024 08:04:07 GMT, Claes Redestad wrote: >> Shaojin Wen has updated the pull request incrementally with one additional >> commit since the last revision: >> >> add jtreg HiddenClassUnloading > > test/jdk/java/lang/String/concat/HiddenClassUnloading.java line 67: > >> 65: new Object[0] >> 66: ); >> 67: MethodHandle mh = callSite.dynamicInvoker(); > > Actually invoking the concats seem unnecessary for this test; even with the > rest of this method removed many thousands of classes is unloaded. We also > seem to do pretty well with fewer iterations in the outer loop. Yes, this is not necessary, it is there to check correctness, but it is not what this test is about, so I have removed it. - PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1714924871
Re: RFR: 8336856: Efficient hidden class-based string concatenation strategy [v51]
> This PR implements the same algorithm as the current generateMHInlineCopy > based on bytecode to improve startup performance. Shaojin Wen has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java Co-authored-by: Claes Redestad - Changes: - all: https://git.openjdk.org/jdk/pull/20273/files - new: https://git.openjdk.org/jdk/pull/20273/files/babc76e1..14205596 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20273&range=50 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20273&range=49-50 Stats: 4 lines in 1 file changed: 0 ins; 3 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/20273.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20273/head:pull/20273 PR: https://git.openjdk.org/jdk/pull/20273
Re: RFR: 8336856: Efficient hidden class-based string concatenation strategy [v52]
> This PR implements the same algorithm as the current generateMHInlineCopy > based on bytecode to improve startup performance. Shaojin Wen has updated the pull request incrementally with one additional commit since the last revision: Simplify HiddenClassUnloading - Changes: - all: https://git.openjdk.org/jdk/pull/20273/files - new: https://git.openjdk.org/jdk/pull/20273/files/14205596..e4f49c5e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20273&range=51 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20273&range=50-51 Stats: 11 lines in 1 file changed: 0 ins; 11 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/20273.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20273/head:pull/20273 PR: https://git.openjdk.org/jdk/pull/20273
Re: RFR: 8336856: Efficient hidden class-based string concatenation strategy [v51]
On Tue, 13 Aug 2024 09:00:10 GMT, Shaojin Wen wrote:
>> This PR implements the same algorithm as the current generateMHInlineCopy
>> based on bytecode to improve startup performance.
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java
>
> Co-authored-by: Claes Redestad
test/jdk/java/lang/String/concat/HiddenClassUnloading.java line 70:
> 68: }
> 69:
> 70: static void assertEquals(String expected, String actual) {
unused
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1714934816
Re: RFR: 8336856: Efficient hidden class-based string concatenation strategy [v53]
> This PR implements the same algorithm as the current generateMHInlineCopy > based on bytecode to improve startup performance. Shaojin Wen has updated the pull request incrementally with one additional commit since the last revision: Minor refactoring - Changes: - all: https://git.openjdk.org/jdk/pull/20273/files - new: https://git.openjdk.org/jdk/pull/20273/files/e4f49c5e..62dd2ce7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20273&range=52 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20273&range=51-52 Stats: 9 lines in 1 file changed: 2 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/20273.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20273/head:pull/20273 PR: https://git.openjdk.org/jdk/pull/20273
Re: RFR: 8336856: Efficient hidden class-based string concatenation strategy [v51]
On Tue, 13 Aug 2024 09:00:10 GMT, Shaojin Wen wrote:
>> This PR implements the same algorithm as the current generateMHInlineCopy
>> based on bytecode to improve startup performance.
>
> Shaojin Wen has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java
>
> Co-authored-by: Claes Redestad
src/java.base/share/classes/java/lang/StringConcatHelper.java line 121:
> 119: @ForceInline
> 120: String concat(Object value) {
> 121: String str = stringOf(value);
Redundant (remove, or replace `stringOf(value)` on next line with `str`)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1714962672
Re: RFR: 8336856: Efficient hidden class-based string concatenation strategy [v54]
> This PR implements the same algorithm as the current generateMHInlineCopy > based on bytecode to improve startup performance. Shaojin Wen has updated the pull request incrementally with two additional commits since the last revision: - remove unused - Minor refactoring - Changes: - all: https://git.openjdk.org/jdk/pull/20273/files - new: https://git.openjdk.org/jdk/pull/20273/files/62dd2ce7..094b2e54 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20273&range=53 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20273&range=52-53 Stats: 6 lines in 2 files changed: 0 ins; 1 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/20273.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20273/head:pull/20273 PR: https://git.openjdk.org/jdk/pull/20273
Integrated: 8336384: AbstractQueuedSynchronizer.acquire should cancel acquire when failing due to a LinkageError or other errors
On Mon, 12 Aug 2024 13:41:24 GMT, Viktor Klang wrote: > 8336384: AbstractQueuedSynchronizer.acquire should cancel acquire when > failing due to a LinkageError or other errors This pull request has now been integrated. Changeset: fbe4cc96 Author:Viktor Klang URL: https://git.openjdk.org/jdk/commit/fbe4cc96e223882a18c7ff666fe6f68b3fa2cfe4 Stats: 26 lines in 2 files changed: 12 ins; 2 del; 12 mod 8336384: AbstractQueuedSynchronizer.acquire should cancel acquire when failing due to a LinkageError or other errors Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/20548
Re: RFR: 8338146: Improve Exchanger performance with VirtualThreads
On Mon, 12 Aug 2024 17:07:42 GMT, Doug Lea wrote:
> The Exchanger class uses spin-waits that are hostile to some uses of
> VirtualThreads. Improving this requires a means of estimating whether there
> are many VirtualThreads with few carriers, which can be supported by adding a
> method in class ForkJoinWorkerThread. This enables a reworking of the
> exchange method, and can also be used to deal with similar issues in
> LinkedTransferQueue and possibly elsewhere. We leave for now open whether
> this method (hasKnownQueuedWork) should be public, which would allow users to
> use it in similar contexts, at the possible expense of revealing too much
> about current VT implementation
src/java.base/share/classes/java/util/concurrent/Exchanger.java line 379:
> 377: if ((v = p.match) != null) {
> 378: MATCH.set(p, null);
> 379: break outer; // spin wait
Is this comment accurate?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20554#discussion_r1715164058
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v9]
On Tue, 16 Jul 2024 12:36:08 GMT, Roman Kennke wrote:
>> Axel Boldt-Christmas has updated the pull request incrementally with 10
>> additional commits since the last revision:
>>
>> - Remove try_read
>> - Add explicit to single parameter constructors
>> - Remove superfluous access specifier
>> - Remove unused include
>> - Update assert message OMCache::set_monitor
>> - Fix indentation
>> - Remove outdated comment LightweightSynchronizer::exit
>> - Remove logStream include
>> - Remove strange comment
>> - Fix javaThread include
>
> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 674:
>
>> 672:
>> 673: // Search for obj in cache.
>> 674: bind(loop);
>
> Same loop transformation would be possible here.
I tried the following (see diff below) and it shows about a 5-10% regression in
most the `LockUnlock.testInflated*` micros. Also tried with just `num_unrolled
= 1` saw the same regression. Maybe there was some other pattern you were
thinking of. There are probably architecture and platform differences. This can
and should probably be explored in a followup PR.
diff --git a/src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp
b/src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp
index 5dbfdbc225d..4e6621cfece 100644
--- a/src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp
+++ b/src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp
@@ -663,25 +663,28 @@ void C2_MacroAssembler::fast_lock_lightweight(Register
obj, Register box, Regist
const int num_unrolled = 2;
for (int i = 0; i < num_unrolled; i++) {
-cmpptr(obj, Address(t));
-jccb(Assembler::equal, monitor_found);
-increment(t, in_bytes(OMCache::oop_to_oop_difference()));
+Label next;
+cmpptr(obj, Address(t, OMCache::oop_to_oop_difference() * i));
+jccb(Assembler::notEqual, next);
+increment(t, in_bytes(OMCache::oop_to_oop_difference() * i));
+jmpb(monitor_found);
+bind(next);
}
+ increment(t, in_bytes(OMCache::oop_to_oop_difference() * (num_unrolled -
1)));
Label loop;
// Search for obj in cache.
bind(loop);
-
- // Check for match.
- cmpptr(obj, Address(t));
- jccb(Assembler::equal, monitor_found);
-
+ // Advance.
+ increment(t, in_bytes(OMCache::oop_to_oop_difference()));
// Search until null encountered, guaranteed _null_sentinel at end.
cmpptr(Address(t), 1);
jcc(Assembler::below, slow_path); // 0 check, but with ZF=0 when *t == 0
- increment(t, in_bytes(OMCache::oop_to_oop_difference()));
- jmpb(loop);
+
+ // Check for match.
+ cmpptr(obj, Address(t));
+ jccb(Assembler::notEqual, loop);
// Cache hit.
bind(monitor_found);
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715249312
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v12]
> When inflating a monitor the `ObjectMonitor*` is written directly over the > `markWord` and any overwritten data is displaced into a displaced `markWord`. > This is problematic for concurrent GCs which needs extra care or looser > semantics to use this displaced data. In Lilliput this data also contains the > klass forcing this to be something that the GC has to take into account > everywhere. > > This patch introduces an alternative solution where locking only uses the > lock bits of the `markWord` and inflation does not override and displace the > `markWord`. This is done by keeping associations between objects and > `ObjectMonitor*` in an external hash table. Different caching techniques are > used to speedup lookups from compiled code. > > A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is > only supported in combination with the LM_LIGHTWEIGHT locking mode (the > default). > > This patch has been evaluated to be performance neutral when > `UseObjectMonitorTable` is turned off (the default). > > Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT` > and `UseObjectMonitorTable` works. > > # Cleanups > > Cleaned up displaced header usage for: > * BasicLock > * Contains some Zero changes > * Renames one exported JVMCI field > * ObjectMonitor > * Updates comments and tests consistencies > > # Refactoring > > `ObjectMonitor::enter` has been refactored an a `ObjectMonitorContentionMark` > witness object has been introduced to the signatures. Which signals that the > contentions reference counter is being held. More details are given below in > the section about deflation. > > The initial purpose of this was to allow `UseObjectMonitorTable` to interact > more seamlessly with the `ObjectMonitor::enter` code. > > _There is even more `ObjectMonitor` refactoring which can be done here to > create a more understandable and enforceable API. There are a handful of > invariants / assumptions which are not always explicitly asserted which could > be trivially abstracted and verified by the type system by using similar > witness objects._ > > # LightweightSynchronizer > > Working on adapting and incorporating the following section as a comment in > the source code > > ## Fast Locking > > CAS on locking bits in markWord. > 0b00 (Fast Locked) <--> 0b01 (Unlocked) > > When locking and 0b00 (Fast Locked) is observed, it may be beneficial to > avoid inflating by spinning a bit. > > If 0b10 (Inflated) is observed or there is to much contention or to long > critical sections for spinning to be feasible, inf... Axel Boldt-Christmas has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 39 commits: - Merge remote-tracking branch 'upstream_jdk/master' into JDK-8315884 - Remove top comment - Missing DEBUG_ONLY - Remove extra whitespace in UseObjectMonitorTableTest.java - Inline _table - Rename ObjectMonitorWorld to ObjectMonitorTable - Update comment basicLock.hpp - Remove const for InflateCause parameters in lightweightSynchronizer - Use [inc/dec]_no_safepoint_count directly instead of a conditionally created NoSafepointVerifier - Remove unnecessary assert - ... and 29 more: https://git.openjdk.org/jdk/compare/76e33b6c...b96b916a - Changes: https://git.openjdk.org/jdk/pull/20067/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20067&range=11 Stats: 3612 lines in 69 files changed: 2699 ins; 314 del; 599 mod Patch: https://git.openjdk.org/jdk/pull/20067.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20067/head:pull/20067 PR: https://git.openjdk.org/jdk/pull/20067
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v6]
On Fri, 12 Jul 2024 15:32:45 GMT, Roman Kennke wrote: >> Axel Boldt-Christmas has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update arguments.cpp > > src/hotspot/share/runtime/lightweightSynchronizer.cpp line 58: > >> 56: >> 57: // >> 58: // Lightweight synchronization. > > This comment doesn't really say anything. Either remove it, or add a nice > summary of how LW locking and OM table stuff works. Removed it. - PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715291832
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v9]
On Tue, 23 Jul 2024 20:21:05 GMT, Coleen Phillimore wrote: >> Only legacy locking uses the displaced header, I believe, which isn't clear >> in this code at all. This seems like a fix. We should probably assert that >> only legacy locking uses this field as a displaced header. > > Update: yes, this code change does assert if you use BasicLock's displaced > header for locking modes other than LM_LEGACY. This is correct. The `displaced_header` in the BasicLock is only used by Legacy. Which is more strongly asserted now. - PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715299608
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v9]
On Tue, 23 Jul 2024 13:19:02 GMT, Coleen Phillimore wrote:
>> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 62:
>>
>>> 60: class ObjectMonitorWorld : public CHeapObj {
>>> 61: struct Config {
>>> 62: using Value = ObjectMonitor*;
>>
>> Does this alias really help? We don't state the type that many times and it
>> looks odd to end up with a mix of `Value` and `ObjectMonitor*` in the same
>> code.
>
> This alias is present in the other CHT implementations, alas as a typedef in
> StringTable and SymbolTable so this follows the pattern and allows cut/paste
> of the allocate_node, get_hash, and other functions.
It is required by the `ConcurrentHashTable` implementation.
https://github.com/openjdk/jdk/blob/ebf1154292aa5e78c9eb9ddb26a1a3f9885c2ef8/src/hotspot/share/utilities/concurrentHashTable.hpp#L43-L45
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715304376
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v9]
On Tue, 23 Jul 2024 16:36:18 GMT, Coleen Phillimore wrote: >> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 102: >> >>> 100: assert(*value != nullptr, "must be"); >>> 101: return (*value)->object_is_cleared(); >>> 102: } >> >> The `is_dead` functions seem oddly placed given they do not relate to the >> object stored in the wrapper. Why are they here? And what is the difference >> between `object_is_cleared` and `object_is_dead` (as used by >> `LookupMonitor`) ? > > This is a good question. When we look up the Monitor, we don't want to find > any that the GC has marked dead, so that's why we call object_is_dead. When > we look up with the object to find the Monitor, the object won't be dead > (since we're using it to look up). But we don't want to find one that we've > cleared because the Monitor was deflated? I don't see where we would clear > it though. We clear the WeakHandle in the destructor after the Monitor has > been removed from the table. What @coleenp said is correct. One is just a null check, the other interacts with the GC and the objects lifecycle. But as also mentioned we do not ever clear these any WeakHandle, so this is currently always returning false. (At least from what I can see). This load should be cached or in a register because it always load this to check if it is the value when doing a lookup. So there should be no performance cost here, but it is unnecessary. I'll remove this. The `object_is_dead` is only used when removing, where we can take the extra cost. - PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715347334
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v9]
On Tue, 23 Jul 2024 14:27:34 GMT, Coleen Phillimore wrote:
>> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 105:
>>
>>> 103: };
>>> 104:
>>> 105: class LookupMonitor : public StackObj {
>>
>> I'm not understanding why we need this little wrapper class.
>
> It's a two way lookup. The plain Lookup class is used to lookup the Monitor
> given the object. This LookupMonitor class is used to lookup the object
> given the Monitor. The CHT takes these wrapper classes. Maybe we should
> rename LookupObject to be more clear?
As @coleenp said there are two lookups. One when you have the object and want
to get or insert a new ObjectMonitor. Or the second when the deflator has an
ObjectMonitor and wants to deflate and remove that object - ObjectMonitor
association.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715356203
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v13]
> When inflating a monitor the `ObjectMonitor*` is written directly over the > `markWord` and any overwritten data is displaced into a displaced `markWord`. > This is problematic for concurrent GCs which needs extra care or looser > semantics to use this displaced data. In Lilliput this data also contains the > klass forcing this to be something that the GC has to take into account > everywhere. > > This patch introduces an alternative solution where locking only uses the > lock bits of the `markWord` and inflation does not override and displace the > `markWord`. This is done by keeping associations between objects and > `ObjectMonitor*` in an external hash table. Different caching techniques are > used to speedup lookups from compiled code. > > A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is > only supported in combination with the LM_LIGHTWEIGHT locking mode (the > default). > > This patch has been evaluated to be performance neutral when > `UseObjectMonitorTable` is turned off (the default). > > Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT` > and `UseObjectMonitorTable` works. > > # Cleanups > > Cleaned up displaced header usage for: > * BasicLock > * Contains some Zero changes > * Renames one exported JVMCI field > * ObjectMonitor > * Updates comments and tests consistencies > > # Refactoring > > `ObjectMonitor::enter` has been refactored an a `ObjectMonitorContentionMark` > witness object has been introduced to the signatures. Which signals that the > contentions reference counter is being held. More details are given below in > the section about deflation. > > The initial purpose of this was to allow `UseObjectMonitorTable` to interact > more seamlessly with the `ObjectMonitor::enter` code. > > _There is even more `ObjectMonitor` refactoring which can be done here to > create a more understandable and enforceable API. There are a handful of > invariants / assumptions which are not always explicitly asserted which could > be trivially abstracted and verified by the type system by using similar > witness objects._ > > # LightweightSynchronizer > > Working on adapting and incorporating the following section as a comment in > the source code > > ## Fast Locking > > CAS on locking bits in markWord. > 0b00 (Fast Locked) <--> 0b01 (Unlocked) > > When locking and 0b00 (Fast Locked) is observed, it may be beneficial to > avoid inflating by spinning a bit. > > If 0b10 (Inflated) is observed or there is to much contention or to long > critical sections for spinning to be feasible, inf... Axel Boldt-Christmas has updated the pull request incrementally with one additional commit since the last revision: Remove object_is_cleared - Changes: - all: https://git.openjdk.org/jdk/pull/20067/files - new: https://git.openjdk.org/jdk/pull/20067/files/b96b916a..53f833bc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20067&range=12 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20067&range=11-12 Stats: 6 lines in 3 files changed: 0 ins; 5 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/20067.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20067/head:pull/20067 PR: https://git.openjdk.org/jdk/pull/20067
Re: RFR: 8338146: Improve Exchanger performance with VirtualThreads
On Tue, 13 Aug 2024 11:52:05 GMT, Viktor Klang wrote:
>> The Exchanger class uses spin-waits that are hostile to some uses of
>> VirtualThreads. Improving this requires a means of estimating whether there
>> are many VirtualThreads with few carriers, which can be supported by adding
>> a method in class ForkJoinWorkerThread. This enables a reworking of the
>> exchange method, and can also be used to deal with similar issues in
>> LinkedTransferQueue and possibly elsewhere. We leave for now open whether
>> this method (hasKnownQueuedWork) should be public, which would allow users
>> to use it in similar contexts, at the possible expense of revealing too much
>> about current VT implementation
>
> src/java.base/share/classes/java/util/concurrent/Exchanger.java line 379:
>
>> 377: if ((v = p.match) != null) {
>> 378: MATCH.set(p, null);
>> 379: break outer; // spin wait
>
> Is this comment accurate?
No. Carried over by mistake in a paste. Thanks. Removed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20554#discussion_r1715375622
Re: RFR: 8338014: Improve usage of @jvms tags in class file API [v2]
> Hi all, > > This PR addresses [8338014](https://bugs.openjdk.org/browse/JDK-8338014) > improving the use of `@jvms` tags by adding `JVMS` prior to the tag. > > Thanks, > Sonia Sonia Zaldana Calles has updated the pull request incrementally with two additional commits since the last revision: - Merge branch 'JDK-8338014' of github.com:SoniaZaldana/jdk into JDK-8338014 - Adding parentheses to references which are not part of sentences and removing redundant additions - Changes: - all: https://git.openjdk.org/jdk/pull/20513/files - new: https://git.openjdk.org/jdk/pull/20513/files/099f2b81..4a709416 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20513&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20513&range=00-01 Stats: 36 lines in 35 files changed: 0 ins; 0 del; 36 mod Patch: https://git.openjdk.org/jdk/pull/20513.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20513/head:pull/20513 PR: https://git.openjdk.org/jdk/pull/20513
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v14]
> When inflating a monitor the `ObjectMonitor*` is written directly over the > `markWord` and any overwritten data is displaced into a displaced `markWord`. > This is problematic for concurrent GCs which needs extra care or looser > semantics to use this displaced data. In Lilliput this data also contains the > klass forcing this to be something that the GC has to take into account > everywhere. > > This patch introduces an alternative solution where locking only uses the > lock bits of the `markWord` and inflation does not override and displace the > `markWord`. This is done by keeping associations between objects and > `ObjectMonitor*` in an external hash table. Different caching techniques are > used to speedup lookups from compiled code. > > A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is > only supported in combination with the LM_LIGHTWEIGHT locking mode (the > default). > > This patch has been evaluated to be performance neutral when > `UseObjectMonitorTable` is turned off (the default). > > Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT` > and `UseObjectMonitorTable` works. > > # Cleanups > > Cleaned up displaced header usage for: > * BasicLock > * Contains some Zero changes > * Renames one exported JVMCI field > * ObjectMonitor > * Updates comments and tests consistencies > > # Refactoring > > `ObjectMonitor::enter` has been refactored an a `ObjectMonitorContentionMark` > witness object has been introduced to the signatures. Which signals that the > contentions reference counter is being held. More details are given below in > the section about deflation. > > The initial purpose of this was to allow `UseObjectMonitorTable` to interact > more seamlessly with the `ObjectMonitor::enter` code. > > _There is even more `ObjectMonitor` refactoring which can be done here to > create a more understandable and enforceable API. There are a handful of > invariants / assumptions which are not always explicitly asserted which could > be trivially abstracted and verified by the type system by using similar > witness objects._ > > # LightweightSynchronizer > > Working on adapting and incorporating the following section as a comment in > the source code > > ## Fast Locking > > CAS on locking bits in markWord. > 0b00 (Fast Locked) <--> 0b01 (Unlocked) > > When locking and 0b00 (Fast Locked) is observed, it may be beneficial to > avoid inflating by spinning a bit. > > If 0b10 (Inflated) is observed or there is to much contention or to long > critical sections for spinning to be feasible, inf... Axel Boldt-Christmas has updated the pull request incrementally with three additional commits since the last revision: - Remove _omworld - Make ObjectMonitorTable AllStatic - Revert "Inline _table" This reverts commit 937f531364bb7025998d3a80f9ea93cbc8c9650f. - Changes: - all: https://git.openjdk.org/jdk/pull/20067/files - new: https://git.openjdk.org/jdk/pull/20067/files/53f833bc..123a2683 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20067&range=13 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20067&range=12-13 Stats: 64 lines in 3 files changed: 4 ins; 5 del; 55 mod Patch: https://git.openjdk.org/jdk/pull/20067.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20067/head:pull/20067 PR: https://git.openjdk.org/jdk/pull/20067
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v15]
> When inflating a monitor the `ObjectMonitor*` is written directly over the > `markWord` and any overwritten data is displaced into a displaced `markWord`. > This is problematic for concurrent GCs which needs extra care or looser > semantics to use this displaced data. In Lilliput this data also contains the > klass forcing this to be something that the GC has to take into account > everywhere. > > This patch introduces an alternative solution where locking only uses the > lock bits of the `markWord` and inflation does not override and displace the > `markWord`. This is done by keeping associations between objects and > `ObjectMonitor*` in an external hash table. Different caching techniques are > used to speedup lookups from compiled code. > > A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is > only supported in combination with the LM_LIGHTWEIGHT locking mode (the > default). > > This patch has been evaluated to be performance neutral when > `UseObjectMonitorTable` is turned off (the default). > > Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT` > and `UseObjectMonitorTable` works. > > # Cleanups > > Cleaned up displaced header usage for: > * BasicLock > * Contains some Zero changes > * Renames one exported JVMCI field > * ObjectMonitor > * Updates comments and tests consistencies > > # Refactoring > > `ObjectMonitor::enter` has been refactored an a `ObjectMonitorContentionMark` > witness object has been introduced to the signatures. Which signals that the > contentions reference counter is being held. More details are given below in > the section about deflation. > > The initial purpose of this was to allow `UseObjectMonitorTable` to interact > more seamlessly with the `ObjectMonitor::enter` code. > > _There is even more `ObjectMonitor` refactoring which can be done here to > create a more understandable and enforceable API. There are a handful of > invariants / assumptions which are not always explicitly asserted which could > be trivially abstracted and verified by the type system by using similar > witness objects._ > > # LightweightSynchronizer > > Working on adapting and incorporating the following section as a comment in > the source code > > ## Fast Locking > > CAS on locking bits in markWord. > 0b00 (Fast Locked) <--> 0b01 (Unlocked) > > When locking and 0b00 (Fast Locked) is observed, it may be beneficial to > avoid inflating by spinning a bit. > > If 0b10 (Inflated) is observed or there is to much contention or to long > critical sections for spinning to be feasible, inf... Axel Boldt-Christmas has updated the pull request incrementally with two additional commits since the last revision: - Remove the last OMWorld references - Rename omworldtable_work to object_monitor_table_work - Changes: - all: https://git.openjdk.org/jdk/pull/20067/files - new: https://git.openjdk.org/jdk/pull/20067/files/123a2683..7946d148 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20067&range=14 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20067&range=13-14 Stats: 6 lines in 2 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/20067.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20067/head:pull/20067 PR: https://git.openjdk.org/jdk/pull/20067
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v11]
On Mon, 12 Aug 2024 18:55:41 GMT, Coleen Phillimore wrote: >> Axel Boldt-Christmas has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Missing DEBUG_ONLY > > src/hotspot/share/runtime/lightweightSynchronizer.cpp line 341: > >> 339: }; >> 340: >> 341: ObjectMonitorTable* LightweightSynchronizer::_omworld = nullptr; > > I preferred my version where ObjectMonitorTable was AllStatic which gets rid > of _omworld-> everwhere, but the internal table is the pointer. You can > remove more omworld names also. I implemented the AllStatic table, and remove all mentions of OMWorld. - PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715432994
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v9]
On Mon, 12 Aug 2024 14:37:50 GMT, Axel Boldt-Christmas wrote: >> This is a really good suggestion and might help a lot with the performance >> problems that we see with the table with heavily contended locking. I think >> we should change this in a follow-on patch (which I'll work on). > > I inlined the table in the surrounding object as it is a trivial change. > > Removing both indirections and creating static storage I would require more > work (some conditional deferred creation, similar to an optional). Sadly doesn't help with performance. - PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715443138
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v15]
On Tue, 13 Aug 2024 14:52:03 GMT, Axel Boldt-Christmas
wrote:
>> When inflating a monitor the `ObjectMonitor*` is written directly over the
>> `markWord` and any overwritten data is displaced into a displaced
>> `markWord`. This is problematic for concurrent GCs which needs extra care or
>> looser semantics to use this displaced data. In Lilliput this data also
>> contains the klass forcing this to be something that the GC has to take into
>> account everywhere.
>>
>> This patch introduces an alternative solution where locking only uses the
>> lock bits of the `markWord` and inflation does not override and displace the
>> `markWord`. This is done by keeping associations between objects and
>> `ObjectMonitor*` in an external hash table. Different caching techniques are
>> used to speedup lookups from compiled code.
>>
>> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is
>> only supported in combination with the LM_LIGHTWEIGHT locking mode (the
>> default).
>>
>> This patch has been evaluated to be performance neutral when
>> `UseObjectMonitorTable` is turned off (the default).
>>
>> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT`
>> and `UseObjectMonitorTable` works.
>>
>> # Cleanups
>>
>> Cleaned up displaced header usage for:
>> * BasicLock
>> * Contains some Zero changes
>> * Renames one exported JVMCI field
>> * ObjectMonitor
>> * Updates comments and tests consistencies
>>
>> # Refactoring
>>
>> `ObjectMonitor::enter` has been refactored an a
>> `ObjectMonitorContentionMark` witness object has been introduced to the
>> signatures. Which signals that the contentions reference counter is being
>> held. More details are given below in the section about deflation.
>>
>> The initial purpose of this was to allow `UseObjectMonitorTable` to interact
>> more seamlessly with the `ObjectMonitor::enter` code.
>>
>> _There is even more `ObjectMonitor` refactoring which can be done here to
>> create a more understandable and enforceable API. There are a handful of
>> invariants / assumptions which are not always explicitly asserted which
>> could be trivially abstracted and verified by the type system by using
>> similar witness objects._
>>
>> # LightweightSynchronizer
>>
>> Working on adapting and incorporating the following section as a comment in
>> the source code
>>
>> ## Fast Locking
>>
>> CAS on locking bits in markWord.
>> 0b00 (Fast Locked) <--> 0b01 (Unlocked)
>>
>> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to
>> avoid inflating by spinning a bit.
>>
>> If 0b10 (Inflated) is observed or there is to...
>
> Axel Boldt-Christmas has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Remove the last OMWorld references
> - Rename omworldtable_work to object_monitor_table_work
Two tiny nits.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 172:
> 170: static void create() {
> 171: _table = new ConcurrentTable(initial_log_size(),
> 172: max_log_size(),
nit, can you line up these parameters?
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 579:
> 577: }
> 578:
> 579:
Extra blank line.
-
Marked as reviewed by coleenp (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/20067#pullrequestreview-2235807775
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715446552
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715449461
Re: RFR: 8336856: Efficient hidden class-based string concatenation strategy [v54]
On Tue, 13 Aug 2024 09:38:31 GMT, Shaojin Wen wrote: >> This PR implements the same algorithm as the current generateMHInlineCopy >> based on bytecode to improve startup performance. > > Shaojin Wen has updated the pull request incrementally with two additional > commits since the last revision: > > - remove unused > - Minor refactoring src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1232: > 1230: // 1 argment use built-in method > 1231: int paramCount = args.parameterCount(); > 1232: var concatClass = ConstantUtils.binaryNameToDesc(className); `String className` and `ClassDesc concatClass` are effectively constant. Refactor to `static final` to save a few instructions. - PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1715473623
Re: RFR: 8335927: Revisit AnnotationConstantValueEntry and AnnotationValue.OfConstant [v5]
On Fri, 2 Aug 2024 16:31:48 GMT, Chen Liang wrote: >> 1. Add notes and docs about the difference between resolved constants and >> constant pool descriptors for annotation constants (e.g. `char` vs >> `IntegerEntry`) >> 2. Improved value specification to specify their tags. >> 3. Improved value factories to return their specific types instead of >> `OfConstant` >> 4. Improved value classes to return specific `PoolEntry` subtypes and >> specific live constant subtypes >> 5. Removed confusing and meaningless >> `ConstantPoolBuilder.annotationConstantValueEntry` > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Fix another failing test Thanks for the reviews! - PR Comment: https://git.openjdk.org/jdk/pull/20436#issuecomment-2286501451
Integrated: 8335927: Revisit AnnotationConstantValueEntry and AnnotationValue.OfConstant
On Fri, 2 Aug 2024 00:39:12 GMT, Chen Liang wrote: > 1. Add notes and docs about the difference between resolved constants and > constant pool descriptors for annotation constants (e.g. `char` vs > `IntegerEntry`) > 2. Improved value specification to specify their tags. > 3. Improved value factories to return their specific types instead of > `OfConstant` > 4. Improved value classes to return specific `PoolEntry` subtypes and > specific live constant subtypes > 5. Removed confusing and meaningless > `ConstantPoolBuilder.annotationConstantValueEntry` This pull request has now been integrated. Changeset: 6af1d6ff Author:Chen Liang URL: https://git.openjdk.org/jdk/commit/6af1d6ff210b3ddbc7d1585428b49631248a500b Stats: 397 lines in 12 files changed: 216 ins; 39 del; 142 mod 8335927: Revisit AnnotationConstantValueEntry and AnnotationValue.OfConstant Reviewed-by: asotona - PR: https://git.openjdk.org/jdk/pull/20436
Re: RFR: 8338014: Improve usage of @jvms tags in class file API [v2]
On Tue, 13 Aug 2024 14:37:32 GMT, Sonia Zaldana Calles wrote: >> Hi all, >> >> This PR addresses [8338014](https://bugs.openjdk.org/browse/JDK-8338014) >> improving the use of `@jvms` tags by adding `JVMS` prior to the tag. >> >> Thanks, >> Sonia > > Sonia Zaldana Calles has updated the pull request incrementally with two > additional commits since the last revision: > > - Merge branch 'JDK-8338014' of github.com:SoniaZaldana/jdk into JDK-8338014 > - Adding parentheses to references which are not part of sentences and > removing redundant additions Thanks for the cleanup. - Marked as reviewed by liach (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20513#pullrequestreview-2235893657
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v9]
On Tue, 13 Aug 2024 12:57:23 GMT, Axel Boldt-Christmas
wrote:
>> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 674:
>>
>>> 672:
>>> 673: // Search for obj in cache.
>>> 674: bind(loop);
>>
>> Same loop transformation would be possible here.
>
> I tried the following (see diff below) and it shows about a 5-10% regression
> in most the `LockUnlock.testInflated*` micros. Also tried with just
> `num_unrolled = 1` saw the same regression. Maybe there was some other
> pattern you were thinking of. There are probably architecture and platform
> differences. This can and should probably be explored in a followup PR.
>
>
>
> diff --git a/src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp
> b/src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp
> index 5dbfdbc225d..4e6621cfece 100644
> --- a/src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp
> +++ b/src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp
> @@ -663,25 +663,28 @@ void C2_MacroAssembler::fast_lock_lightweight(Register
> obj, Register box, Regist
>
>const int num_unrolled = 2;
>for (int i = 0; i < num_unrolled; i++) {
> -cmpptr(obj, Address(t));
> -jccb(Assembler::equal, monitor_found);
> -increment(t, in_bytes(OMCache::oop_to_oop_difference()));
> +Label next;
> +cmpptr(obj, Address(t, OMCache::oop_to_oop_difference() * i));
> +jccb(Assembler::notEqual, next);
> +increment(t, in_bytes(OMCache::oop_to_oop_difference() * i));
> +jmpb(monitor_found);
> +bind(next);
>}
> + increment(t, in_bytes(OMCache::oop_to_oop_difference() * (num_unrolled
> - 1)));
>
>Label loop;
>
>// Search for obj in cache.
>bind(loop);
> -
> - // Check for match.
> - cmpptr(obj, Address(t));
> - jccb(Assembler::equal, monitor_found);
> -
> + // Advance.
> + increment(t, in_bytes(OMCache::oop_to_oop_difference()));
>// Search until null encountered, guaranteed _null_sentinel at end.
>cmpptr(Address(t), 1);
>jcc(Assembler::below, slow_path); // 0 check, but with ZF=0 when *t == > 0
> - increment(t, in_bytes(OMCache::oop_to_oop_difference()));
> - jmpb(loop);
> +
> + // Check for match.
> + cmpptr(obj, Address(t));
> + jccb(Assembler::notEqual, loop);
>
>// Cache hit.
>bind(monitor_found);
Yeah it's probably not very important. But it's not quite what I had in mind, I
was thinking more something like (aarch64 version, untested, may be wrong):
diff --git a/src/hotspot/cpu/aarch64/c2_MacroAssembler_aarch64.cpp
b/src/hotspot/cpu/aarch64/c2_MacroAssembler_aarch64.cpp
index 19af03d3488..05bbb5760b8 100644
--- a/src/hotspot/cpu/aarch64/c2_MacroAssembler_aarch64.cpp
+++ b/src/hotspot/cpu/aarch64/c2_MacroAssembler_aarch64.cpp
@@ -302,14 +302,14 @@ void C2_MacroAssembler::fast_lock_lightweight(Register
obj, Register box, Regist
Label monitor_found;
// Load cache address
- lea(t3_t, Address(rthread, JavaThread::om_cache_oops_offset()));
+ lea(t3_t, Address(rthread, JavaThread::om_cache_oops_offset() -
OMCache::oop_to_oop_difference()));
const int num_unrolled = 2;
for (int i = 0; i < num_unrolled; i++) {
+increment(t3_t, in_bytes(OMCache::oop_to_oop_difference()));
ldr(t1, Address(t3_t));
cmp(obj, t1);
br(Assembler::EQ, monitor_found);
-increment(t3_t, in_bytes(OMCache::oop_to_oop_difference()));
}
Label loop;
@@ -317,16 +317,14 @@ void C2_MacroAssembler::fast_lock_lightweight(Register
obj, Register box, Regist
// Search for obj in cache.
bind(loop);
- // Check for match.
- ldr(t1, Address(t3_t));
- cmp(obj, t1);
- br(Assembler::EQ, monitor_found);
+ increment(t3_t, in_bytes(OMCache::oop_to_oop_difference()));
+ ldr(t1, Address(t3_t));
// Search until null encountered, guaranteed _null_sentinel at end.
- increment(t3_t, in_bytes(OMCache::oop_to_oop_difference()));
- cbnz(t1, loop);
- // Cache Miss, NE set from cmp above, cbnz does not set flags
- b(slow_path);
+ cbz(t1, slow_path);
+ // Check for match.
+ cmp(obj, t1);
+ br(Assembler::NE, loop);
bind(monitor_found);
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715557161
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v16]
> When inflating a monitor the `ObjectMonitor*` is written directly over the > `markWord` and any overwritten data is displaced into a displaced `markWord`. > This is problematic for concurrent GCs which needs extra care or looser > semantics to use this displaced data. In Lilliput this data also contains the > klass forcing this to be something that the GC has to take into account > everywhere. > > This patch introduces an alternative solution where locking only uses the > lock bits of the `markWord` and inflation does not override and displace the > `markWord`. This is done by keeping associations between objects and > `ObjectMonitor*` in an external hash table. Different caching techniques are > used to speedup lookups from compiled code. > > A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is > only supported in combination with the LM_LIGHTWEIGHT locking mode (the > default). > > This patch has been evaluated to be performance neutral when > `UseObjectMonitorTable` is turned off (the default). > > Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT` > and `UseObjectMonitorTable` works. > > # Cleanups > > Cleaned up displaced header usage for: > * BasicLock > * Contains some Zero changes > * Renames one exported JVMCI field > * ObjectMonitor > * Updates comments and tests consistencies > > # Refactoring > > `ObjectMonitor::enter` has been refactored an a `ObjectMonitorContentionMark` > witness object has been introduced to the signatures. Which signals that the > contentions reference counter is being held. More details are given below in > the section about deflation. > > The initial purpose of this was to allow `UseObjectMonitorTable` to interact > more seamlessly with the `ObjectMonitor::enter` code. > > _There is even more `ObjectMonitor` refactoring which can be done here to > create a more understandable and enforceable API. There are a handful of > invariants / assumptions which are not always explicitly asserted which could > be trivially abstracted and verified by the type system by using similar > witness objects._ > > # LightweightSynchronizer > > Working on adapting and incorporating the following section as a comment in > the source code > > ## Fast Locking > > CAS on locking bits in markWord. > 0b00 (Fast Locked) <--> 0b01 (Unlocked) > > When locking and 0b00 (Fast Locked) is observed, it may be beneficial to > avoid inflating by spinning a bit. > > If 0b10 (Inflated) is observed or there is to much contention or to long > critical sections for spinning to be feasible, inf... Axel Boldt-Christmas has updated the pull request incrementally with one additional commit since the last revision: Whitespace and nits - Changes: - all: https://git.openjdk.org/jdk/pull/20067/files - new: https://git.openjdk.org/jdk/pull/20067/files/7946d148..68681c84 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20067&range=15 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20067&range=14-15 Stats: 4 lines in 1 file changed: 0 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/20067.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20067/head:pull/20067 PR: https://git.openjdk.org/jdk/pull/20067
Re: Stream Gatherers (JEP 473) feedback
Hi Viktor Your previous response inspired me to experiment some more with Gatherers As a result I'd like to make a couple propositions: * add an additional type parameter. After doing so, type inference no longer needs any assistance: `var maxGatherer = Gatherer.ofSequential(State::new, State::integrate, State::finish);` * add an identity Gatherer with an optimized `andThen` implementation as well as an optimization in the default implementation of `Gatherer::andThen` * eliminate the use of raw types in `Gatherers.Value` Code that implements these propositions is in this gist: https://gist.github.com/anthonyvdotbe/37c85eaa86a7833051bc33f6fe88046c Kind regards, Anthony July 31, 2024 at 7:58 PM, "Viktor Klang" wrote: > > Hi Anthony, > > >The use case is a time series, which has methods to return a Stream of data > >points, `record DataPoint(Instant, BigDecimal)`. In DataPoint, there are > >several Gatherer factory methods, one of which is `Gatherer >DataPoint> withInterpolationAt(NavigableSet instants)`. If > >`instants.isEmpty()`, it returns a no-op Gatherer. In general, I guess most > >factory methods with a collection parameter (and compatible type arguments > >for T and R) will have a degenerate case like this when the collection is > >empty. ` Gatherer append(T... elements)` would be another > >example. > > `identity()` would also allow an optimized `andThen` implementation, simply > returning its argument. And when uncomposed, the Stream library could > eliminate the `gather` stage, allowing characteristics to propogate in this > case. So `treeSet.stream().gather(identity()).sorted().distinct().toList()` > could be optimized to `treeSet.stream().toList()`. > > Have you experimented with implementing your own identity Gatherer and > implemented its andThen to return the second argument? > > >That being said, I hadn't considered cases where an intermediate stage in > >the pipeline would not propagate the characteristics. And in cases where the > >intermediate stage doesn't affect the characteristics, it would actually be > >worse to use something like `Gatherers.sorted().andThen(…)`, instead of just > >keeping track of the previous element and throwing an IllegalStateException > >if necessary. > > Yeah, that or implementing a reorder buffer Gatherer (in case you have unique > and continuous sequence numbers). > > >This raises a new question though: on line 27 I'd expect I wouldn't need to > >specify the type arguments for the `ofSequential` method invocation. Is this > >hitting inherent limitations of type inference or is it possible that some > >generic type bounds aren't as generic as they could be, prohibiting type > >inference in certain cases? > > Yes, there are some limitations to inference, you can see usage examples in > the implementation of Gatherers which does need some assistance to > inference:https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/stream/Gatherers.java > > Cheers, > > √ > > **Viktor Klang** > Software Architect, Java Platform Group > > Oracle > > > > **From:** Anthony Vanelverdinghe > **Sent:** Tuesday, 30 July 2024 17:20 > **To:** Viktor Klang ; core-libs-dev@openjdk.org > > **Subject:** [External] : Re: Stream Gatherers (JEP 473) feedback >  > > July 29, 2024 at 8:08 PM, "Viktor Klang" wrote: > > > > > > Hi Anthony, > > Hi Viktor > > > Thank you for your patience, and for providing feedback, it is always much > > appreciated. > > > > > > >When writing factory methods for Gatherers, there's sometimes a > > > degenerate case that requires returning a no-op Gatherer. So I'd like a > > > way to mark a no-op Gatherer as such, allowing the Stream implementation > > > to recognize and eliminate it from the pipeline. One idea is to add > > > Gatherer.defaultIntegrator(), analogous to the other default… methods. > > > Another is to add Gatherers.identity(), analogous to Function.identity(). > > > > > > I contemplated adding that but in the end I decided I didn't want to add it > > for the sake of adding it, > > > but rather adding it in case it was deemed necessary. > > > > > > Do you have a concrete use-case (code) that you could share? > > The use case is a time series, which has methods to return a Stream of data > points, `record DataPoint(Instant, BigDecimal)`. In DataPoint, there are > several Gatherer factory methods, one of which is `Gatherer DataPoint> withInterpolationAt(NavigableSet instants)`. If > `instants.isEmpty()`, it returns a no-op Gatherer. In general, I guess most > factory methods with a collection parameter (and compatible type arguments > for T and R) will have a degenerate case like this when the collection is > empty. ` Gatherer append(T... elements)` would be another example. > > `identity()` would also allow an optimized `andThen` implementation, simply > returning its argument. A
Re: RFR: 8336856: Efficient hidden class-based string concatenation strategy [v55]
> This PR implements the same algorithm as the current generateMHInlineCopy > based on bytecode to improve startup performance. Shaojin Wen has updated the pull request incrementally with two additional commits since the last revision: - static final - code style - Changes: - all: https://git.openjdk.org/jdk/pull/20273/files - new: https://git.openjdk.org/jdk/pull/20273/files/094b2e54..7d481ece Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20273&range=54 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20273&range=53-54 Stats: 25 lines in 2 files changed: 7 ins; 4 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/20273.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20273/head:pull/20273 PR: https://git.openjdk.org/jdk/pull/20273
compatibility impact of JDK-8325621 (Improve jspawnhelper version checks)
Hi,
I have a data point to share on the compatibility impact of
JDK-8325621 (Improve jspawnhelper version checks), which was
backported to earlier release trains.
As a result of that change, doing an in-place upgrade from 22.0.1 to
22.0.2 breaks long-running Java applications that create subprocesses.
This is an expected consequence of the change, which adds version
checks to jspawnhelper to detect if a JVM is updated in-place to a
version with an incompatible jspawnhelper.
I'm curious if the list has suggestions on the best way to mitigate
the compatibility impact. One possibility is that replacing a JVM
in-place for a running process should never happen, and installers
should use new directories, or ensure any long-running Java processes
are shut down during the upgrade. Is that considered a best practice?
Here's a demo of the issue I saw:
$ cat T.java
public class T {
public static void main(String[] args) throws Exception {
while (true) {
Thread.sleep(1000);
System.err.println(Runtime.version());
ProcessBuilder pb = new ProcessBuilder("/usr/bin/date");
pb.redirectError(ProcessBuilder.Redirect.INHERIT);
pb.redirectOutput(ProcessBuilder.Redirect.INHERIT);
pb.start();
}
}
}
$ ./jdk-22.0.1/bin/java -fullversion
openjdk full version "22.0.1+8-16"
$ ./jdk-22.0.2/bin/java -fullversion
openjdk full version "22.0.2+9-70"
$ javac T.java
$ rsync -a jdk-22.0.1/ jdk/
$ ./jdk/bin/java T &
$ rsync -a jdk-22.0.2/ jdk/
...
jspawnhelper version 22.0.2+9-70
This command is not for general use and should only be run as the
result of a call to
ProcessBuilder.start() or Runtime.exec() in a java application
Exception in thread "main" java.io.IOException: Cannot run program
"/usr/bin/date": error=0, Failed to exec spawn helper: pid: 3740946,
exit value: 1
at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1170)
at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1089)
at T.main(T.java:12)
Caused by: java.io.IOException: error=0, Failed to exec spawn helper:
pid: 3740946, exit value: 1
at java.base/java.lang.ProcessImpl.forkAndExec(Native Method)
at java.base/java.lang.ProcessImpl.(ProcessImpl.java:295)
at java.base/java.lang.ProcessImpl.start(ProcessImpl.java:225)
at java.base/java.lang.ProcessBuilder.start(ProcessBuilder.java:1126)
... 2 more
Re: RFR: 8332842: Optimize empty CopyOnWriteArrayList allocations [v3]
On Wed, 19 Jun 2024 12:52:46 GMT, Alan Bateman wrote: >> @AlanBateman -- could you please take a look? Thanks. > >> @AlanBateman -- could you please take a look? Thanks. > > There was a lot of heap analysis done a few years ago that shined a light on > the number of empty collections in a typical heap. I don't recall seeing > COWAL in any of the data but it seems plausible that there are cases where > there are a lot of empty COWAL instances in the heap. > > Main thing for a change like this to make sure it doesn't hurt other cases > and check if there are overridable methods used in the existing + updated > implementations that might get reported as a behavior change by something > that extends and overrides some methods. I don't see anything so I think it's > okay. > > One thing that surprises me is the change to remove(Object) to handle the > case where the last remaining element is removed. Does that help the > application that prompted this change? Part of me wonders if remove(int) > needs to do the same as someone will show up sometime to ask. @AlanBateman review please. - PR Comment: https://git.openjdk.org/jdk/pull/19527#issuecomment-2286841739
Re: RFR: 8336856: Efficient hidden class-based string concatenation strategy [v55]
On Tue, 13 Aug 2024 16:34:18 GMT, Shaojin Wen wrote: >> This PR implements the same algorithm as the current generateMHInlineCopy >> based on bytecode to improve startup performance. > > Shaojin Wen has updated the pull request incrementally with two additional > commits since the last revision: > > - static final > - code style Marked as reviewed by redestad (Reviewer). Thanks for your patience and hard work on this! I think this is ready to go, assuming tests and performance looks fine. I'll start some larger tests in our test system. - PR Review: https://git.openjdk.org/jdk/pull/20273#pullrequestreview-2236302953 PR Comment: https://git.openjdk.org/jdk/pull/20273#issuecomment-2286890456
Re: RFR: 8337302: Undefined type variable results in null [v3]
On Mon, 12 Aug 2024 23:08:07 GMT, Rafael Winterhalter wrote: >> When a type uses a type variable without a declaration, no exception is >> thrown. This change triggers a `TypeNotFoundException` to be thrown. > > Rafael Winterhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8337302: Add missing visibility modifiers. src/java.base/share/classes/java/lang/TypeNotPresentException.java line 1: > 1: /* Can you update the copyright header to this? /* * Copyright (c) 2003, 2024, Oracle and/or its affiliates. All rights reserved. So 2024 instead of 2020. Same for `CoreReflectionFactory`. test/jdk/java/lang/reflect/Generics/TestMissingTypeVariable.java line 2: > 1: /* > 2: * Copyright (c) 2004, Oracle and/or its affiliates. All rights reserved. Suggestion: * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. - PR Review Comment: https://git.openjdk.org/jdk/pull/20535#discussion_r1715835788 PR Review Comment: https://git.openjdk.org/jdk/pull/20535#discussion_r1715835042
Withdrawn: 8261400: Reflection member filtering registration might be flawed
On Fri, 5 Jul 2024 19:27:27 GMT, Chen Liang wrote: > Please review this patch that address the reflection member filtering flaws. > > 1. Remove a pointless bootstrap check to ensure no filter is accidentally > bypassed due to class-loading order. > 2. Clarify the scenarios for filtering, and the correct filter actions for > each scenario. > 3. Remove the filter in `sun.misc.Unsafe`; users are already using other ways > to steal this instance, bypassing the filtered getter. The > `jdk.internal.misc.Unsafe`'s filter was already removed in JDK-8161129, so > this filter is meaningless. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/20058
Re: RFR: 8336754: Remodel TypeAnnotation to "has" instead of "be" an Annotation [v9]
> `TypeAnnotation` is not an annotation, as it should not be used in places > like `AnnotationValue.ofAnnotation`. Thus it's remodeled to contain an > annotation at a given location instead of to be an annotation. > > Depends on #20205. Chen Liang 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 18 additional commits since the last revision: - More bad terms and redundancy - Fix terminology - Merge branch 'master' of https://github.com/openjdk/jdk into fix/typeanno-model - Merge branch 'master' of https://github.com/openjdk/jdk into fix/typeanno-model - Stage - Merge branch 'master' of https://github.com/openjdk/jdk into fix/typeanno-model - remove compile, use element-value, break long sentences - Merge branch 'master' of https://github.com/openjdk/jdk into fix/typeanno-model - Improve docs for repeating, default, and value name - Merge branch 'master' of https://github.com/openjdk/jdk into fix/typeanno-model - ... and 8 more: https://git.openjdk.org/jdk/compare/f09e48cb...c66c7d25 - Changes: - all: https://git.openjdk.org/jdk/pull/20247/files - new: https://git.openjdk.org/jdk/pull/20247/files/3a91a3a5..c66c7d25 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20247&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20247&range=07-08 Stats: 15357 lines in 539 files changed: 6184 ins; 6850 del; 2323 mod Patch: https://git.openjdk.org/jdk/pull/20247.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20247/head:pull/20247 PR: https://git.openjdk.org/jdk/pull/20247
Re: RFR: 8336754: Remodel TypeAnnotation to "has" instead of "be" an Annotation [v10]
> `TypeAnnotation` is not an annotation, as it should not be used in places > like `AnnotationValue.ofAnnotation`. Thus it's remodeled to contain an > annotation at a given location instead of to be an annotation. > > Depends on #20205. Chen Liang has updated the pull request incrementally with one additional commit since the last revision: Missed JLS prefix from web review - Changes: - all: https://git.openjdk.org/jdk/pull/20247/files - new: https://git.openjdk.org/jdk/pull/20247/files/c66c7d25..8af07782 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20247&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20247&range=08-09 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/20247.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20247/head:pull/20247 PR: https://git.openjdk.org/jdk/pull/20247
Re: RFR: 8338014: Improve usage of @jvms tags in class file API [v2]
On Mon, 12 Aug 2024 18:55:47 GMT, Joe Darcy wrote:
>> Looks fine; increasing review count to also include someone who more
>> directly maintains this API.
>>
>> If you haven't done so already, I recommend also doing a quick check for an
>> analagous issue with `@jls` tags in this API.
>
>> @jddarcy Perhaps we should revisit the inline forms of `@jls` and `@jvms`
>> tags to come up with similar and more consistent usage.
>
> Certainly a variant do generate the most common usage pattern would be
> helpful -- brainstorming --
>
> `{@jvms cite 1.2.3}`
>
> to generate
>
> (JVMVS 1.2.3)
>
> where "1.2.3" was the expected link.
Hi @jddarcy, just checking in if the update looks good prior to integration.
-
PR Comment: https://git.openjdk.org/jdk/pull/20513#issuecomment-2287112583
Re: RFR: 8338014: Improve usage of @jvms tags in class file API [v2]
On Tue, 13 Aug 2024 14:37:32 GMT, Sonia Zaldana Calles wrote: >> Hi all, >> >> This PR addresses [8338014](https://bugs.openjdk.org/browse/JDK-8338014) >> improving the use of `@jvms` tags by adding `JVMS` prior to the tag. >> >> Thanks, >> Sonia > > Sonia Zaldana Calles has updated the pull request incrementally with two > additional commits since the last revision: > > - Merge branch 'JDK-8338014' of github.com:SoniaZaldana/jdk into JDK-8338014 > - Adding parentheses to references which are not part of sentences and > removing redundant additions Marked as reviewed by darcy (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20513#pullrequestreview-2236559081
Re: RFR: 8338014: Improve usage of @jvms tags in class file API [v2]
On Tue, 13 Aug 2024 14:37:32 GMT, Sonia Zaldana Calles wrote: >> Hi all, >> >> This PR addresses [8338014](https://bugs.openjdk.org/browse/JDK-8338014) >> improving the use of `@jvms` tags by adding `JVMS` prior to the tag. >> >> Thanks, >> Sonia > > Sonia Zaldana Calles has updated the pull request incrementally with two > additional commits since the last revision: > > - Merge branch 'JDK-8338014' of github.com:SoniaZaldana/jdk into JDK-8338014 > - Adding parentheses to references which are not part of sentences and > removing redundant additions Please allow for about 24 hours after the last push so that everyone interested may review, such as @asotona. - PR Comment: https://git.openjdk.org/jdk/pull/20513#issuecomment-2287132524
Re: RFR: 8336384: AbstractQueuedSynchronizer.acquire should cancel acquire when failing due to a LinkageError or other errors [v2]
On Tue, 13 Aug 2024 08:02:04 GMT, Alan Bateman wrote: > > It has been a while since I knew this code reasonably well so perhaps I > > have just forgotten this difference between AQS and built-in monitors, but > > it seems that a Condition.await can return by throwing an exception without > > re-acquiring the associated synchronizer. Or is that handled at a > > higher-level? > > The semantics are the same as monitor wait/notify so Condition.await must > guarantee to hold the lock when it returns. If ConditionNode.block were to > throw something like StackOverflowError then there would be an issue (it's a > different park to the one changed in this PR but I think you do have a good > point). AFAICS `await` would call the acquire method that was changed here. I know we have issues with OOME and SOE, but these changes admit more general exception possibilities that would seem to undermine the required guarantee. But perhaps a different `acquire` is involved in the `await` case? - PR Comment: https://git.openjdk.org/jdk/pull/20548#issuecomment-2287218877
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v11]
On Mon, 12 Aug 2024 15:58:14 GMT, Axel Boldt-Christmas
wrote:
>> When inflating a monitor the `ObjectMonitor*` is written directly over the
>> `markWord` and any overwritten data is displaced into a displaced
>> `markWord`. This is problematic for concurrent GCs which needs extra care or
>> looser semantics to use this displaced data. In Lilliput this data also
>> contains the klass forcing this to be something that the GC has to take into
>> account everywhere.
>>
>> This patch introduces an alternative solution where locking only uses the
>> lock bits of the `markWord` and inflation does not override and displace the
>> `markWord`. This is done by keeping associations between objects and
>> `ObjectMonitor*` in an external hash table. Different caching techniques are
>> used to speedup lookups from compiled code.
>>
>> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is
>> only supported in combination with the LM_LIGHTWEIGHT locking mode (the
>> default).
>>
>> This patch has been evaluated to be performance neutral when
>> `UseObjectMonitorTable` is turned off (the default).
>>
>> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT`
>> and `UseObjectMonitorTable` works.
>>
>> # Cleanups
>>
>> Cleaned up displaced header usage for:
>> * BasicLock
>> * Contains some Zero changes
>> * Renames one exported JVMCI field
>> * ObjectMonitor
>> * Updates comments and tests consistencies
>>
>> # Refactoring
>>
>> `ObjectMonitor::enter` has been refactored an a
>> `ObjectMonitorContentionMark` witness object has been introduced to the
>> signatures. Which signals that the contentions reference counter is being
>> held. More details are given below in the section about deflation.
>>
>> The initial purpose of this was to allow `UseObjectMonitorTable` to interact
>> more seamlessly with the `ObjectMonitor::enter` code.
>>
>> _There is even more `ObjectMonitor` refactoring which can be done here to
>> create a more understandable and enforceable API. There are a handful of
>> invariants / assumptions which are not always explicitly asserted which
>> could be trivially abstracted and verified by the type system by using
>> similar witness objects._
>>
>> # LightweightSynchronizer
>>
>> Working on adapting and incorporating the following section as a comment in
>> the source code
>>
>> ## Fast Locking
>>
>> CAS on locking bits in markWord.
>> 0b00 (Fast Locked) <--> 0b01 (Unlocked)
>>
>> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to
>> avoid inflating by spinning a bit.
>>
>> If 0b10 (Inflated) is observed or there is to...
>
> Axel Boldt-Christmas has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Missing DEBUG_ONLY
src/hotspot/share/interpreter/zero/bytecodeInterpreter.cpp line 632:
> 630: bool success = false;
> 631: if (LockingMode == LM_LEGACY) {
> 632:// Traditional lightweight locking.
The if-statement is for legacy locking so the comment about lightweight
locking seems wrong.
src/hotspot/share/interpreter/zero/bytecodeInterpreter.cpp line 736:
> 734: bool success = false;
> 735: if (LockingMode == LM_LEGACY) {
> 736: // traditional lightweight locking
The if-statement is for legacy locking so the comment about lightweight
locking seems wrong.
src/hotspot/share/interpreter/zero/bytecodeInterpreter.cpp line 1671:
> 1669: bool success = false;
> 1670: if (LockingMode == LM_LEGACY) {
> 1671: // traditional lightweight locking
The if-statement is for legacy locking so the comment about lightweight
locking seems wrong.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1503:
> 1501:
> 1502: if (mon != nullptr) {
> 1503: assert(mon != nullptr, "must have monitor");
With the new if-statement on L1502, the assert is not needed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1714439929
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1714440641
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1714441506
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1714448544
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v15]
On Tue, 13 Aug 2024 14:52:03 GMT, Axel Boldt-Christmas
wrote:
>> When inflating a monitor the `ObjectMonitor*` is written directly over the
>> `markWord` and any overwritten data is displaced into a displaced
>> `markWord`. This is problematic for concurrent GCs which needs extra care or
>> looser semantics to use this displaced data. In Lilliput this data also
>> contains the klass forcing this to be something that the GC has to take into
>> account everywhere.
>>
>> This patch introduces an alternative solution where locking only uses the
>> lock bits of the `markWord` and inflation does not override and displace the
>> `markWord`. This is done by keeping associations between objects and
>> `ObjectMonitor*` in an external hash table. Different caching techniques are
>> used to speedup lookups from compiled code.
>>
>> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is
>> only supported in combination with the LM_LIGHTWEIGHT locking mode (the
>> default).
>>
>> This patch has been evaluated to be performance neutral when
>> `UseObjectMonitorTable` is turned off (the default).
>>
>> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT`
>> and `UseObjectMonitorTable` works.
>>
>> # Cleanups
>>
>> Cleaned up displaced header usage for:
>> * BasicLock
>> * Contains some Zero changes
>> * Renames one exported JVMCI field
>> * ObjectMonitor
>> * Updates comments and tests consistencies
>>
>> # Refactoring
>>
>> `ObjectMonitor::enter` has been refactored an a
>> `ObjectMonitorContentionMark` witness object has been introduced to the
>> signatures. Which signals that the contentions reference counter is being
>> held. More details are given below in the section about deflation.
>>
>> The initial purpose of this was to allow `UseObjectMonitorTable` to interact
>> more seamlessly with the `ObjectMonitor::enter` code.
>>
>> _There is even more `ObjectMonitor` refactoring which can be done here to
>> create a more understandable and enforceable API. There are a handful of
>> invariants / assumptions which are not always explicitly asserted which
>> could be trivially abstracted and verified by the type system by using
>> similar witness objects._
>>
>> # LightweightSynchronizer
>>
>> Working on adapting and incorporating the following section as a comment in
>> the source code
>>
>> ## Fast Locking
>>
>> CAS on locking bits in markWord.
>> 0b00 (Fast Locked) <--> 0b01 (Unlocked)
>>
>> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to
>> avoid inflating by spinning a bit.
>>
>> If 0b10 (Inflated) is observed or there is to...
>
> Axel Boldt-Christmas has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Remove the last OMWorld references
> - Rename omworldtable_work to object_monitor_table_work
src/hotspot/share/runtime/basicLock.inline.hpp line 45:
> 43: return reinterpret_cast(get_metadata());
> 44: #else
> 45: // Other platforms does not make use of the cache yet,
nit typo: s/does not/do not/
src/hotspot/share/runtime/basicLock.inline.hpp line 54:
> 52: inline void BasicLock::clear_object_monitor_cache() {
> 53: assert(UseObjectMonitorTable, "must be");
> 54: set_metadata(0);
Should this be a literal `0` or should it be `nullptr`?
Update: The metadata field is of type `unintptr_t`. Got it.
src/hotspot/share/runtime/deoptimization.cpp line 1650:
> 1648: mon_info->lock()->set_bad_metadata_deopt();
> 1649: }
> 1650: #endif
I like this!
src/hotspot/share/runtime/globals.hpp line 1964:
> 1962:
> \
> 1963: product(int, LightweightFastLockingSpins, 13, DIAGNOSTIC,
> \
> 1964: "Specifies the number of time lightweight fast locking will "
> \
nit typo: s/number of time/number of times/
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 34:
> 32: #include "oops/oop.inline.hpp"
> 33: #include "runtime/atomic.hpp"
> 34: #include "memory/allStatic.hpp"
nit: this include is out of order.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 43:
> 41: #include "runtime/mutexLocker.hpp"
> 42: #include "runtime/objectMonitor.hpp"
> 43: #include "runtime/objectMonitor.inline.hpp"
Shouldn't have both includes here.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 81:
> 79: oop _obj;
> 80:
> 81: public:
nit - please indent by one more space.
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 86:
> 84: uintx get_hash() const {
> 85: uintx hash = _obj->mark().hash();
> 86: assert(hash != 0, "should have a hash");
Hmmm... I can remember seeing hash values of zero in some
of my older legacy inflation stress runs. Is a hash value of zero
not a thing with lightweight locking?
src/hotspot/share/runtime/lightweightSynchronizer.c
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v6]
On Mon, 15 Jul 2024 00:45:10 GMT, Axel Boldt-Christmas
wrote:
>> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 401:
>>
>>> 399:
>>> 400: if (inserted) {
>>> 401: // Hopefully the performance counters are allocated on distinct
>>
>> It doesn't look like the counters are on distinct cache lines (see
>> objectMonitor.hpp, lines 212ff). If this is a concern, file a bug to
>> investigate it later? The comment here is a bit misplaced, IMO.
>
> It originates from
> https://github.com/openjdk/jdk/blob/15997bc3dfe9dddf21f20fa189f97291824892de/src/hotspot/share/runtime/synchronizer.cpp#L1543
>
>
> I think we just kept it and did not think more about it.
>
> Not sure what it is referring to. Maybe @dcubed-ojdk knows more, they
> originated from him (9 years old comment).
I don't think we ever got around to experimenting with putting the perf counters
on distinct cache lines. We've always had bigger fish to fry.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715669185
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v15]
On Tue, 13 Aug 2024 14:56:32 GMT, Coleen Phillimore wrote:
>> Axel Boldt-Christmas has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Remove the last OMWorld references
>> - Rename omworldtable_work to object_monitor_table_work
>
> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 172:
>
>> 170: static void create() {
>> 171: _table = new ConcurrentTable(initial_log_size(),
>> 172: max_log_size(),
>
> nit, can you line up these parameters?
Or put them all on L171 if that doesn't make it too long...
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715648059
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v16]
On Tue, 13 Aug 2024 16:30:12 GMT, Axel Boldt-Christmas
wrote:
>> When inflating a monitor the `ObjectMonitor*` is written directly over the
>> `markWord` and any overwritten data is displaced into a displaced
>> `markWord`. This is problematic for concurrent GCs which needs extra care or
>> looser semantics to use this displaced data. In Lilliput this data also
>> contains the klass forcing this to be something that the GC has to take into
>> account everywhere.
>>
>> This patch introduces an alternative solution where locking only uses the
>> lock bits of the `markWord` and inflation does not override and displace the
>> `markWord`. This is done by keeping associations between objects and
>> `ObjectMonitor*` in an external hash table. Different caching techniques are
>> used to speedup lookups from compiled code.
>>
>> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is
>> only supported in combination with the LM_LIGHTWEIGHT locking mode (the
>> default).
>>
>> This patch has been evaluated to be performance neutral when
>> `UseObjectMonitorTable` is turned off (the default).
>>
>> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT`
>> and `UseObjectMonitorTable` works.
>>
>> # Cleanups
>>
>> Cleaned up displaced header usage for:
>> * BasicLock
>> * Contains some Zero changes
>> * Renames one exported JVMCI field
>> * ObjectMonitor
>> * Updates comments and tests consistencies
>>
>> # Refactoring
>>
>> `ObjectMonitor::enter` has been refactored an a
>> `ObjectMonitorContentionMark` witness object has been introduced to the
>> signatures. Which signals that the contentions reference counter is being
>> held. More details are given below in the section about deflation.
>>
>> The initial purpose of this was to allow `UseObjectMonitorTable` to interact
>> more seamlessly with the `ObjectMonitor::enter` code.
>>
>> _There is even more `ObjectMonitor` refactoring which can be done here to
>> create a more understandable and enforceable API. There are a handful of
>> invariants / assumptions which are not always explicitly asserted which
>> could be trivially abstracted and verified by the type system by using
>> similar witness objects._
>>
>> # LightweightSynchronizer
>>
>> Working on adapting and incorporating the following section as a comment in
>> the source code
>>
>> ## Fast Locking
>>
>> CAS on locking bits in markWord.
>> 0b00 (Fast Locked) <--> 0b01 (Unlocked)
>>
>> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to
>> avoid inflating by spinning a bit.
>>
>> If 0b10 (Inflated) is observed or there is to...
>
> Axel Boldt-Christmas has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Whitespace and nits
I finished my first pass crawl thru on these changes. I need to mull
on these changes a bit before I make another pass. I think there's
only one real bug buried in all of my comments.
src/hotspot/share/runtime/objectMonitor.cpp line 377:
> 375:
> 376: if (cur == current) {
> 377: // TODO-FIXME: check for integer overflow! BUGID 6557169.
Thanks for removing this comment. JDK-6557169 was closed as
"Will Not FIx" in 2017.
src/hotspot/share/runtime/synchronizer.cpp line 970:
> 968: if (value == 0) value = 0xBAD;
> 969: assert(value != markWord::no_hash, "invariant");
> 970: return value;
In the elided part above this line, we have:
if (value == 0) value = 0xBAD;
assert(value != markWord::no_hash, "invariant");
so my memory about zero being returnable as a hash value is wrong.
src/hotspot/share/runtime/synchronizer.cpp line 977:
> 975:
> 976: markWord mark = obj->mark_acquire();
> 977: for(;;) {
nit - please insert space before `(`
src/hotspot/share/runtime/synchronizer.cpp line 997:
> 995: // Since the monitor isn't in the object header, it can simply be
> installed.
> 996: if (UseObjectMonitorTable) {
> 997: return install_hash_code(current, obj);
Perhaps:
if (UseObjectMonitorTable) {
// Since the monitor isn't in the object header, the hash can simply be
// installed in the object header.
return install_hash_code(current, obj);
src/hotspot/share/runtime/synchronizer.cpp line 1271:
> 1269: _no_progress_cnt >= NoAsyncDeflationProgressMax) {
> 1270: double remainder = (100.0 - MonitorUsedDeflationThreshold) / 100.0;
> 1271: size_t new_ceiling = ceiling / remainder + 1;
Why was the `new_ceiling` calculation changed?
I think the `new_ceiling` value is going to lower than the old ceiling value.
src/hotspot/share/runtime/synchronizer.inline.hpp line 83:
> 81:
> 82:
> 83: #endif // SHARE_RUNTIME_SYNCHRONIZER_INLINE_HPP
nit - please delete one of the blank lines.
src/hotspot/share/runtime/vframe.cpp line 252:
> 250: if (mark.has_monitor()) {
> 251: ObjectMonitor* mon =
> ObjectSynchronizer::read_monitor(current, mon
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v15]
On Tue, 13 Aug 2024 16:49:42 GMT, Daniel D. Daugherty
wrote:
>> Axel Boldt-Christmas has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Remove the last OMWorld references
>> - Rename omworldtable_work to object_monitor_table_work
>
> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 86:
>
>> 84: uintx get_hash() const {
>> 85: uintx hash = _obj->mark().hash();
>> 86: assert(hash != 0, "should have a hash");
>
> Hmmm... I can remember seeing hash values of zero in some
> of my older legacy inflation stress runs. Is a hash value of zero
> not a thing with lightweight locking?
Update: My memory was wrong. When zero is encountered as a
hash value, it is replaced with `0xBAD`.
> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 414:
>
>> 412:
>> 413: intptr_t hash = obj->mark().hash();
>> 414: assert(hash != 0, "must be set when claiming the object monitor");
>
> Hmmm... I can remember seeing hash values of zero in some
> of my older legacy inflation stress runs. Is a hash value of zero
> not a thing with lightweight locking?
Update: My memory was wrong. When zero is encountered as a
hash value, it is replaced with `0xBAD`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715952007
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1715952460
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v16]
On Tue, 13 Aug 2024 16:30:12 GMT, Axel Boldt-Christmas
wrote:
>> When inflating a monitor the `ObjectMonitor*` is written directly over the
>> `markWord` and any overwritten data is displaced into a displaced
>> `markWord`. This is problematic for concurrent GCs which needs extra care or
>> looser semantics to use this displaced data. In Lilliput this data also
>> contains the klass forcing this to be something that the GC has to take into
>> account everywhere.
>>
>> This patch introduces an alternative solution where locking only uses the
>> lock bits of the `markWord` and inflation does not override and displace the
>> `markWord`. This is done by keeping associations between objects and
>> `ObjectMonitor*` in an external hash table. Different caching techniques are
>> used to speedup lookups from compiled code.
>>
>> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is
>> only supported in combination with the LM_LIGHTWEIGHT locking mode (the
>> default).
>>
>> This patch has been evaluated to be performance neutral when
>> `UseObjectMonitorTable` is turned off (the default).
>>
>> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT`
>> and `UseObjectMonitorTable` works.
>>
>> # Cleanups
>>
>> Cleaned up displaced header usage for:
>> * BasicLock
>> * Contains some Zero changes
>> * Renames one exported JVMCI field
>> * ObjectMonitor
>> * Updates comments and tests consistencies
>>
>> # Refactoring
>>
>> `ObjectMonitor::enter` has been refactored an a
>> `ObjectMonitorContentionMark` witness object has been introduced to the
>> signatures. Which signals that the contentions reference counter is being
>> held. More details are given below in the section about deflation.
>>
>> The initial purpose of this was to allow `UseObjectMonitorTable` to interact
>> more seamlessly with the `ObjectMonitor::enter` code.
>>
>> _There is even more `ObjectMonitor` refactoring which can be done here to
>> create a more understandable and enforceable API. There are a handful of
>> invariants / assumptions which are not always explicitly asserted which
>> could be trivially abstracted and verified by the type system by using
>> similar witness objects._
>>
>> # LightweightSynchronizer
>>
>> Working on adapting and incorporating the following section as a comment in
>> the source code
>>
>> ## Fast Locking
>>
>> CAS on locking bits in markWord.
>> 0b00 (Fast Locked) <--> 0b01 (Unlocked)
>>
>> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to
>> avoid inflating by spinning a bit.
>>
>> If 0b10 (Inflated) is observed or there is to...
>
> Axel Boldt-Christmas has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Whitespace and nits
Just for clarity, I think this is the one real bug that I found:
src/hotspot/share/runtime/lightweightSynchronizer.cpp line 126:
> 124:
> 125: static void dec_items_count() {
> 126: Atomic::inc(&_items_count);
Shouldn't this be `Atomic::dec`?
-
PR Comment: https://git.openjdk.org/jdk/pull/20067#issuecomment-2287237680
Re: RFR: 8337302: Undefined type variable results in null [v4]
> When a type uses a type variable without a declaration, no exception is > thrown. This change triggers a `TypeNotFoundException` to be thrown. Rafael Winterhalter has updated the pull request incrementally with one additional commit since the last revision: 8337302: Fix copyright years. - Changes: - all: https://git.openjdk.org/jdk/pull/20535/files - new: https://git.openjdk.org/jdk/pull/20535/files/2dbb17c6..10d73b6c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20535&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20535&range=02-03 Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/20535.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20535/head:pull/20535 PR: https://git.openjdk.org/jdk/pull/20535
Re: RFR: 8337302: Undefined type variable results in null [v4]
On Tue, 13 Aug 2024 23:41:06 GMT, Rafael Winterhalter wrote: >> When a type uses a type variable without a declaration, no exception is >> thrown. This change triggers a `TypeNotFoundException` to be thrown. > > Rafael Winterhalter has updated the pull request incrementally with one > additional commit since the last revision: > > 8337302: Fix copyright years. Thanks for this patch! - Marked as reviewed by liach (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20535#pullrequestreview-2236902946
Re: compatibility impact of JDK-8325621 (Improve jspawnhelper version checks)
HI, In addition to Liam's point, I'd like to point out: 1. The version check in jspawnhelper is for the full JDK version. This means any future in-place JDK update could trigger this problem for long-running applications. 2. Typical Linux package managers (e.g. apt, yum) seem to update the JDK package in place by default. We run into this problem with updates from "apt install" internally. Admittedly, I have not tested if updating Debian's openjdk-21-jre-headless from 17.0.12 to 17.0.13, or from 21.0.3 to 21.0.4 would trigger the same issue, but it likely will. This could be a widespread problem for future JDK updates on Linux distros. Perhaps we should also raise this concern to jdk-updates-...@openjdk.org? -Man
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v20]
> The testcase requires root permissions. > > Fix by Severin Gehwolf. > Testcase by Jan Kratochvil. Jan Kratochvil has updated the pull request incrementally with one additional commit since the last revision: Testcase update upon review by Severin Gehwolf - Changes: - all: https://git.openjdk.org/jdk/pull/17198/files - new: https://git.openjdk.org/jdk/pull/17198/files/4aec7a6e..b790b181 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17198&range=19 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17198&range=18-19 Stats: 131 lines in 3 files changed: 21 ins; 67 del; 43 mod Patch: https://git.openjdk.org/jdk/pull/17198.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17198/head:pull/17198 PR: https://git.openjdk.org/jdk/pull/17198
Re: RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v19]
On Mon, 12 Aug 2024 18:39:46 GMT, Severin Gehwolf wrote: >> Jan Kratochvil has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Inline adjust_controller() twice >> - Revert "Unify 4 copies of adjust_controller()" >> >>This reverts commit 77a81d07d74c8ae9bf34bfd8df9bcaca451ede9a. > > test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 217: > >> 215: >> 216: // KFAIL - verify the >> CgroupSubsystem::initialize_hierarchy() and >> jdk.internal.platform.CgroupSubsystem.initializeHierarchy() bug >> 217: // TestTwoLimits does not see the lower MEMORY_MAX_OUTER >> limit. > > Remove this obsolete(?) comment. This comment is documenting what the `TestTwoLimits` testcase does. Which I find useful. It is KFAIL - Known Failure - the current OpenJDK code does not properly simulate what Linux kernel does. If you do: cgset -r memory.max=$[1024*1024*1024] a/b cgset -r memory.max=$[512*1024] a cgexec -g memory:a/b java... Then OpenJDK thinks it is `1024*1024*1024 KB` but Linux kernel will limit OpenJDK to `512*1024 KB`. So this problem is documented and tested. - PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1716261053
Re: RFR: 8338021: Support saturating vector operators in VectorAPI [v3]
> Hi All, > > As per the discussion on panama-dev mailing list[1], patch adds the support > following new vector operators. > > > . SATURATING_UADD : Saturating unsigned addition. > . SATURATING_ADD: Saturating signed addition. > . SATURATING_USUB : Saturating unsigned subtraction. > . SATURATING_SUB: Saturating signed subtraction. > . UMAX : Unsigned max > . UMIN : Unsigned min. > > > New vector operators are applicable to only integral types since their values > wraparound in over/underflowing scenarios after setting appropriate status > flags. For floating point types, as per IEEE 754 specs there are multiple > schemes to handler underflow, one of them is gradual underflow which > transitions the value to subnormal range. Similarly, overflow implicitly > saturates the floating-point value to an Infinite value. > > As the name suggests, these are saturating operations, i.e. the result of the > computation is strictly capped by lower and upper bounds of the result type > and is not wrapped around in underflowing or overflowing scenarios. > > Summary of changes: > - Java side implementation of new vector operators. > - Add new scalar saturating APIs for each of the above saturating vector > operator in corresponding primitive box classes, fallback implementation of > vector operators is based over it. > - C2 compiler IR and inline expander changes. > - Optimized x86 backend implementation for new vector operators and their > predicated counterparts. > - Extends existing VectorAPI Jtreg test suite to cover new operations. > > Kindly review and share your feedback. > > Best Regards, > PS: Intrinsification and auto-vectorization of new core-lib API will be > addressed separately in a follow-up patch. > > [1] https://mail.openjdk.org/pipermail/panama-dev/2024-May/020408.html Jatin Bhateja has updated the pull request incrementally with one additional commit since the last revision: Review comments resolutions - Changes: - all: https://git.openjdk.org/jdk/pull/20507/files - new: https://git.openjdk.org/jdk/pull/20507/files/5468e72b..8c9bfeca Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20507&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20507&range=01-02 Stats: 776 lines in 34 files changed: 0 ins; 0 del; 776 mod Patch: https://git.openjdk.org/jdk/pull/20507.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20507/head:pull/20507 PR: https://git.openjdk.org/jdk/pull/20507
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v16]
On Tue, 13 Aug 2024 16:30:12 GMT, Axel Boldt-Christmas
wrote:
>> When inflating a monitor the `ObjectMonitor*` is written directly over the
>> `markWord` and any overwritten data is displaced into a displaced
>> `markWord`. This is problematic for concurrent GCs which needs extra care or
>> looser semantics to use this displaced data. In Lilliput this data also
>> contains the klass forcing this to be something that the GC has to take into
>> account everywhere.
>>
>> This patch introduces an alternative solution where locking only uses the
>> lock bits of the `markWord` and inflation does not override and displace the
>> `markWord`. This is done by keeping associations between objects and
>> `ObjectMonitor*` in an external hash table. Different caching techniques are
>> used to speedup lookups from compiled code.
>>
>> A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is
>> only supported in combination with the LM_LIGHTWEIGHT locking mode (the
>> default).
>>
>> This patch has been evaluated to be performance neutral when
>> `UseObjectMonitorTable` is turned off (the default).
>>
>> Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT`
>> and `UseObjectMonitorTable` works.
>>
>> # Cleanups
>>
>> Cleaned up displaced header usage for:
>> * BasicLock
>> * Contains some Zero changes
>> * Renames one exported JVMCI field
>> * ObjectMonitor
>> * Updates comments and tests consistencies
>>
>> # Refactoring
>>
>> `ObjectMonitor::enter` has been refactored an a
>> `ObjectMonitorContentionMark` witness object has been introduced to the
>> signatures. Which signals that the contentions reference counter is being
>> held. More details are given below in the section about deflation.
>>
>> The initial purpose of this was to allow `UseObjectMonitorTable` to interact
>> more seamlessly with the `ObjectMonitor::enter` code.
>>
>> _There is even more `ObjectMonitor` refactoring which can be done here to
>> create a more understandable and enforceable API. There are a handful of
>> invariants / assumptions which are not always explicitly asserted which
>> could be trivially abstracted and verified by the type system by using
>> similar witness objects._
>>
>> # LightweightSynchronizer
>>
>> Working on adapting and incorporating the following section as a comment in
>> the source code
>>
>> ## Fast Locking
>>
>> CAS on locking bits in markWord.
>> 0b00 (Fast Locked) <--> 0b01 (Unlocked)
>>
>> When locking and 0b00 (Fast Locked) is observed, it may be beneficial to
>> avoid inflating by spinning a bit.
>>
>> If 0b10 (Inflated) is observed or there is to...
>
> Axel Boldt-Christmas has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Whitespace and nits
src/hotspot/cpu/aarch64/c1_MacroAssembler_aarch64.cpp line 84:
> 82:
> 83: if (LockingMode == LM_LIGHTWEIGHT) {
> 84: lightweight_lock(disp_hdr, obj, hdr, temp, rscratch2, slow_case);
Given the declaration:
void MacroAssembler::lightweight_lock(Register basic_lock, Register obj,
Register t1, Register t2, Register t3, Label& slow)
it looks odd to pass `disp_hdr` here - is that variable just mis-named?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1716339494
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v17]
> When inflating a monitor the `ObjectMonitor*` is written directly over the > `markWord` and any overwritten data is displaced into a displaced `markWord`. > This is problematic for concurrent GCs which needs extra care or looser > semantics to use this displaced data. In Lilliput this data also contains the > klass forcing this to be something that the GC has to take into account > everywhere. > > This patch introduces an alternative solution where locking only uses the > lock bits of the `markWord` and inflation does not override and displace the > `markWord`. This is done by keeping associations between objects and > `ObjectMonitor*` in an external hash table. Different caching techniques are > used to speedup lookups from compiled code. > > A diagnostic VM option is introduced called `UseObjectMonitorTable`. It is > only supported in combination with the LM_LIGHTWEIGHT locking mode (the > default). > > This patch has been evaluated to be performance neutral when > `UseObjectMonitorTable` is turned off (the default). > > Below is a more detailed explanation of this change and how `LM_LIGHTWEIGHT` > and `UseObjectMonitorTable` works. > > # Cleanups > > Cleaned up displaced header usage for: > * BasicLock > * Contains some Zero changes > * Renames one exported JVMCI field > * ObjectMonitor > * Updates comments and tests consistencies > > # Refactoring > > `ObjectMonitor::enter` has been refactored an a `ObjectMonitorContentionMark` > witness object has been introduced to the signatures. Which signals that the > contentions reference counter is being held. More details are given below in > the section about deflation. > > The initial purpose of this was to allow `UseObjectMonitorTable` to interact > more seamlessly with the `ObjectMonitor::enter` code. > > _There is even more `ObjectMonitor` refactoring which can be done here to > create a more understandable and enforceable API. There are a handful of > invariants / assumptions which are not always explicitly asserted which could > be trivially abstracted and verified by the type system by using similar > witness objects._ > > # LightweightSynchronizer > > Working on adapting and incorporating the following section as a comment in > the source code > > ## Fast Locking > > CAS on locking bits in markWord. > 0b00 (Fast Locked) <--> 0b01 (Unlocked) > > When locking and 0b00 (Fast Locked) is observed, it may be beneficial to > avoid inflating by spinning a bit. > > If 0b10 (Inflated) is observed or there is to much contention or to long > critical sections for spinning to be feasible, inf... Axel Boldt-Christmas has updated the pull request incrementally with one additional commit since the last revision: Fix items count - Changes: - all: https://git.openjdk.org/jdk/pull/20067/files - new: https://git.openjdk.org/jdk/pull/20067/files/68681c84..41ac7d37 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=20067&range=16 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20067&range=15-16 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/20067.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/20067/head:pull/20067 PR: https://git.openjdk.org/jdk/pull/20067
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v11]
On Mon, 12 Aug 2024 22:40:06 GMT, Daniel D. Daugherty
wrote:
>> Axel Boldt-Christmas has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Missing DEBUG_ONLY
>
> src/hotspot/share/interpreter/zero/bytecodeInterpreter.cpp line 632:
>
>> 630: bool success = false;
>> 631: if (LockingMode == LM_LEGACY) {
>> 632:// Traditional lightweight locking.
>
> The if-statement is for legacy locking so the comment about lightweight
> locking seems wrong.
Yeah. It is an old comment from before LM_LIGHTWEIGHT existed. Will change to
call it fast locking.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1716348558
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v15]
On Tue, 13 Aug 2024 16:34:17 GMT, Daniel D. Daugherty wrote: >> Axel Boldt-Christmas has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Remove the last OMWorld references >> - Rename omworldtable_work to object_monitor_table_work > > src/hotspot/share/runtime/lightweightSynchronizer.hpp line 41: > >> 39: >> 40: static ObjectMonitor* add_monitor(JavaThread* current, ObjectMonitor* >> monitor, oop obj); >> 41: static bool remove_monitor(Thread* current, oop obj, ObjectMonitor* >> monitor); > > Hmmm... `add_monitor` has `monitor` and then `obj` params. > `remove_monitor` has `obj` and then `monitor` params. Why > not use the same order? Yeah, why not? :) - PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1716353596
Re: RFR: 8315884: New Object to ObjectMonitor mapping [v15]
On Tue, 13 Aug 2024 17:05:38 GMT, Daniel D. Daugherty
wrote:
>> Axel Boldt-Christmas has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Remove the last OMWorld references
>> - Rename omworldtable_work to object_monitor_table_work
>
> src/hotspot/share/runtime/lightweightSynchronizer.cpp line 126:
>
>> 124:
>> 125: static void dec_items_count() {
>> 126: Atomic::inc(&_items_count);
>
> Shouldn't this be `Atomic::dec`?
Yes it should be. Surprised I never saw this. (Even though that using the
service thread to grow was the very last thing changed before the PR, the
resizing was handled differently before, by the deflator thread.)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/20067#discussion_r1716377509
