Re: RFR: 8294977: Convert test/jdk/java tests from ASM library to Classfile API [v2]
> Summaries: > 1. A few recommendations about updating the constant API is made at > https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000233.html > and I may update this patch shall the API changes be integrated before > 2. One ASM library-specific test, `LambdaAsm` is removed. Others have their > code generation infrastructure upgraded from ASM to Classfile API. > 3. Most tests are included in tier1, but some are not: > In `:jdk_io`: (tier2, part 2) > > test/jdk/java/io/Serializable/records/SerialPersistentFieldsTest.java > test/jdk/java/io/Serializable/records/ProhibitedMethods.java > test/jdk/java/io/Serializable/records/BadCanonicalCtrTest.java > > In `:jdk_instrument`: (tier 3) > > test/jdk/java/lang/instrument/RetransformAgent.java > test/jdk/java/lang/instrument/NativeMethodPrefixAgent.java > test/jdk/java/lang/instrument/asmlib/Instrumentor.java > > > @asotona Would you mind reviewing? liach has updated the pull request incrementally with one additional commit since the last revision: Shorten lines, move from mask() to ACC_ constants, other misc improvements - Changes: - all: https://git.openjdk.org/jdk/pull/13009/files - new: https://git.openjdk.org/jdk/pull/13009/files/837ea4bb..c6536bf9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13009&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13009&range=00-01 Stats: 196 lines in 19 files changed: 59 ins; 26 del; 111 mod Patch: https://git.openjdk.org/jdk/pull/13009.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13009/head:pull/13009 PR: https://git.openjdk.org/jdk/pull/13009
Re: RFR: 8303921: serviceability/sa/UniqueVtableTest.java timed out [v2]
On Wed, 15 Mar 2023 00:34:00 GMT, Alex Menkov wrote: >> The change: >> - updates UniqueVtableTest to follow standard SA way - attach to target from >> subprocess and use SATestUtils.addPrivilegesIfNeeded for the subprocess; >> - updates several tests in the same directory to resolve >> NoClassDefFoundError failures; It's known JTReg issue that "@build" actions >> for part of used shared classes may cause intermittent NoClassDefFoundError >> in other tests which use the same shared library classpath. >> >> Tested: 100 runs on all platforms, no failures > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > feedback Not sure removing the build directives was the right way to go. As per the jtreg tag guide: > A test that relies upon library classes should contain appropriate @build > directives to ensure that the classes will be compiled. It is strongly > recommended that tests do not rely on the use of implicit compilation by the > Java compiler. so the problem is likely caused by missing build directives in the test(s) that fails. - PR: https://git.openjdk.org/jdk/pull/13030
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v3]
On Tue, 14 Mar 2023 13:59:48 GMT, Matias Saavedra Silva wrote: >> The current structure used to store the resolution information for >> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its >> ambigious fields f1 and f2. This structure can hold information for fields, >> methods, and invokedynamics and each of its fields can hold different types >> of values depending on the entry. >> >> This enhancement proposes a new structure to exclusively contain >> invokedynamic information in a manner that is easy to interpret and easy to >> extend. Resolved invokedynamic entries will be stored in an array in the >> constant pool cache and the operand of the invokedynamic bytecode will be >> rewritten to be the index into this array. >> >> Any areas that previously accessed invokedynamic data from >> ConstantPoolCacheEntry will be replaced with accesses to this new array and >> structure. Verified with tier1-9 tests. >> >> The PPC was provided by @reinrich and the RISCV port was provided by >> @DingliZhang and @zifeihan. >> >> This change supports the following platforms: x86, aarch64, PPC, and RISCV > > Matias Saavedra Silva 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 five additional > commits since the last revision: > > - Typo in comment > - Merge branch 'master' into resolvedIndyEntry_8301995 > - Interpreter optimization and comments > - PPC and RISCV port > - 8301995: Move invokedynamic resolution information out of the cpCache src/hotspot/cpu/aarch64/interp_masm_aarch64.cpp line 1843: > 1841: ldr(cache, Address(rcpool, > in_bytes(ConstantPoolCache::invokedynamic_entries_offset(; > 1842: // Scale the index to be the entry index * > sizeof(ResolvedInvokeDynamicInfo) > 1843: mov(tmp, sizeof(ResolvedIndyEntry)); The tmp register is not used here, is it redundant? - PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v4]
On Tue, 14 Mar 2023 20:20:41 GMT, Matias Saavedra Silva wrote: >> The current structure used to store the resolution information for >> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its >> ambigious fields f1 and f2. This structure can hold information for fields, >> methods, and invokedynamics and each of its fields can hold different types >> of values depending on the entry. >> >> This enhancement proposes a new structure to exclusively contain >> invokedynamic information in a manner that is easy to interpret and easy to >> extend. Resolved invokedynamic entries will be stored in an array in the >> constant pool cache and the operand of the invokedynamic bytecode will be >> rewritten to be the index into this array. >> >> Any areas that previously accessed invokedynamic data from >> ConstantPoolCacheEntry will be replaced with accesses to this new array and >> structure. Verified with tier1-9 tests. >> >> The PPC was provided by @reinrich and the RISCV port was provided by >> @DingliZhang and @zifeihan. >> >> This change supports the following platforms: x86, aarch64, PPC, and RISCV > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > RISCV port update Changes requested by gcao (Author). - PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8291555: Implement alternative fast-locking scheme [v25]
On Tue, 14 Mar 2023 18:46:47 GMT, Roman Kennke wrote: >> I've reviewed the changes in v23 and v24. Trying another >> Mach5 Tier1 job set. > >> I've reviewed the changes in v23 and v24. Trying another Mach5 Tier1 job set. > > I just now pushed a simple change that changes the log message > 'inflate(fast-locked)' to 'inflate(has_locker)' to make those tests happy. @rkennke this still seems to be very much a work-in-progress rather than actual PR review at this stage. Perhaps it should move back to draft until you actually have something you think is ready for integration? - PR: https://git.openjdk.org/jdk/pull/10907
Re: RFR: 8303921: serviceability/sa/UniqueVtableTest.java timed out [v2]
On Wed, 15 Mar 2023 00:34:32 GMT, Alex Menkov wrote: >> "else" part is a sub-process. >> As far as I understand it SATestUtils.skipIfCannotAttach can be skipped for >> "else", but it's needed for main process. > > Added comment. > Left SATestUtils.skipIfCannotAttach as is (this is consistent with other SA > tests) > "else" part is a sub-process. Ok, I read it backwards. Part of the reason I asked for comments. :) - PR: https://git.openjdk.org/jdk/pull/13030
Re: RFR: 8291555: Implement alternative fast-locking scheme [v26]
On Tue, 14 Mar 2023 18:52:39 GMT, Roman Kennke wrote: >> This change adds a fast-locking scheme as an alternative to the current >> stack-locking implementation. It retains the advantages of stack-locking >> (namely fast locking in uncontended code-paths), while avoiding the overload >> of the mark word. That overloading causes massive problems with Lilliput, >> because it means we have to check and deal with this situation when trying >> to access the mark-word. And because of the very racy nature, this turns out >> to be very complex and would involve a variant of the inflation protocol to >> ensure that the object header is stable. (The current implementation of >> setting/fetching the i-hash provides a glimpse into the complexity). >> >> What the original stack-locking does is basically to push a stack-lock onto >> the stack which consists only of the displaced header, and CAS a pointer to >> this stack location into the object header (the lowest two header bits being >> 00 indicate 'stack-locked'). The pointer into the stack can then be used to >> identify which thread currently owns the lock. >> >> This change basically reverses stack-locking: It still CASes the lowest two >> header bits to 00 to indicate 'fast-locked' but does *not* overload the >> upper bits with a stack-pointer. Instead, it pushes the object-reference to >> a thread-local lock-stack. This is a new structure which is basically a >> small array of oops that is associated with each thread. Experience shows >> that this array typcially remains very small (3-5 elements). Using this lock >> stack, it is possible to query which threads own which locks. Most >> importantly, the most common question 'does the current thread own me?' is >> very quickly answered by doing a quick scan of the array. More complex >> queries like 'which thread owns X?' are not performed in very >> performance-critical paths (usually in code like JVMTI or deadlock >> detection) where it is ok to do more complex operations (and we already do). >> The lock-stack is also a new set of GC roots, and would be scanned during >> thread scanning, possibly concurrently, via the normal protocols. >> >> The lock-stack is grown when needed. This means that we need to check for >> potential overflow before attempting locking. When that is the case, locking >> fast-paths would call into the runtime to grow the stack and handle the >> locking. Compiled fast-paths (C1 and C2 on x86_64 and aarch64) do this check >> on method entry to avoid (possibly lots) of such checks at locking sites. >> >> In contrast to stack-locking, fast-locking does *not* support recursive >> locking (yet). When that happens, the fast-lock gets inflated to a full >> monitor. It is not clear if it is worth to add support for recursive >> fast-locking. >> >> One trouble is that when a contending thread arrives at a fast-locked >> object, it must inflate the fast-lock to a full monitor. Normally, we need >> to know the current owning thread, and record that in the monitor, so that >> the contending thread can wait for the current owner to properly exit the >> monitor. However, fast-locking doesn't have this information. What we do >> instead is to record a special marker ANONYMOUS_OWNER. When the thread that >> currently holds the lock arrives at monitorexit, and observes >> ANONYMOUS_OWNER, it knows it must be itself, fixes the owner to be itself, >> and then properly exits the monitor, and thus handing over to the contending >> thread. >> >> As an alternative, I considered to remove stack-locking altogether, and only >> use heavy monitors. In most workloads this did not show measurable >> regressions. However, in a few workloads, I have observed severe >> regressions. All of them have been using old synchronized Java collections >> (Vector, Stack), StringBuffer or similar code. The combination of two >> conditions leads to regressions without stack- or fast-locking: 1. The >> workload synchronizes on uncontended locks (e.g. single-threaded use of >> Vector or StringBuffer) and 2. The workload churns such locks. IOW, >> uncontended use of Vector, StringBuffer, etc as such is ok, but creating >> lots of such single-use, single-threaded-locked objects leads to massive >> ObjectMonitor churn, which can lead to a significant performance impact. But >> alas, such code exists, and we probably don't want to punish it if we can >> avoid it. >> >> This change enables to simplify (and speed-up!) a lot of code: >> >> - The inflation protocol is no longer necessary: we can directly CAS the >> (tagged) ObjectMonitor pointer to the object header. >> - Accessing the hashcode could now be done in the fastpath always, if the >> hashcode has been installed. Fast-locked headers can be used directly, for >> monitor-locked objects we can easily reach-through to the displaced header. >> This is safe because Java threads participate in monitor deflation protocol.
Re: RFR: 8303921: serviceability/sa/UniqueVtableTest.java timed out [v2]
On Tue, 14 Mar 2023 23:41:47 GMT, Alex Menkov wrote:
>> test/hotspot/jtreg/serviceability/sa/UniqueVtableTest.java line 195:
>>
>>> 193: } else {
>>> 194: runTest(Long.parseLong(args[0]));
>>> 195: }
>>
>> Could use some comments here. Also, I think `SATestUtils.skipIfCannotAttach`
>> is only needed for the `else` part.
>
> "else" part is a sub-process.
> As far as I understand it SATestUtils.skipIfCannotAttach can be skipped for
> "else", but it's needed for main process.
Added comment.
Left SATestUtils.skipIfCannotAttach as is (this is consistent with other SA
tests)
-
PR: https://git.openjdk.org/jdk/pull/13030
Re: RFR: 8303921: serviceability/sa/UniqueVtableTest.java timed out [v2]
On Tue, 14 Mar 2023 22:50:32 GMT, Chris Plummer wrote: >> Alex Menkov has updated the pull request incrementally with one additional >> commit since the last revision: >> >> feedback > > test/hotspot/jtreg/serviceability/sa/UniqueVtableTest.java line 158: > >> 156: Long.toString(lingeredAppPid)); >> 157: SATestUtils.addPrivilegesIfNeeded(processBuilder); >> 158: OutputAnalyzer SAOutput = >> ProcessTools.executeProcess(processBuilder); > > `SAOutput`: local variables should start with lower case. Fixed - PR: https://git.openjdk.org/jdk/pull/13030
Re: RFR: 8303921: serviceability/sa/UniqueVtableTest.java timed out [v2]
> The change: > - updates UniqueVtableTest to follow standard SA way - attach to target from > subprocess and use SATestUtils.addPrivilegesIfNeeded for the subprocess; > - updates several tests in the same directory to resolve NoClassDefFoundError > failures; It's known JTReg issue that "@build" actions for part of used > shared classes may cause intermittent NoClassDefFoundError in other tests > which use the same shared library classpath. > > Tested: 100 runs on all platforms, no failures Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: feedback - Changes: - all: https://git.openjdk.org/jdk/pull/13030/files - new: https://git.openjdk.org/jdk/pull/13030/files/69cc6dae..0e8573d8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13030&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13030&range=00-01 Stats: 5 lines in 1 file changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/13030.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13030/head:pull/13030 PR: https://git.openjdk.org/jdk/pull/13030
Re: RFR: 8303921: serviceability/sa/UniqueVtableTest.java timed out
On Tue, 14 Mar 2023 22:48:30 GMT, Chris Plummer wrote:
>> The change:
>> - updates UniqueVtableTest to follow standard SA way - attach to target from
>> subprocess and use SATestUtils.addPrivilegesIfNeeded for the subprocess;
>> - updates several tests in the same directory to resolve
>> NoClassDefFoundError failures; It's known JTReg issue that "@build" actions
>> for part of used shared classes may cause intermittent NoClassDefFoundError
>> in other tests which use the same shared library classpath.
>>
>> Tested: 100 runs on all platforms, no failures
>
> test/hotspot/jtreg/serviceability/sa/UniqueVtableTest.java line 195:
>
>> 193: } else {
>> 194: runTest(Long.parseLong(args[0]));
>> 195: }
>
> Could use some comments here. Also, I think `SATestUtils.skipIfCannotAttach`
> is only needed for the `else` part.
"else" part is a sub-process.
As far as I understand it SATestUtils.skipIfCannotAttach can be skipped for
"else", but it's needed for main process.
-
PR: https://git.openjdk.org/jdk/pull/13030
Re: RFR: 8303921: serviceability/sa/UniqueVtableTest.java timed out
On Tue, 14 Mar 2023 22:49:07 GMT, Chris Plummer wrote:
>> The change:
>> - updates UniqueVtableTest to follow standard SA way - attach to target from
>> subprocess and use SATestUtils.addPrivilegesIfNeeded for the subprocess;
>> - updates several tests in the same directory to resolve
>> NoClassDefFoundError failures; It's known JTReg issue that "@build" actions
>> for part of used shared classes may cause intermittent NoClassDefFoundError
>> in other tests which use the same shared library classpath.
>>
>> Tested: 100 runs on all platforms, no failures
>
> test/hotspot/jtreg/serviceability/sa/UniqueVtableTest.java line 168:
>
>> 166: try {
>> 167: app = LingeredApp.startApp();
>> 168: createAnotherToAttach(app.getPid());
>
> Did you ever figure out why attaching from the main test process sometimes
> fails?
No. It looks like JTReg executes main test process differently than the
sub-process is executed, but I didn't find difference which can cause attach
timeouts.
-
PR: https://git.openjdk.org/jdk/pull/13030
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v4]
On Tue, 14 Mar 2023 20:20:41 GMT, Matias Saavedra Silva
wrote:
>> The current structure used to store the resolution information for
>> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its
>> ambigious fields f1 and f2. This structure can hold information for fields,
>> methods, and invokedynamics and each of its fields can hold different types
>> of values depending on the entry.
>>
>> This enhancement proposes a new structure to exclusively contain
>> invokedynamic information in a manner that is easy to interpret and easy to
>> extend. Resolved invokedynamic entries will be stored in an array in the
>> constant pool cache and the operand of the invokedynamic bytecode will be
>> rewritten to be the index into this array.
>>
>> Any areas that previously accessed invokedynamic data from
>> ConstantPoolCacheEntry will be replaced with accesses to this new array and
>> structure. Verified with tier1-9 tests.
>>
>> The PPC was provided by @reinrich and the RISCV port was provided by
>> @DingliZhang and @zifeihan.
>>
>> This change supports the following platforms: x86, aarch64, PPC, and RISCV
>
> Matias Saavedra Silva has updated the pull request incrementally with one
> additional commit since the last revision:
>
> RISCV port update
Looks good. Just a few minor comments.
src/hotspot/share/interpreter/bootstrapInfo.cpp line 218:
> 216: _indy_index,
> 217:
> pool()->tag_at(_bss_index),
> 218: CHECK_false);
Please indent lines 216-218 like before.
src/hotspot/share/interpreter/bootstrapInfo.cpp line 234:
> 232: if (_indy_index > -1) {
> 233: os::snprintf_checked(what, sizeof(what), "indy#%d", _indy_index);
> 234: }
Since the `else` case doesn’t have braces, maybe omit the braces for this case
as well?
src/hotspot/share/oops/cpCache.cpp line 618:
> 616: indy_resolution_failed(), parameter_size());
> 617: if ((bytecode_1() == Bytecodes::_invokehandle)) {
> 618: constantPoolHandle cph(Thread::current(), cache->constant_pool());
There is another `cph` defined at line 601. Could that one be used?
src/hotspot/share/oops/cpCache.cpp line 652:
> 650: int size = ConstantPoolCache::size(length);
> 651:
> 652: // Initialize resolvedinvokedynamicinfo array with available data
Maybe breakup the long word `resolvedinvokedynamicinfo`?
src/hotspot/share/oops/cpCache.cpp line 653:
> 651:
> 652: // Initialize resolvedinvokedynamicinfo array with available data
> 653: Array* array;
Suggestion: rename `array` to `resolved_indy_entries`.
src/hotspot/share/oops/cpCache.cpp line 664:
> 662:
> 663: return new (loader_data, size, MetaspaceObj::ConstantPoolCacheType,
> THREAD)
> 664: ConstantPoolCache(length, index_map, invokedynamic_map, array);
I think it reads better if this line is indented to right after the open
parenthesis.
src/hotspot/share/prims/methodComparator.cpp line 119:
> 117: if ((old_cp->name_ref_at(index_old) !=
> new_cp->name_ref_at(index_new)) ||
> 118: (old_cp->signature_ref_at(index_old) !=
> new_cp->signature_ref_at(index_new)))
> 119: return false;
Please adjust the indentations of lines 118 and 119 to be the same as lines 124
and 125.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/interpreter/BytecodeWithCPIndex.java
line 61:
> 59: } else {
> 60: return cpCache.getEntryAt((int) (0x &
> cpCacheIndex)).getConstantPoolIndex();
> 61: }
Maybe align all `return` statements with line 56?
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ResolvedIndyArray.java
line 38:
> 36: public class ResolvedIndyArray extends GenericArray {
> 37: static {
> 38: VM.registerVMInitializedObserver(new Observer() {
Indentation for java code should be 4 spaces.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ResolvedIndyEntry.java
line 38:
> 36: private static long size;
> 37: private static long baseOffset;
> 38: private static CIntegerField cpIndex;
Indentation for java code should be 4 spaces.
-
PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8303921: serviceability/sa/UniqueVtableTest.java timed out
On Tue, 14 Mar 2023 23:07:13 GMT, Daniel D. Daugherty wrote: >> The change: >> - updates UniqueVtableTest to follow standard SA way - attach to target from >> subprocess and use SATestUtils.addPrivilegesIfNeeded for the subprocess; >> - updates several tests in the same directory to resolve >> NoClassDefFoundError failures; It's known JTReg issue that "@build" actions >> for part of used shared classes may cause intermittent NoClassDefFoundError >> in other tests which use the same shared library classpath. >> >> Tested: 100 runs on all platforms, no failures > > test/hotspot/jtreg/serviceability/sa/UniqueVtableTest.java line 34: > >> 32: * jdk.hotspot.agent/sun.jvm.hotspot.types.basic >> 33: * >> 34: * @run driver UniqueVtableTest > > The other tests you touched in this PR use: > `@run main/othervm ...` > so why did this one have to change to: > `@run driver ...` Due changes in the test it doesn't need to be run in "othervm" mode, "driver" is ok now to make the test a bit faster I didn't change mode other for other tests - PR: https://git.openjdk.org/jdk/pull/13030
Re: RFR: 8303921: serviceability/sa/UniqueVtableTest.java timed out
On Tue, 14 Mar 2023 22:05:44 GMT, Alex Menkov wrote: > The change: > - updates UniqueVtableTest to follow standard SA way - attach to target from > subprocess and use SATestUtils.addPrivilegesIfNeeded for the subprocess; > - updates several tests in the same directory to resolve NoClassDefFoundError > failures; It's known JTReg issue that "@build" actions for part of used > shared classes may cause intermittent NoClassDefFoundError in other tests > which use the same shared library classpath. > > Tested: 100 runs on all platforms, no failures test/hotspot/jtreg/serviceability/sa/UniqueVtableTest.java line 34: > 32: * jdk.hotspot.agent/sun.jvm.hotspot.types.basic > 33: * > 34: * @run driver UniqueVtableTest The other tests you touched in this PR use: `@run main/othervm ...` so why did this one have to change to: `@run driver ...` - PR: https://git.openjdk.org/jdk/pull/13030
Re: RFR: 8303921: serviceability/sa/UniqueVtableTest.java timed out
On Tue, 14 Mar 2023 22:05:44 GMT, Alex Menkov wrote:
> The change:
> - updates UniqueVtableTest to follow standard SA way - attach to target from
> subprocess and use SATestUtils.addPrivilegesIfNeeded for the subprocess;
> - updates several tests in the same directory to resolve NoClassDefFoundError
> failures; It's known JTReg issue that "@build" actions for part of used
> shared classes may cause intermittent NoClassDefFoundError in other tests
> which use the same shared library classpath.
>
> Tested: 100 runs on all platforms, no failures
Changes requested by cjplummer (Reviewer).
test/hotspot/jtreg/serviceability/sa/UniqueVtableTest.java line 158:
> 156: Long.toString(lingeredAppPid));
> 157: SATestUtils.addPrivilegesIfNeeded(processBuilder);
> 158: OutputAnalyzer SAOutput =
> ProcessTools.executeProcess(processBuilder);
`SAOutput`: local variables should start with lower case.
test/hotspot/jtreg/serviceability/sa/UniqueVtableTest.java line 168:
> 166: try {
> 167: app = LingeredApp.startApp();
> 168: createAnotherToAttach(app.getPid());
Did you ever figure out why attaching from the main test process sometimes
fails?
test/hotspot/jtreg/serviceability/sa/UniqueVtableTest.java line 195:
> 193: } else {
> 194: runTest(Long.parseLong(args[0]));
> 195: }
Could use some comments here. Also, I think `SATestUtils.skipIfCannotAttach` is
only needed for the `else` part.
-
PR: https://git.openjdk.org/jdk/pull/13030
RFR: 8303921: serviceability/sa/UniqueVtableTest.java timed out
The change: - updates UniqueVtableTest to follow standard SA way - attach to target from subprocess and use SATestUtils.addPrivilegesIfNeeded for the subprocess; - updates several tests in the same directory to resolve NoClassDefFoundError failures; It's known JTReg issue that "@build" actions for part of used shared classes may cause intermittent NoClassDefFoundError in other tests which use the same shared library classpath. Tested: 100 runs on all platforms, no failures - Commit messages: - UniqueVtableTest Changes: https://git.openjdk.org/jdk/pull/13030/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13030&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8303921 Stats: 75 lines in 6 files changed: 37 ins; 22 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/13030.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13030/head:pull/13030 PR: https://git.openjdk.org/jdk/pull/13030
Integrated: 8303705: Field sleeper.started should be volatile JdbLockTestTarg.java
On Tue, 14 Mar 2023 03:23:05 GMT, Leonid Mesnik wrote: > Field has been made volatile. This pull request has now been integrated. Changeset: cd41c69d Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/cd41c69d4484f900a89a71f1c9bab2bc2e383c1e Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8303705: Field sleeper.started should be volatile JdbLockTestTarg.java Reviewed-by: dholmes - PR: https://git.openjdk.org/jdk/pull/13010
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
On Tue, 14 Mar 2023 15:10:04 GMT, Doug Simon wrote: >> Access to internal modifiers is needed in >> `HotSpotResolvedJavaFieldTest.testEquivalenceForInternalFields()`. I moved >> the declaration of the method to `HotSpotResolvedJavaField`. Does this >> change work for you? > > Just use reflection to read the internal flags (like this test already does > for the `index` field). > > I've attached > [review.patch](https://github.com/openjdk/jdk/files/10970245/review.patch) > with this change and a few other changes I think should be made for better > naming (plus one test cleanup). Thank you for the patch, it will be included in the next commit. - PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v4]
> The current structure used to store the resolution information for > invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its > ambigious fields f1 and f2. This structure can hold information for fields, > methods, and invokedynamics and each of its fields can hold different types > of values depending on the entry. > > This enhancement proposes a new structure to exclusively contain > invokedynamic information in a manner that is easy to interpret and easy to > extend. Resolved invokedynamic entries will be stored in an array in the > constant pool cache and the operand of the invokedynamic bytecode will be > rewritten to be the index into this array. > > Any areas that previously accessed invokedynamic data from > ConstantPoolCacheEntry will be replaced with accesses to this new array and > structure. Verified with tier1-9 tests. > > The PPC was provided by @reinrich and the RISCV port was provided by > @DingliZhang and @zifeihan. > > This change supports the following platforms: x86, aarch64, PPC, and RISCV Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision: RISCV port update - Changes: - all: https://git.openjdk.org/jdk/pull/12778/files - new: https://git.openjdk.org/jdk/pull/12778/files/a3e7ca02..db892223 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12778&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12778&range=02-03 Stats: 23 lines in 2 files changed: 5 ins; 12 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/12778.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12778/head:pull/12778 PR: https://git.openjdk.org/jdk/pull/12778
Integrated: 8304172: ProblemList serviceability/sa/UniqueVtableTest.java
On Tue, 14 Mar 2023 19:52:01 GMT, Daniel D. Daugherty wrote: > Trivial fixes to ProblemList a couple of tests: > > [JDK-8304172](https://bugs.openjdk.org/browse/JDK-8304172) ProblemList > serviceability/sa/UniqueVtableTest.java > [JDK-8304175](https://bugs.openjdk.org/browse/JDK-8304175) ProblemList > compiler/vectorapi/VectorLogicalOpIdentityTest.java on 2 platforms This pull request has now been integrated. Changeset: 617c15f5 Author:Daniel D. Daugherty URL: https://git.openjdk.org/jdk/commit/617c15f5a131fdf254fc4277f6dd78d64292db1c Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod 8304172: ProblemList serviceability/sa/UniqueVtableTest.java 8304175: ProblemList compiler/vectorapi/VectorLogicalOpIdentityTest.java on 2 platforms Reviewed-by: azvegint - PR: https://git.openjdk.org/jdk/pull/13029
Re: RFR: 8304172: ProblemList serviceability/sa/UniqueVtableTest.java
On Tue, 14 Mar 2023 20:00:30 GMT, Alexander Zvegintsev wrote: >> Trivial fixes to ProblemList a couple of tests: >> >> [JDK-8304172](https://bugs.openjdk.org/browse/JDK-8304172) ProblemList >> serviceability/sa/UniqueVtableTest.java >> [JDK-8304175](https://bugs.openjdk.org/browse/JDK-8304175) ProblemList >> compiler/vectorapi/VectorLogicalOpIdentityTest.java on 2 platforms > > Marked as reviewed by azvegint (Reviewer). @azvegint - Thanks for the fast review! - PR: https://git.openjdk.org/jdk/pull/13029
Re: RFR: 8304172: ProblemList serviceability/sa/UniqueVtableTest.java
On Tue, 14 Mar 2023 19:52:01 GMT, Daniel D. Daugherty wrote: > Trivial fixes to ProblemList a couple of tests: > > [JDK-8304172](https://bugs.openjdk.org/browse/JDK-8304172) ProblemList > serviceability/sa/UniqueVtableTest.java > [JDK-8304175](https://bugs.openjdk.org/browse/JDK-8304175) ProblemList > compiler/vectorapi/VectorLogicalOpIdentityTest.java on 2 platforms Marked as reviewed by azvegint (Reviewer). - PR: https://git.openjdk.org/jdk/pull/13029
RFR: 8304172: ProblemList serviceability/sa/UniqueVtableTest.java
Trivial fixes to ProblemList a couple of tests: [JDK-8304172](https://bugs.openjdk.org/browse/JDK-8304172) ProblemList serviceability/sa/UniqueVtableTest.java [JDK-8304175](https://bugs.openjdk.org/browse/JDK-8304175) ProblemList compiler/vectorapi/VectorLogicalOpIdentityTest.java on 2 platforms - Commit messages: - 8304175: ProblemList compiler/vectorapi/VectorLogicalOpIdentityTest.java on 2 platforms - 8304172: ProblemList serviceability/sa/UniqueVtableTest.java Changes: https://git.openjdk.org/jdk/pull/13029/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13029&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8304172 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13029.diff Fetch: git fetch https://git.openjdk.org/jdk pull/13029/head:pull/13029 PR: https://git.openjdk.org/jdk/pull/13029
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
On Tue, 14 Mar 2023 15:11:36 GMT, Doug Simon wrote: >> Without this declaration, builds fail on Windows with this error: >> `error C2375: 'c2v_getDeclaredFieldsInfo': redefinition; different linkage` > > Strange - thats not needed for other `JVMCIEnv` methods called from > `jvmciCompilerToVM.cpp`. There must be some way to avoid this. The issue was caused by the `friend` declaration below (I cannot remember why I added in the first place), which seems to add an implicit declaration of the method that was conflicting with the original declaration of the method. Once the `friend` declaration is removed, builds on Windows don't need the `extern` declaration anymore. - PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
On Tue, 14 Mar 2023 13:37:23 GMT, Frederic Parain wrote: >> src/hotspot/share/jvmci/jvmciEnv.hpp line 149: >> >>> 147: }; >>> 148: >>> 149: extern JNIEXPORT jobjectArray c2v_getDeclaredFieldsInfo(JNIEnv* env, >>> jobject, jobject, jlong); >> >> What's the purpose of this declaration? I don't think you need it or the >> `friend` declaration below since `new_FieldInfo(FieldInfo* fieldinfo, >> JVMCI_TRAPS)` is public. > > Without this declaration, builds fail on Windows with this error: > `error C2375: 'c2v_getDeclaredFieldsInfo': redefinition; different linkage` Strange - thats not needed for other `JVMCIEnv` methods called from `jvmciCompilerToVM.cpp`. There must be some way to avoid this. - PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8291555: Implement alternative fast-locking scheme [v25]
On Tue, 14 Mar 2023 15:47:29 GMT, Roman Kennke wrote: >> This change adds a fast-locking scheme as an alternative to the current >> stack-locking implementation. It retains the advantages of stack-locking >> (namely fast locking in uncontended code-paths), while avoiding the overload >> of the mark word. That overloading causes massive problems with Lilliput, >> because it means we have to check and deal with this situation when trying >> to access the mark-word. And because of the very racy nature, this turns out >> to be very complex and would involve a variant of the inflation protocol to >> ensure that the object header is stable. (The current implementation of >> setting/fetching the i-hash provides a glimpse into the complexity). >> >> What the original stack-locking does is basically to push a stack-lock onto >> the stack which consists only of the displaced header, and CAS a pointer to >> this stack location into the object header (the lowest two header bits being >> 00 indicate 'stack-locked'). The pointer into the stack can then be used to >> identify which thread currently owns the lock. >> >> This change basically reverses stack-locking: It still CASes the lowest two >> header bits to 00 to indicate 'fast-locked' but does *not* overload the >> upper bits with a stack-pointer. Instead, it pushes the object-reference to >> a thread-local lock-stack. This is a new structure which is basically a >> small array of oops that is associated with each thread. Experience shows >> that this array typcially remains very small (3-5 elements). Using this lock >> stack, it is possible to query which threads own which locks. Most >> importantly, the most common question 'does the current thread own me?' is >> very quickly answered by doing a quick scan of the array. More complex >> queries like 'which thread owns X?' are not performed in very >> performance-critical paths (usually in code like JVMTI or deadlock >> detection) where it is ok to do more complex operations (and we already do). >> The lock-stack is also a new set of GC roots, and would be scanned during >> thread scanning, possibly concurrently, via the normal protocols. >> >> The lock-stack is grown when needed. This means that we need to check for >> potential overflow before attempting locking. When that is the case, locking >> fast-paths would call into the runtime to grow the stack and handle the >> locking. Compiled fast-paths (C1 and C2 on x86_64 and aarch64) do this check >> on method entry to avoid (possibly lots) of such checks at locking sites. >> >> In contrast to stack-locking, fast-locking does *not* support recursive >> locking (yet). When that happens, the fast-lock gets inflated to a full >> monitor. It is not clear if it is worth to add support for recursive >> fast-locking. >> >> One trouble is that when a contending thread arrives at a fast-locked >> object, it must inflate the fast-lock to a full monitor. Normally, we need >> to know the current owning thread, and record that in the monitor, so that >> the contending thread can wait for the current owner to properly exit the >> monitor. However, fast-locking doesn't have this information. What we do >> instead is to record a special marker ANONYMOUS_OWNER. When the thread that >> currently holds the lock arrives at monitorexit, and observes >> ANONYMOUS_OWNER, it knows it must be itself, fixes the owner to be itself, >> and then properly exits the monitor, and thus handing over to the contending >> thread. >> >> As an alternative, I considered to remove stack-locking altogether, and only >> use heavy monitors. In most workloads this did not show measurable >> regressions. However, in a few workloads, I have observed severe >> regressions. All of them have been using old synchronized Java collections >> (Vector, Stack), StringBuffer or similar code. The combination of two >> conditions leads to regressions without stack- or fast-locking: 1. The >> workload synchronizes on uncontended locks (e.g. single-threaded use of >> Vector or StringBuffer) and 2. The workload churns such locks. IOW, >> uncontended use of Vector, StringBuffer, etc as such is ok, but creating >> lots of such single-use, single-threaded-locked objects leads to massive >> ObjectMonitor churn, which can lead to a significant performance impact. But >> alas, such code exists, and we probably don't want to punish it if we can >> avoid it. >> >> This change enables to simplify (and speed-up!) a lot of code: >> >> - The inflation protocol is no longer necessary: we can directly CAS the >> (tagged) ObjectMonitor pointer to the object header. >> - Accessing the hashcode could now be done in the fastpath always, if the >> hashcode has been installed. Fast-locked headers can be used directly, for >> monitor-locked objects we can easily reach-through to the displaced header. >> This is safe because Java threads participate in monitor deflation protocol.
Re: RFR: 8291555: Implement alternative fast-locking scheme [v25]
On Tue, 14 Mar 2023 18:41:59 GMT, Daniel D. Daugherty wrote: > I've reviewed the changes in v23 and v24. Trying another Mach5 Tier1 job set. I just now pushed a simple change that changes the log message 'inflate(fast-locked)' to 'inflate(has_locker)' to make those tests happy. - PR: https://git.openjdk.org/jdk/pull/10907
Re: RFR: 8291555: Implement alternative fast-locking scheme [v26]
> This change adds a fast-locking scheme as an alternative to the current > stack-locking implementation. It retains the advantages of stack-locking > (namely fast locking in uncontended code-paths), while avoiding the overload > of the mark word. That overloading causes massive problems with Lilliput, > because it means we have to check and deal with this situation when trying to > access the mark-word. And because of the very racy nature, this turns out to > be very complex and would involve a variant of the inflation protocol to > ensure that the object header is stable. (The current implementation of > setting/fetching the i-hash provides a glimpse into the complexity). > > What the original stack-locking does is basically to push a stack-lock onto > the stack which consists only of the displaced header, and CAS a pointer to > this stack location into the object header (the lowest two header bits being > 00 indicate 'stack-locked'). The pointer into the stack can then be used to > identify which thread currently owns the lock. > > This change basically reverses stack-locking: It still CASes the lowest two > header bits to 00 to indicate 'fast-locked' but does *not* overload the upper > bits with a stack-pointer. Instead, it pushes the object-reference to a > thread-local lock-stack. This is a new structure which is basically a small > array of oops that is associated with each thread. Experience shows that this > array typcially remains very small (3-5 elements). Using this lock stack, it > is possible to query which threads own which locks. Most importantly, the > most common question 'does the current thread own me?' is very quickly > answered by doing a quick scan of the array. More complex queries like 'which > thread owns X?' are not performed in very performance-critical paths (usually > in code like JVMTI or deadlock detection) where it is ok to do more complex > operations (and we already do). The lock-stack is also a new set of GC roots, > and would be scanned during thread scanning, possibly concurrently, via the > normal p rotocols. > > The lock-stack is grown when needed. This means that we need to check for > potential overflow before attempting locking. When that is the case, locking > fast-paths would call into the runtime to grow the stack and handle the > locking. Compiled fast-paths (C1 and C2 on x86_64 and aarch64) do this check > on method entry to avoid (possibly lots) of such checks at locking sites. > > In contrast to stack-locking, fast-locking does *not* support recursive > locking (yet). When that happens, the fast-lock gets inflated to a full > monitor. It is not clear if it is worth to add support for recursive > fast-locking. > > One trouble is that when a contending thread arrives at a fast-locked object, > it must inflate the fast-lock to a full monitor. Normally, we need to know > the current owning thread, and record that in the monitor, so that the > contending thread can wait for the current owner to properly exit the > monitor. However, fast-locking doesn't have this information. What we do > instead is to record a special marker ANONYMOUS_OWNER. When the thread that > currently holds the lock arrives at monitorexit, and observes > ANONYMOUS_OWNER, it knows it must be itself, fixes the owner to be itself, > and then properly exits the monitor, and thus handing over to the contending > thread. > > As an alternative, I considered to remove stack-locking altogether, and only > use heavy monitors. In most workloads this did not show measurable > regressions. However, in a few workloads, I have observed severe regressions. > All of them have been using old synchronized Java collections (Vector, > Stack), StringBuffer or similar code. The combination of two conditions leads > to regressions without stack- or fast-locking: 1. The workload synchronizes > on uncontended locks (e.g. single-threaded use of Vector or StringBuffer) and > 2. The workload churns such locks. IOW, uncontended use of Vector, > StringBuffer, etc as such is ok, but creating lots of such single-use, > single-threaded-locked objects leads to massive ObjectMonitor churn, which > can lead to a significant performance impact. But alas, such code exists, and > we probably don't want to punish it if we can avoid it. > > This change enables to simplify (and speed-up!) a lot of code: > > - The inflation protocol is no longer necessary: we can directly CAS the > (tagged) ObjectMonitor pointer to the object header. > - Accessing the hashcode could now be done in the fastpath always, if the > hashcode has been installed. Fast-locked headers can be used directly, for > monitor-locked objects we can easily reach-through to the displaced header. > This is safe because Java threads participate in monitor deflation protocol. > This would be implemented in a separate PR > > > Testing: > - [x] tier1 x86_64 x aarch64 x +UseFastLocking > - [x] tier2 x86_6
Re: RFR: 8291555: Implement alternative fast-locking scheme [v25]
On Tue, 14 Mar 2023 15:47:29 GMT, Roman Kennke wrote: >> This change adds a fast-locking scheme as an alternative to the current >> stack-locking implementation. It retains the advantages of stack-locking >> (namely fast locking in uncontended code-paths), while avoiding the overload >> of the mark word. That overloading causes massive problems with Lilliput, >> because it means we have to check and deal with this situation when trying >> to access the mark-word. And because of the very racy nature, this turns out >> to be very complex and would involve a variant of the inflation protocol to >> ensure that the object header is stable. (The current implementation of >> setting/fetching the i-hash provides a glimpse into the complexity). >> >> What the original stack-locking does is basically to push a stack-lock onto >> the stack which consists only of the displaced header, and CAS a pointer to >> this stack location into the object header (the lowest two header bits being >> 00 indicate 'stack-locked'). The pointer into the stack can then be used to >> identify which thread currently owns the lock. >> >> This change basically reverses stack-locking: It still CASes the lowest two >> header bits to 00 to indicate 'fast-locked' but does *not* overload the >> upper bits with a stack-pointer. Instead, it pushes the object-reference to >> a thread-local lock-stack. This is a new structure which is basically a >> small array of oops that is associated with each thread. Experience shows >> that this array typcially remains very small (3-5 elements). Using this lock >> stack, it is possible to query which threads own which locks. Most >> importantly, the most common question 'does the current thread own me?' is >> very quickly answered by doing a quick scan of the array. More complex >> queries like 'which thread owns X?' are not performed in very >> performance-critical paths (usually in code like JVMTI or deadlock >> detection) where it is ok to do more complex operations (and we already do). >> The lock-stack is also a new set of GC roots, and would be scanned during >> thread scanning, possibly concurrently, via the normal protocols. >> >> The lock-stack is grown when needed. This means that we need to check for >> potential overflow before attempting locking. When that is the case, locking >> fast-paths would call into the runtime to grow the stack and handle the >> locking. Compiled fast-paths (C1 and C2 on x86_64 and aarch64) do this check >> on method entry to avoid (possibly lots) of such checks at locking sites. >> >> In contrast to stack-locking, fast-locking does *not* support recursive >> locking (yet). When that happens, the fast-lock gets inflated to a full >> monitor. It is not clear if it is worth to add support for recursive >> fast-locking. >> >> One trouble is that when a contending thread arrives at a fast-locked >> object, it must inflate the fast-lock to a full monitor. Normally, we need >> to know the current owning thread, and record that in the monitor, so that >> the contending thread can wait for the current owner to properly exit the >> monitor. However, fast-locking doesn't have this information. What we do >> instead is to record a special marker ANONYMOUS_OWNER. When the thread that >> currently holds the lock arrives at monitorexit, and observes >> ANONYMOUS_OWNER, it knows it must be itself, fixes the owner to be itself, >> and then properly exits the monitor, and thus handing over to the contending >> thread. >> >> As an alternative, I considered to remove stack-locking altogether, and only >> use heavy monitors. In most workloads this did not show measurable >> regressions. However, in a few workloads, I have observed severe >> regressions. All of them have been using old synchronized Java collections >> (Vector, Stack), StringBuffer or similar code. The combination of two >> conditions leads to regressions without stack- or fast-locking: 1. The >> workload synchronizes on uncontended locks (e.g. single-threaded use of >> Vector or StringBuffer) and 2. The workload churns such locks. IOW, >> uncontended use of Vector, StringBuffer, etc as such is ok, but creating >> lots of such single-use, single-threaded-locked objects leads to massive >> ObjectMonitor churn, which can lead to a significant performance impact. But >> alas, such code exists, and we probably don't want to punish it if we can >> avoid it. >> >> This change enables to simplify (and speed-up!) a lot of code: >> >> - The inflation protocol is no longer necessary: we can directly CAS the >> (tagged) ObjectMonitor pointer to the object header. >> - Accessing the hashcode could now be done in the fastpath always, if the >> hashcode has been installed. Fast-locked headers can be used directly, for >> monitor-locked objects we can easily reach-through to the displaced header. >> This is safe because Java threads participate in monitor deflation protocol.
Re: RFR: 8291555: Implement alternative fast-locking scheme [v25]
On Tue, 14 Mar 2023 18:16:36 GMT, Daniel D. Daugherty wrote: > I kicked off a round of Mach5 Tier1 testing last night. I got 133 SA test > failures that are probably fixed by v24. > runtime/logging/MonitorInflationTest.java also failed on all 5 configs tested > in Tier1: > > ``` > java.lang.RuntimeException: 'inflate(has_locker):' missing from stdout/stderr > at > jdk.test.lib.process.OutputAnalyzer.shouldContain(OutputAnalyzer.java:221) > at MonitorInflationTest.analyzeOutputOn(MonitorInflationTest.java:41) > at MonitorInflationTest.main(MonitorInflationTest.java:56) > at > java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103) > at java.base/java.lang.reflect.Method.invoke(Method.java:578) > at > com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:312) > at java.base/java.lang.Thread.run(Thread.java:1623) > ``` > > I suspect that the failing condition is one that I added to the test a long > time ago so I'll be taking a look. Aww, that is bad timing. I pushed a change yesterday that broke SA, and I only pushed a fix 2 hours ago. It should be good now, in case you want to try it again. Thank you for your effort to review and test this change! Roman - PR: https://git.openjdk.org/jdk/pull/10907
Re: RFR: 8291555: Implement alternative fast-locking scheme [v25]
On Tue, 14 Mar 2023 15:47:29 GMT, Roman Kennke wrote: >> This change adds a fast-locking scheme as an alternative to the current >> stack-locking implementation. It retains the advantages of stack-locking >> (namely fast locking in uncontended code-paths), while avoiding the overload >> of the mark word. That overloading causes massive problems with Lilliput, >> because it means we have to check and deal with this situation when trying >> to access the mark-word. And because of the very racy nature, this turns out >> to be very complex and would involve a variant of the inflation protocol to >> ensure that the object header is stable. (The current implementation of >> setting/fetching the i-hash provides a glimpse into the complexity). >> >> What the original stack-locking does is basically to push a stack-lock onto >> the stack which consists only of the displaced header, and CAS a pointer to >> this stack location into the object header (the lowest two header bits being >> 00 indicate 'stack-locked'). The pointer into the stack can then be used to >> identify which thread currently owns the lock. >> >> This change basically reverses stack-locking: It still CASes the lowest two >> header bits to 00 to indicate 'fast-locked' but does *not* overload the >> upper bits with a stack-pointer. Instead, it pushes the object-reference to >> a thread-local lock-stack. This is a new structure which is basically a >> small array of oops that is associated with each thread. Experience shows >> that this array typcially remains very small (3-5 elements). Using this lock >> stack, it is possible to query which threads own which locks. Most >> importantly, the most common question 'does the current thread own me?' is >> very quickly answered by doing a quick scan of the array. More complex >> queries like 'which thread owns X?' are not performed in very >> performance-critical paths (usually in code like JVMTI or deadlock >> detection) where it is ok to do more complex operations (and we already do). >> The lock-stack is also a new set of GC roots, and would be scanned during >> thread scanning, possibly concurrently, via the normal protocols. >> >> The lock-stack is grown when needed. This means that we need to check for >> potential overflow before attempting locking. When that is the case, locking >> fast-paths would call into the runtime to grow the stack and handle the >> locking. Compiled fast-paths (C1 and C2 on x86_64 and aarch64) do this check >> on method entry to avoid (possibly lots) of such checks at locking sites. >> >> In contrast to stack-locking, fast-locking does *not* support recursive >> locking (yet). When that happens, the fast-lock gets inflated to a full >> monitor. It is not clear if it is worth to add support for recursive >> fast-locking. >> >> One trouble is that when a contending thread arrives at a fast-locked >> object, it must inflate the fast-lock to a full monitor. Normally, we need >> to know the current owning thread, and record that in the monitor, so that >> the contending thread can wait for the current owner to properly exit the >> monitor. However, fast-locking doesn't have this information. What we do >> instead is to record a special marker ANONYMOUS_OWNER. When the thread that >> currently holds the lock arrives at monitorexit, and observes >> ANONYMOUS_OWNER, it knows it must be itself, fixes the owner to be itself, >> and then properly exits the monitor, and thus handing over to the contending >> thread. >> >> As an alternative, I considered to remove stack-locking altogether, and only >> use heavy monitors. In most workloads this did not show measurable >> regressions. However, in a few workloads, I have observed severe >> regressions. All of them have been using old synchronized Java collections >> (Vector, Stack), StringBuffer or similar code. The combination of two >> conditions leads to regressions without stack- or fast-locking: 1. The >> workload synchronizes on uncontended locks (e.g. single-threaded use of >> Vector or StringBuffer) and 2. The workload churns such locks. IOW, >> uncontended use of Vector, StringBuffer, etc as such is ok, but creating >> lots of such single-use, single-threaded-locked objects leads to massive >> ObjectMonitor churn, which can lead to a significant performance impact. But >> alas, such code exists, and we probably don't want to punish it if we can >> avoid it. >> >> This change enables to simplify (and speed-up!) a lot of code: >> >> - The inflation protocol is no longer necessary: we can directly CAS the >> (tagged) ObjectMonitor pointer to the object header. >> - Accessing the hashcode could now be done in the fastpath always, if the >> hashcode has been installed. Fast-locked headers can be used directly, for >> monitor-locked objects we can easily reach-through to the displaced header. >> This is safe because Java threads participate in monitor deflation protocol.
Re: RFR: 8303814: getLastErrorString should avoid charset conversions [v3]
On Mon, 13 Mar 2023 15:55:27 GMT, Daniel Jeliński wrote:
>> This patch modifies the `getLastErrorString` method to return a `jstring`.
>> Thanks to that we can avoid unnecessary back and forth conversions between
>> Unicode and other charsets on Windows.
>>
>> Other changes include:
>> - the Windows implementation of `getLastErrorString` no longer checks
>> `errno`. I verified all uses of the method and confirmed that `errno` is not
>> used anywhere.
>> - While at it, I found and fixed a few calls to
>> `JNU_ThrowIOExceptionWithLastError` that were done in context where
>> `LastError` was not set.
>> - jdk.hotspot.agent was modified to use `JNU_ThrowByNameWithLastError` and
>> `JNU_ThrowByName` instead of `getLastErrorString`; the code is expected to
>> have identical behavior.
>> - zip_util was modified to return static messages instead of generated ones.
>> The generated messages were not observed anywhere, because they were
>> replaced by a static message in ZIP_Open, which is the only method used by
>> other native code.
>> - `getLastErrorString` is no longer exported by libjava.
>>
>> Tier1-3 tests continue to pass.
>>
>> No new automated regression test; testing this requires installing a
>> language pack that cannot be displayed in the current code page.
>> Tested this manually by installing Chinese language pack on English Windows
>> 11, selecting Chinese language, then checking if the message on exception
>> thrown by `InetAddress.getByName("nonexistent.local");` starts with
>> `"不知道这样的主机。"` (or
>> `"\u4e0d\u77e5\u9053\u8fd9\u6837\u7684\u4e3b\u673a\u3002"`). Without the
>> change, the exception message started with a row of question marks.
>
> Daniel Jeliński has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Use NULL where appropriate
we never use any of the JNU_XXX functions to report errno on Windows as far as
I could tell. And even if we did, we'd need to call SetLastError(0) before
JNU_Throw to make it work, which we never did. I think we're ok here.
-
PR: https://git.openjdk.org/jdk/pull/12922
Re: RFR: 8303814: getLastErrorString should avoid charset conversions [v3]
On Mon, 13 Mar 2023 15:55:27 GMT, Daniel Jeliński wrote:
>> This patch modifies the `getLastErrorString` method to return a `jstring`.
>> Thanks to that we can avoid unnecessary back and forth conversions between
>> Unicode and other charsets on Windows.
>>
>> Other changes include:
>> - the Windows implementation of `getLastErrorString` no longer checks
>> `errno`. I verified all uses of the method and confirmed that `errno` is not
>> used anywhere.
>> - While at it, I found and fixed a few calls to
>> `JNU_ThrowIOExceptionWithLastError` that were done in context where
>> `LastError` was not set.
>> - jdk.hotspot.agent was modified to use `JNU_ThrowByNameWithLastError` and
>> `JNU_ThrowByName` instead of `getLastErrorString`; the code is expected to
>> have identical behavior.
>> - zip_util was modified to return static messages instead of generated ones.
>> The generated messages were not observed anywhere, because they were
>> replaced by a static message in ZIP_Open, which is the only method used by
>> other native code.
>> - `getLastErrorString` is no longer exported by libjava.
>>
>> Tier1-3 tests continue to pass.
>>
>> No new automated regression test; testing this requires installing a
>> language pack that cannot be displayed in the current code page.
>> Tested this manually by installing Chinese language pack on English Windows
>> 11, selecting Chinese language, then checking if the message on exception
>> thrown by `InetAddress.getByName("nonexistent.local");` starts with
>> `"不知道这样的主机。"` (or
>> `"\u4e0d\u77e5\u9053\u8fd9\u6837\u7684\u4e3b\u673a\u3002"`). Without the
>> change, the exception message started with a row of question marks.
>
> Daniel Jeliński has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Use NULL where appropriate
The change in Windows behaviour seems like a worrying gotcha that someone using
the method might be trapped by (C library errors and system errors are reported
for Unix while only WINAPI is returned on Windows). Although this has already
been pushed I'd still like to mention that me and Thomas did have quite a bit
of discussion regarding the conundrum on Windows about whether to report both
when the error routines are called or have separate methods/mechanism to select
either, see [8292016](https://bugs.openjdk.org/browse/JDK-8292016?filter=-1)
-
PR: https://git.openjdk.org/jdk/pull/12922
Integrated: 8303814: getLastErrorString should avoid charset conversions
On Wed, 8 Mar 2023 11:30:27 GMT, Daniel Jeliński wrote:
> This patch modifies the `getLastErrorString` method to return a `jstring`.
> Thanks to that we can avoid unnecessary back and forth conversions between
> Unicode and other charsets on Windows.
>
> Other changes include:
> - the Windows implementation of `getLastErrorString` no longer checks
> `errno`. I verified all uses of the method and confirmed that `errno` is not
> used anywhere.
> - While at it, I found and fixed a few calls to
> `JNU_ThrowIOExceptionWithLastError` that were done in context where
> `LastError` was not set.
> - jdk.hotspot.agent was modified to use `JNU_ThrowByNameWithLastError` and
> `JNU_ThrowByName` instead of `getLastErrorString`; the code is expected to
> have identical behavior.
> - zip_util was modified to return static messages instead of generated ones.
> The generated messages were not observed anywhere, because they were replaced
> by a static message in ZIP_Open, which is the only method used by other
> native code.
> - `getLastErrorString` is no longer exported by libjava.
>
> Tier1-3 tests continue to pass.
>
> No new automated regression test; testing this requires installing a language
> pack that cannot be displayed in the current code page.
> Tested this manually by installing Chinese language pack on English Windows
> 11, selecting Chinese language, then checking if the message on exception
> thrown by `InetAddress.getByName("nonexistent.local");` starts with
> `"不知道这样的主机。"` (or
> `"\u4e0d\u77e5\u9053\u8fd9\u6837\u7684\u4e3b\u673a\u3002"`). Without the
> change, the exception message started with a row of question marks.
This pull request has now been integrated.
Changeset: baf11e73
Author:Daniel Jeliński
URL:
https://git.openjdk.org/jdk/commit/baf11e734f7b5308490edc74f3168744c0857b24
Stats: 151 lines in 8 files changed: 21 ins; 44 del; 86 mod
8303814: getLastErrorString should avoid charset conversions
Reviewed-by: naoto, cjplummer, rriggs
-
PR: https://git.openjdk.org/jdk/pull/12922
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v2]
On Tue, 14 Mar 2023 09:20:54 GMT, Richard Reingruber wrote: > @matias9927 can I ask you to merge master? There seem to be conflicts (at > least I see a message "This branch has conflicts that must be resolved"). I'd > like to give the change a spin in our CI testing. This requires that it can > be applied on master. I saw that merge error but nothing came up when I tried to merge locally. The branch is updated nonetheless, so you should be able to test it now @reinrich ! - PR: https://git.openjdk.org/jdk/pull/12778
Integrated: 8298966: Deprecate JMX Subject Delegation and the method JMXConnector.getMBeanServerConnection(Subject) for removal.
On Fri, 6 Jan 2023 12:02:37 GMT, Kevin Walls wrote: > Deprecate the Java Management Extension (JMX) Subject Delegation feature for > removal in a future release. > > Given no known usage, there is no replacement feature for JMX Subject > Delegation. > > CSR is https://bugs.openjdk.org/browse/JDK-8298967 This pull request has now been integrated. Changeset: 4e631fa4 Author:Kevin Walls URL: https://git.openjdk.org/jdk/commit/4e631fa43fd821846c12ae2177360c44cf770766 Stats: 9 lines in 2 files changed: 7 ins; 0 del; 2 mod 8298966: Deprecate JMX Subject Delegation and the method JMXConnector.getMBeanServerConnection(Subject) for removal. Reviewed-by: mchung, dfuchs - PR: https://git.openjdk.org/jdk/pull/11880
Re: RFR: 8298966: Deprecate JMX Subject Delegation and the method JMXConnector.getMBeanServerConnection(Subject) for removal. [v2]
On Fri, 3 Mar 2023 11:46:49 GMT, Kevin Walls wrote: >> Deprecate the Java Management Extension (JMX) Subject Delegation feature for >> removal in a future release. >> >> Given no known usage, there is no replacement feature for JMX Subject >> Delegation. >> >> CSR is https://bugs.openjdk.org/browse/JDK-8298967 > > Kevin Walls 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 five additional > commits since the last revision: > > - deprecation text update > - Revert "RMIConnection throw comments" > >This reverts commit aceb4fe44189245ac702f0c74c2bb1100a6d17fa. > - Merge remote-tracking branch 'upstream/master' into > Deprecate_SubjectDelegation > - RMIConnection throw comments > - 8298966: Deprecate JMX Subject Delegation and the method > JMXConnector.getMBeanServerConnection(Subject) for removal. Thanks Mandy and Daniel! - PR: https://git.openjdk.org/jdk/pull/11880
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v3]
On Tue, 14 Mar 2023 13:59:48 GMT, Matias Saavedra Silva wrote: >> The current structure used to store the resolution information for >> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its >> ambigious fields f1 and f2. This structure can hold information for fields, >> methods, and invokedynamics and each of its fields can hold different types >> of values depending on the entry. >> >> This enhancement proposes a new structure to exclusively contain >> invokedynamic information in a manner that is easy to interpret and easy to >> extend. Resolved invokedynamic entries will be stored in an array in the >> constant pool cache and the operand of the invokedynamic bytecode will be >> rewritten to be the index into this array. >> >> Any areas that previously accessed invokedynamic data from >> ConstantPoolCacheEntry will be replaced with accesses to this new array and >> structure. Verified with tier1-9 tests. >> >> The PPC was provided by @reinrich and the RISCV port was provided by >> @DingliZhang and @zifeihan. >> >> This change supports the following platforms: x86, aarch64, PPC, and RISCV > > Matias Saavedra Silva 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 five additional > commits since the last revision: > > - Typo in comment > - Merge branch 'master' into resolvedIndyEntry_8301995 > - Interpreter optimization and comments > - PPC and RISCV port > - 8301995: Move invokedynamic resolution information out of the cpCache Hi, I have updated the riscv related code by referring to the latest aarch64 related changes, please help me to update it. https://github.com/zifeihan/jdk/commit/ca9f110ca4eb066f828442265f43ed0d9311a9cc (on this branch: https://github.com/zifeihan/jdk/commits/follow_12778) @RealFYang @DingliZhang Please help review the RISCV port code. - PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8291555: Implement alternative fast-locking scheme [v25]
> This change adds a fast-locking scheme as an alternative to the current > stack-locking implementation. It retains the advantages of stack-locking > (namely fast locking in uncontended code-paths), while avoiding the overload > of the mark word. That overloading causes massive problems with Lilliput, > because it means we have to check and deal with this situation when trying to > access the mark-word. And because of the very racy nature, this turns out to > be very complex and would involve a variant of the inflation protocol to > ensure that the object header is stable. (The current implementation of > setting/fetching the i-hash provides a glimpse into the complexity). > > What the original stack-locking does is basically to push a stack-lock onto > the stack which consists only of the displaced header, and CAS a pointer to > this stack location into the object header (the lowest two header bits being > 00 indicate 'stack-locked'). The pointer into the stack can then be used to > identify which thread currently owns the lock. > > This change basically reverses stack-locking: It still CASes the lowest two > header bits to 00 to indicate 'fast-locked' but does *not* overload the upper > bits with a stack-pointer. Instead, it pushes the object-reference to a > thread-local lock-stack. This is a new structure which is basically a small > array of oops that is associated with each thread. Experience shows that this > array typcially remains very small (3-5 elements). Using this lock stack, it > is possible to query which threads own which locks. Most importantly, the > most common question 'does the current thread own me?' is very quickly > answered by doing a quick scan of the array. More complex queries like 'which > thread owns X?' are not performed in very performance-critical paths (usually > in code like JVMTI or deadlock detection) where it is ok to do more complex > operations (and we already do). The lock-stack is also a new set of GC roots, > and would be scanned during thread scanning, possibly concurrently, via the > normal p rotocols. > > The lock-stack is grown when needed. This means that we need to check for > potential overflow before attempting locking. When that is the case, locking > fast-paths would call into the runtime to grow the stack and handle the > locking. Compiled fast-paths (C1 and C2 on x86_64 and aarch64) do this check > on method entry to avoid (possibly lots) of such checks at locking sites. > > In contrast to stack-locking, fast-locking does *not* support recursive > locking (yet). When that happens, the fast-lock gets inflated to a full > monitor. It is not clear if it is worth to add support for recursive > fast-locking. > > One trouble is that when a contending thread arrives at a fast-locked object, > it must inflate the fast-lock to a full monitor. Normally, we need to know > the current owning thread, and record that in the monitor, so that the > contending thread can wait for the current owner to properly exit the > monitor. However, fast-locking doesn't have this information. What we do > instead is to record a special marker ANONYMOUS_OWNER. When the thread that > currently holds the lock arrives at monitorexit, and observes > ANONYMOUS_OWNER, it knows it must be itself, fixes the owner to be itself, > and then properly exits the monitor, and thus handing over to the contending > thread. > > As an alternative, I considered to remove stack-locking altogether, and only > use heavy monitors. In most workloads this did not show measurable > regressions. However, in a few workloads, I have observed severe regressions. > All of them have been using old synchronized Java collections (Vector, > Stack), StringBuffer or similar code. The combination of two conditions leads > to regressions without stack- or fast-locking: 1. The workload synchronizes > on uncontended locks (e.g. single-threaded use of Vector or StringBuffer) and > 2. The workload churns such locks. IOW, uncontended use of Vector, > StringBuffer, etc as such is ok, but creating lots of such single-use, > single-threaded-locked objects leads to massive ObjectMonitor churn, which > can lead to a significant performance impact. But alas, such code exists, and > we probably don't want to punish it if we can avoid it. > > This change enables to simplify (and speed-up!) a lot of code: > > - The inflation protocol is no longer necessary: we can directly CAS the > (tagged) ObjectMonitor pointer to the object header. > - Accessing the hashcode could now be done in the fastpath always, if the > hashcode has been installed. Fast-locked headers can be used directly, for > monitor-locked objects we can easily reach-through to the displaced header. > This is safe because Java threads participate in monitor deflation protocol. > This would be implemented in a separate PR > > > Testing: > - [x] tier1 x86_64 x aarch64 x +UseFastLocking > - [x] tier2 x86_6
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
On Tue, 14 Mar 2023 13:12:31 GMT, Frederic Parain wrote: >> src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/meta/ResolvedJavaField.java >> line 48: >> >>> 46: * Returns VM internal flags associated with this field >>> 47: */ >>> 48: int getInternalModifiers(); >> >> We've never exposed the internal modifiers before in public JVMCI API and we >> should refrain from doing so until there's a good reason to do so. Please >> remove this method. > > Access to internal modifiers is needed in > `HotSpotResolvedJavaFieldTest.testEquivalenceForInternalFields()`. I moved > the declaration of the method to `HotSpotResolvedJavaField`. Does this change > work for you? Just use reflection to read the internal flags (like this test already does for the `index` field). I've attached [review.patch](https://github.com/openjdk/jdk/files/10970245/review.patch) with this change and a few other changes I think should be made for better naming (plus one test cleanup). - PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8298966: Deprecate JMX Subject Delegation and the method JMXConnector.getMBeanServerConnection(Subject) for removal. [v2]
On Fri, 3 Mar 2023 11:46:49 GMT, Kevin Walls wrote: >> Deprecate the Java Management Extension (JMX) Subject Delegation feature for >> removal in a future release. >> >> Given no known usage, there is no replacement feature for JMX Subject >> Delegation. >> >> CSR is https://bugs.openjdk.org/browse/JDK-8298967 > > Kevin Walls 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 five additional > commits since the last revision: > > - deprecation text update > - Revert "RMIConnection throw comments" > >This reverts commit aceb4fe44189245ac702f0c74c2bb1100a6d17fa. > - Merge remote-tracking branch 'upstream/master' into > Deprecate_SubjectDelegation > - RMIConnection throw comments > - 8298966: Deprecate JMX Subject Delegation and the method > JMXConnector.getMBeanServerConnection(Subject) for removal. Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11880
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v2]
On Tue, 14 Mar 2023 13:18:40 GMT, Coleen Phillimore wrote: > Ok, never mind, I saw s390 port but it doesn't seem to be in these changes (?) It is not in these changes. @offamitkumar is working on s390x. It is not yet finished though. (I wasn't aware that putting the URL of this PR into a comment somewhere else adds a comment to this PR) - PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v3]
> The current structure used to store the resolution information for > invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its > ambigious fields f1 and f2. This structure can hold information for fields, > methods, and invokedynamics and each of its fields can hold different types > of values depending on the entry. > > This enhancement proposes a new structure to exclusively contain > invokedynamic information in a manner that is easy to interpret and easy to > extend. Resolved invokedynamic entries will be stored in an array in the > constant pool cache and the operand of the invokedynamic bytecode will be > rewritten to be the index into this array. > > Any areas that previously accessed invokedynamic data from > ConstantPoolCacheEntry will be replaced with accesses to this new array and > structure. Verified with tier1-9 tests. > > The PPC was provided by @reinrich and the RISCV port was provided by > @DingliZhang and @zifeihan. > > This change supports the following platforms: x86, aarch64, PPC, and RISCV Matias Saavedra Silva 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 five additional commits since the last revision: - Typo in comment - Merge branch 'master' into resolvedIndyEntry_8301995 - Interpreter optimization and comments - PPC and RISCV port - 8301995: Move invokedynamic resolution information out of the cpCache - Changes: - all: https://git.openjdk.org/jdk/pull/12778/files - new: https://git.openjdk.org/jdk/pull/12778/files/c2d87e59..a3e7ca02 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12778&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12778&range=01-02 Stats: 92608 lines in 1481 files changed: 72908 ins; 8825 del; 10875 mod Patch: https://git.openjdk.org/jdk/pull/12778.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12778/head:pull/12778 PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
On Mon, 13 Mar 2023 21:35:17 GMT, Doug Simon wrote: >> Frederic Parain has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fixes includes and style > > src/hotspot/share/jvmci/jvmciEnv.hpp line 149: > >> 147: }; >> 148: >> 149: extern JNIEXPORT jobjectArray c2v_getDeclaredFieldsInfo(JNIEnv* env, >> jobject, jobject, jlong); > > What's the purpose of this declaration? I don't think you need it or the > `friend` declaration below since `new_FieldInfo(FieldInfo* fieldinfo, > JVMCI_TRAPS)` is public. Without this declaration, builds fail on Windows with this error: `error C2375: 'c2v_getDeclaredFieldsInfo': redefinition; different linkage` - PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v2]
On Mon, 13 Mar 2023 21:05:11 GMT, Coleen Phillimore wrote:
>> Matias Saavedra Silva has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Interpreter optimization and comments
>
> src/hotspot/cpu/x86/templateTable_x86.cpp line 2798:
>
>> 2796:bool is_invokevirtual,
>> 2797:bool is_invokevfinal,
>> /*unused*/
>> 2798:bool is_invokedynamic
>> /*unused*/) {
>
> Can you remove the parameter since the s390 port is here?
Ok, never mind, I saw s390 port but it doesn't seem to be in these changes (?)
-
PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
On Mon, 13 Mar 2023 21:44:59 GMT, Doug Simon wrote: >> Frederic Parain has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fixes includes and style > > src/hotspot/share/jvmci/vmStructs_jvmci.cpp line 416: > >> 414: declare_constant(FieldInfo::FieldFlags::_ff_injected) >> \ >> 415: declare_constant(FieldInfo::FieldFlags::_ff_stable) >> \ >> 416: declare_constant(FieldInfo::FieldFlags::_ff_generic) >> \ > > I don't think `_ff_generic` is used in the JVMCI Java code so this entry can > be deleted. Please double check the other entries. _ff_generic removed. _ff_stable is used in `HotSpotResolvedJavaFieldImpl.isStable()`. _ff_injected is used in `HotSpotResolvedJavaFieldImpl.isInternal()` - PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]
On Mon, 13 Mar 2023 21:53:37 GMT, Doug Simon wrote: >> Frederic Parain has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fixes includes and style > > src/jdk.internal.vm.ci/share/classes/jdk/vm/ci/meta/ResolvedJavaField.java > line 48: > >> 46: * Returns VM internal flags associated with this field >> 47: */ >> 48: int getInternalModifiers(); > > We've never exposed the internal modifiers before in public JVMCI API and we > should refrain from doing so until there's a good reason to do so. Please > remove this method. Access to internal modifiers is needed in `HotSpotResolvedJavaFieldTest.testEquivalenceForInternalFields()`. I moved the declaration of the method to `HotSpotResolvedJavaField`. Does this change work for you? - PR: https://git.openjdk.org/jdk/pull/12855
Re: RFR: 8257967: JFR: Events for loaded agents [v10]
On Mon, 13 Mar 2023 09:46:04 GMT, Andrew Dinn wrote: >> Markus Grönlund has updated the pull request incrementally with one >> additional commit since the last revision: >> >> more cleanup > > src/hotspot/share/jfr/metadata/metadata.xml line 1182: > >> 1180: > description="The time the JVM initialized the agent" /> >> 1181: > label="Initialization Time" description="The duration of executing the >> initialization method exported by the agent" /> >> 1182: > > @mgronlun A somewhat drive-by comment. It might be clearer if you renamed > these event fields and accessors, plus also the corresponding fields and > accessors in class Agent, as `initializationTime` and > `initializationDuration`. Makes sense. - PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v10]
On Tue, 14 Mar 2023 06:01:05 GMT, David Holmes wrote: > I've had a good look through now and have a better sense of the refactoring. > Seems good. > > > > I'll wait for any tweaks before hitting the approve button though. > > > > Thanks Thanks so much for taking a look. I realized that implementation details of loading should probably reside in agent.cpp, not agentList.cpp. I am currently off on vacation and will update when back. Thanks also to Andrew Dinn for comments. - PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v10]
On Mon, 13 Mar 2023 09:49:39 GMT, Andrew Dinn wrote:
>> src/hotspot/share/prims/agentList.cpp line 64:
>>
>>> 62: void AgentList::add_xrun(const char* name, char* options, bool
>>> absolute_path) {
>>> 63: Agent* agent = new Agent(name, options, absolute_path);
>>> 64: agent->_is_xrun = true;
>>
>> Why direct access of private field instead of having a setter like other
>> parts of the Agent API?
>
> n.b. that also applies for accesses/updates to field _next.
I wanted all accesses to use the iterator. The only access is given to the
iterator and AgentList by way of being friends. No need to expose more.
-
PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v9]
On Fri, 10 Mar 2023 06:57:46 GMT, David Holmes wrote: >> Markus Grönlund has updated the pull request incrementally with one >> additional commit since the last revision: >> >> handle multiple envs with same VMInit callback > > src/hotspot/share/prims/agent.cpp line 41: > >> 39: char* copy = AllocateHeap(length + 1, mtInternal); >> 40: strncpy(copy, str, length + 1); >> 41: assert(strncmp(copy, str, length + 1) == 0, "invariant"); > > Unclear what you are checking here. Don't you trust strncpy? Maybe a bit paranoid, yes. I can clean up. - PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8257967: JFR: Events for loaded agents [v10]
On Mon, 13 Mar 2023 06:22:21 GMT, David Holmes wrote:
>> Markus Grönlund has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> more cleanup
>
> src/hotspot/share/prims/agent.cpp line 34:
>
>> 32: }
>> 33:
>> 34: static const char* allocate_copy(const char* str) {
>
> Why not just use `os::strdup`?
Better alternative, thanks David.
> src/hotspot/share/prims/agentList.cpp line 227:
>
>> 225: * store data in their JvmtiEnv local storage.
>> 226: *
>> 227: * Please see JPLISAgent.c in module java.instrument, see JPLISAgent.h
>> and JPLISAgent.c.
>
> No need to mention the .c file twice.
Good point.
> src/hotspot/share/prims/agentList.cpp line 419:
>
>> 417: const jint err = (*on_load_entry)(&main_vm,
>> const_cast(agent->options()), NULL);
>> 418: if (err != JNI_OK) {
>> 419: vm_exit_during_initialization("-Xrun library failed to init",
>> agent->name());
>
> Do you need to be back in `_thread_in_vm` before exiting?
Hmm. This was ported as is. I will double-check.
> src/hotspot/share/prims/agentList.cpp line 542:
>
>> 540:
>> 541: // Invoke the Agent_OnAttach function
>> 542: JavaThread* THREAD = JavaThread::current(); // For exception macros.
>
> Nit: just use `current` rather than `THREAD` and don't use the exception
> macros.
Ported as is but good point, will update.
-
PR: https://git.openjdk.org/jdk/pull/12923
Re: RFR: 8291555: Implement alternative fast-locking scheme [v24]
> This change adds a fast-locking scheme as an alternative to the current > stack-locking implementation. It retains the advantages of stack-locking > (namely fast locking in uncontended code-paths), while avoiding the overload > of the mark word. That overloading causes massive problems with Lilliput, > because it means we have to check and deal with this situation when trying to > access the mark-word. And because of the very racy nature, this turns out to > be very complex and would involve a variant of the inflation protocol to > ensure that the object header is stable. (The current implementation of > setting/fetching the i-hash provides a glimpse into the complexity). > > What the original stack-locking does is basically to push a stack-lock onto > the stack which consists only of the displaced header, and CAS a pointer to > this stack location into the object header (the lowest two header bits being > 00 indicate 'stack-locked'). The pointer into the stack can then be used to > identify which thread currently owns the lock. > > This change basically reverses stack-locking: It still CASes the lowest two > header bits to 00 to indicate 'fast-locked' but does *not* overload the upper > bits with a stack-pointer. Instead, it pushes the object-reference to a > thread-local lock-stack. This is a new structure which is basically a small > array of oops that is associated with each thread. Experience shows that this > array typcially remains very small (3-5 elements). Using this lock stack, it > is possible to query which threads own which locks. Most importantly, the > most common question 'does the current thread own me?' is very quickly > answered by doing a quick scan of the array. More complex queries like 'which > thread owns X?' are not performed in very performance-critical paths (usually > in code like JVMTI or deadlock detection) where it is ok to do more complex > operations (and we already do). The lock-stack is also a new set of GC roots, > and would be scanned during thread scanning, possibly concurrently, via the > normal p rotocols. > > The lock-stack is grown when needed. This means that we need to check for > potential overflow before attempting locking. When that is the case, locking > fast-paths would call into the runtime to grow the stack and handle the > locking. Compiled fast-paths (C1 and C2 on x86_64 and aarch64) do this check > on method entry to avoid (possibly lots) of such checks at locking sites. > > In contrast to stack-locking, fast-locking does *not* support recursive > locking (yet). When that happens, the fast-lock gets inflated to a full > monitor. It is not clear if it is worth to add support for recursive > fast-locking. > > One trouble is that when a contending thread arrives at a fast-locked object, > it must inflate the fast-lock to a full monitor. Normally, we need to know > the current owning thread, and record that in the monitor, so that the > contending thread can wait for the current owner to properly exit the > monitor. However, fast-locking doesn't have this information. What we do > instead is to record a special marker ANONYMOUS_OWNER. When the thread that > currently holds the lock arrives at monitorexit, and observes > ANONYMOUS_OWNER, it knows it must be itself, fixes the owner to be itself, > and then properly exits the monitor, and thus handing over to the contending > thread. > > As an alternative, I considered to remove stack-locking altogether, and only > use heavy monitors. In most workloads this did not show measurable > regressions. However, in a few workloads, I have observed severe regressions. > All of them have been using old synchronized Java collections (Vector, > Stack), StringBuffer or similar code. The combination of two conditions leads > to regressions without stack- or fast-locking: 1. The workload synchronizes > on uncontended locks (e.g. single-threaded use of Vector or StringBuffer) and > 2. The workload churns such locks. IOW, uncontended use of Vector, > StringBuffer, etc as such is ok, but creating lots of such single-use, > single-threaded-locked objects leads to massive ObjectMonitor churn, which > can lead to a significant performance impact. But alas, such code exists, and > we probably don't want to punish it if we can avoid it. > > This change enables to simplify (and speed-up!) a lot of code: > > - The inflation protocol is no longer necessary: we can directly CAS the > (tagged) ObjectMonitor pointer to the object header. > - Accessing the hashcode could now be done in the fastpath always, if the > hashcode has been installed. Fast-locked headers can be used directly, for > monitor-locked objects we can easily reach-through to the displaced header. > This is safe because Java threads participate in monitor deflation protocol. > This would be implemented in a separate PR > > > Testing: > - [x] tier1 x86_64 x aarch64 x +UseFastLocking > - [x] tier2 x86_6
Re: RFR: 8291555: Implement alternative fast-locking scheme [v23]
On Mon, 13 Mar 2023 20:02:45 GMT, Roman Kennke wrote: >> This change adds a fast-locking scheme as an alternative to the current >> stack-locking implementation. It retains the advantages of stack-locking >> (namely fast locking in uncontended code-paths), while avoiding the overload >> of the mark word. That overloading causes massive problems with Lilliput, >> because it means we have to check and deal with this situation when trying >> to access the mark-word. And because of the very racy nature, this turns out >> to be very complex and would involve a variant of the inflation protocol to >> ensure that the object header is stable. (The current implementation of >> setting/fetching the i-hash provides a glimpse into the complexity). >> >> What the original stack-locking does is basically to push a stack-lock onto >> the stack which consists only of the displaced header, and CAS a pointer to >> this stack location into the object header (the lowest two header bits being >> 00 indicate 'stack-locked'). The pointer into the stack can then be used to >> identify which thread currently owns the lock. >> >> This change basically reverses stack-locking: It still CASes the lowest two >> header bits to 00 to indicate 'fast-locked' but does *not* overload the >> upper bits with a stack-pointer. Instead, it pushes the object-reference to >> a thread-local lock-stack. This is a new structure which is basically a >> small array of oops that is associated with each thread. Experience shows >> that this array typcially remains very small (3-5 elements). Using this lock >> stack, it is possible to query which threads own which locks. Most >> importantly, the most common question 'does the current thread own me?' is >> very quickly answered by doing a quick scan of the array. More complex >> queries like 'which thread owns X?' are not performed in very >> performance-critical paths (usually in code like JVMTI or deadlock >> detection) where it is ok to do more complex operations (and we already do). >> The lock-stack is also a new set of GC roots, and would be scanned during >> thread scanning, possibly concurrently, via the normal protocols. >> >> The lock-stack is grown when needed. This means that we need to check for >> potential overflow before attempting locking. When that is the case, locking >> fast-paths would call into the runtime to grow the stack and handle the >> locking. Compiled fast-paths (C1 and C2 on x86_64 and aarch64) do this check >> on method entry to avoid (possibly lots) of such checks at locking sites. >> >> In contrast to stack-locking, fast-locking does *not* support recursive >> locking (yet). When that happens, the fast-lock gets inflated to a full >> monitor. It is not clear if it is worth to add support for recursive >> fast-locking. >> >> One trouble is that when a contending thread arrives at a fast-locked >> object, it must inflate the fast-lock to a full monitor. Normally, we need >> to know the current owning thread, and record that in the monitor, so that >> the contending thread can wait for the current owner to properly exit the >> monitor. However, fast-locking doesn't have this information. What we do >> instead is to record a special marker ANONYMOUS_OWNER. When the thread that >> currently holds the lock arrives at monitorexit, and observes >> ANONYMOUS_OWNER, it knows it must be itself, fixes the owner to be itself, >> and then properly exits the monitor, and thus handing over to the contending >> thread. >> >> As an alternative, I considered to remove stack-locking altogether, and only >> use heavy monitors. In most workloads this did not show measurable >> regressions. However, in a few workloads, I have observed severe >> regressions. All of them have been using old synchronized Java collections >> (Vector, Stack), StringBuffer or similar code. The combination of two >> conditions leads to regressions without stack- or fast-locking: 1. The >> workload synchronizes on uncontended locks (e.g. single-threaded use of >> Vector or StringBuffer) and 2. The workload churns such locks. IOW, >> uncontended use of Vector, StringBuffer, etc as such is ok, but creating >> lots of such single-use, single-threaded-locked objects leads to massive >> ObjectMonitor churn, which can lead to a significant performance impact. But >> alas, such code exists, and we probably don't want to punish it if we can >> avoid it. >> >> This change enables to simplify (and speed-up!) a lot of code: >> >> - The inflation protocol is no longer necessary: we can directly CAS the >> (tagged) ObjectMonitor pointer to the object header. >> - Accessing the hashcode could now be done in the fastpath always, if the >> hashcode has been installed. Fast-locked headers can be used directly, for >> monitor-locked objects we can easily reach-through to the displaced header. >> This is safe because Java threads participate in monitor deflation protocol.
Re: RFR: 8301995: Move invokedynamic resolution information out of ConstantPoolCacheEntry [v2]
On Thu, 9 Mar 2023 21:18:19 GMT, Matias Saavedra Silva wrote: >> The current structure used to store the resolution information for >> invokedynamic, ConstantPoolCacheEntry, is difficult to interpret due to its >> ambigious fields f1 and f2. This structure can hold information for fields, >> methods, and invokedynamics and each of its fields can hold different types >> of values depending on the entry. >> >> This enhancement proposes a new structure to exclusively contain >> invokedynamic information in a manner that is easy to interpret and easy to >> extend. Resolved invokedynamic entries will be stored in an array in the >> constant pool cache and the operand of the invokedynamic bytecode will be >> rewritten to be the index into this array. >> >> Any areas that previously accessed invokedynamic data from >> ConstantPoolCacheEntry will be replaced with accesses to this new array and >> structure. Verified with tier1-9 tests. >> >> The PPC was provided by @reinrich and the RISCV port was provided by >> @DingliZhang and @zifeihan. >> >> This change supports the following platforms: x86, aarch64, PPC, and RISCV > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > Interpreter optimization and comments @matias9927 can I ask you to merge master? There seem to be conflicts (at least I see a message "This branch has conflicts that must be resolved"). I'd like to give the change a spin in our CI testing. This requires that it can be applied on master. - PR: https://git.openjdk.org/jdk/pull/12778
Re: RFR: 8294977: Convert test/jdk/java tests from ASM library to Classfile API
On Tue, 14 Mar 2023 02:43:41 GMT, liach wrote:
> Summaries:
> 1. A few recommendations about updating the constant API is made at
> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-March/000233.html
> and I may update this patch shall the API changes be integrated before
> 2. One ASM library-specific test, `LambdaAsm` is removed. Others have their
> code generation infrastructure upgraded from ASM to Classfile API.
> 3. Most tests are included in tier1, but some are not:
> In `:jdk_io`: (tier2, part 2)
>
> test/jdk/java/io/Serializable/records/SerialPersistentFieldsTest.java
> test/jdk/java/io/Serializable/records/ProhibitedMethods.java
> test/jdk/java/io/Serializable/records/BadCanonicalCtrTest.java
>
> In `:jdk_instrument`: (tier 3)
>
> test/jdk/java/lang/instrument/RetransformAgent.java
> test/jdk/java/lang/instrument/NativeMethodPrefixAgent.java
> test/jdk/java/lang/instrument/asmlib/Instrumentor.java
>
>
> @asotona Would you mind reviewing?
Good to see these tests converted, just a few nits about trying to keep the
code/style consistent with the existing code/style where possible.
test/jdk/java/lang/ModuleTests/AnnotationsTest.java line 146:
> 144: */
> 145: static byte[] addDeprecated(byte[] bytes, boolean forRemoval, String
> since) {
> 146: return
> Classfile.parse(bytes).transform(ClassTransform.ACCEPT_ALL.andThen(ClassTransform.endHandler(clb
> -> {
The conversion of this test okay but would be good if you split up the overly
long lines as they are inconsistent with everything else in this test and makes
it annoying to look at the changes side-by-side.
test/jdk/java/lang/invoke/defineHiddenClass/BasicTest.java line 282:
> 280:
> 281: assertTrue(hc.isHidden());
> 282: assertEquals(hc.getModifiers(),
> accessFlags.stream().mapToInt(AccessFlag::mask).reduce(AccessFlag.PUBLIC.mask(),
> (a, b) -> a | b));
Do you mind splitting this line too, it's 140+ characters long so impossible to
look at the changes side-by-side.
test/jdk/java/util/ServiceLoader/BadProvidersTest.java line 216:
> 214: clb.withSuperclass(CD_Object);
> 215: clb.withFlags(AccessFlag.PUBLIC, AccessFlag.SUPER);
> 216: var provider$1Desc = ClassDesc.of("p", "ProviderFactory$1");
This is class descriptor for ProviderFactory$1, not "Provider" so maybe rename
this to providerFactory1 or something a bit clearer.
-
PR: https://git.openjdk.org/jdk/pull/13009
