Re: RFR: 8330969: scalability issue with loaded JVMTI agent
On Thu, 25 Apr 2024 20:39:03 GMT, Alex Menkov wrote:
>> This is a fix of the following JVMTI scalability issue. A closed benchmark
>> with millions of virtual threads shows 3X-4X overhead when a JVMTI agent has
>> been loaded. For instance, this is observable when an app is executed under
>> control of the Oracle Studio `collect` utility.
>> For performance analysis, experiments and numbers, please, see the comment
>> below this description.
>>
>> The fix is to replace the global counter `_VTMS_transition_count` with the
>> mark bit `_VTMS_transition_mark` in each `JavaThread`'.
>>
>> Testing:
>> - Tested with mach5 tiers 1-6
>
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1638:
>
>> 1636: // Iterates over all JavaThread's, counts VTMS transitions and
>> restores
>> 1637: // jt->jvmti_thread_state() and jt->jvmti_vthread() for VTMS
>> transition protocol.
>> 1638: void count_transitions_and_correct_jvmti_thread_states() {
>
> The method doesn't count anything anymore.
> Rename it to `correct_jvmti_thread_states()`?
> Comment needs to be updated too.
Good suggestion, thanks. Renamed function and corrected the comment.
> src/hotspot/share/prims/jvmtiThreadState.cpp line 501:
>
>> 499: oop vt = JNIHandles::resolve_external_guard(vthread);
>> 500: java_lang_Thread::set_is_in_VTMS_transition(vt, false);
>> 501: assert(thread->VTMS_transition_mark(), "sanity ed_heck");
>
> Suggestion:
>
> assert(thread->VTMS_transition_mark(), "sanity check");
Thanks. Fixed now.
> src/hotspot/share/runtime/javaThread.hpp line 668:
>
>> 666: void toggle_is_disable_suspend() { _is_disable_suspend
>> = !_is_disable_suspend; };
>> 667:
>> 668: bool VTMS_transition_mark(){ return
>> Atomic::load(&_VTMS_transition_mark); }
>
> Suggestion:
>
> bool VTMS_transition_mark() const { return
> Atomic::load(&_VTMS_transition_mark); }
Good suggestion, thanks. Fixed now.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18937#discussion_r1580607373
PR Review Comment: https://git.openjdk.org/jdk/pull/18937#discussion_r1580610256
PR Review Comment: https://git.openjdk.org/jdk/pull/18937#discussion_r1580612160
Re: RFR: 8330969: scalability issue with loaded JVMTI agent [v2]
> This is a fix of the following JVMTI scalability issue. A closed benchmark > with millions of virtual threads shows 3X-4X overhead when a JVMTI agent has > been loaded. For instance, this is observable when an app is executed under > control of the Oracle Studio `collect` utility. > For performance analysis, experiments and numbers, please, see the comment > below this description. > > The fix is to replace the global counter `_VTMS_transition_count` with the > mark bit `_VTMS_transition_mark` in each `JavaThread`'. > > Testing: > - Tested with mach5 tiers 1-6 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: fixed minor issues: renamed function, corrected comment, removed typo in assert - Changes: - all: https://git.openjdk.org/jdk/pull/18937/files - new: https://git.openjdk.org/jdk/pull/18937/files/6e1bf369..03bcfecb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18937=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18937=00-01 Stats: 6 lines in 3 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/18937.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18937/head:pull/18937 PR: https://git.openjdk.org/jdk/pull/18937
Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v10]
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Jonathan Gibbons has updated the pull request incrementally with one additional
commit since the last revision:
fix typo
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18527/files
- new: https://git.openjdk.org/jdk/pull/18527/files/39689a52..48e8b0a2
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18527=09
- incr: https://webrevs.openjdk.org/?repo=jdk=18527=08-09
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v10]
On Fri, 26 Apr 2024 16:04:09 GMT, Jonathan Gibbons wrote:
>> Please review the updates to support a proposed new
>> `-Xlint:dangling-doc-comments` option.
>>
>> The work can be thought of as in 3 parts:
>>
>> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
>> is possible to specify the appropriately configured `Lint` object when it is
>> time to consider whether to generate the diagnostic or not.
>>
>> 2. Updates to the `javac` front end to record "dangling docs comments" found
>> near the beginning of a declaration, and to report them using an instance of
>> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
>> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
>> procedure for handling dangling doc comments is described in this comment in
>> `JavacParser`.
>>
>> * Dangling documentation comments are handled as follows.
>> * 1. {@code Scanner} adds all doc comments to a queue of
>> * recent doc comments. The queue is flushed whenever
>> * it is known that the recent doc comments should be
>> * ignored and should not cause any warnings.
>> * 2. The primary documentation comment is the one obtained
>> * from the first token of any declaration.
>> * (using {@code token.getDocComment()}.
>> * 3. At the end of the "signature" of the declaration
>> * (that is, before any initialization or body for the
>> * declaration) any other "recent" comments are saved
>> * in a map using the primary comment as a key,
>> * using this method, {@code saveDanglingComments}.
>> * 4. When the tree node for the declaration is finally
>> * available, and the primary comment, if any,
>> * is "attached", (in {@link #attach}) any related
>> * dangling comments are also attached to the tree node
>> * by registering them using the {@link #deferredLintHandler}.
>> * 5. (Later) Warnings may be genereated for the dangling
>> * comments, subject to the {@code -Xlint} and
>> * {@code @SuppressWarnings}.
>>
>>
>> 3. Updates to the make files to disable the warnings in modules for which
>> the
>> warning is generated. This is often because of the confusing use of `/**` to
>> create box or other standout comments.
>
> Jonathan Gibbons has updated the pull request incrementally with one
> additional commit since the last revision:
>
> fix typo
Marked as reviewed by prr (Reviewer).
-
PR Review: https://git.openjdk.org/jdk/pull/18527#pullrequestreview-2025744069
Integrated: 8303689: javac -Xlint could/should report on "dangling" doc comments
On Wed, 27 Mar 2024 22:04:30 GMT, Jonathan Gibbons wrote:
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
This pull request has now been integrated.
Changeset: a920af23
Author:Jonathan Gibbons
URL:
https://git.openjdk.org/jdk/commit/a920af233a11592af113f456f7608cb59dd1617e
Stats: 482 lines in 58 files changed: 385 ins; 3 del; 94 mod
8303689: javac -Xlint could/should report on "dangling" doc comments
Reviewed-by: vromero, ihse, prr
-
PR: https://git.openjdk.org/jdk/pull/18527
RFR: 8330852: All callers of JvmtiEnvBase::get_threadOop_and_JavaThread should pass current thread explicitly
Some cleanup related to JvmtiEnvBase::get_threadOop_and_JavaThread method Testing: tier1-6 - Commit messages: - fix Changes: https://git.openjdk.org/jdk/pull/18986/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18986=00 Issue: https://bugs.openjdk.org/browse/JDK-8330852 Stats: 42 lines in 3 files changed: 3 ins; 10 del; 29 mod Patch: https://git.openjdk.org/jdk/pull/18986.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18986/head:pull/18986 PR: https://git.openjdk.org/jdk/pull/18986
Re: RFR: 8331087: Move immutable nmethod data from CodeCache
On Fri, 26 Apr 2024 21:36:50 GMT, Vladimir Kozlov wrote: >> Move immutable nmethod's data from CodeCache to C heap. It includes >> `dependencies, nul_chk_table, handler_table, scopes_pcs, scopes_data, >> speculations, jvmci_data`. It amounts for about 30% (optimized VM) of space >> in CodeCache. >> >> Use HotSpot's `os::malloc()` to allocate memory in C heap for immutable >> nmethod's data. Bail out compilation if allocation failed. >> >> Shuffle fields order and change some fields size from 4 to 2 bytes to avoid >> nmethod's header size increase. >> >> Tested tier1-5, stress,xcomp >> >> Our performance testing does not show difference. >> >> Example of updated `-XX:+PrintNMethodStatistics` output is in JBS comment. > > src/hotspot/share/code/nmethod.cpp line 117: > >> 115: result = static_cast(thing); \ >> 116: assert(static_cast(result) == thing, "failed: %d != %d", >> static_cast(result), thing); >> 117: > > I replaced `checked_cast<>()` with this macro because of next issues: > - The existing assert points to `utilities/checkedCast.hpp` file where this > method is located and not where failed cast. It does not help when it is used > several times in one method (for example, in `nmethod()` constructors). > - The existing assert does not print values I thought @kimbarrett had a draft PR to address the error reporting issue, but I can't seem to find it. To solve the general problem, I think we need a version of vmassert() that takes `char* file, int lineno` as arguments, and a macro wrapper for checked_cast() that passes `__FILE__` and `__LINEN__` from the caller. - PR Review Comment: https://git.openjdk.org/jdk/pull/18984#discussion_r1581628786
Re: RFR: 8331087: Move immutable nmethod data from CodeCache
On Fri, 26 Apr 2024 21:16:03 GMT, Vladimir Kozlov wrote:
> Move immutable nmethod's data from CodeCache to C heap. It includes
> `dependencies, nul_chk_table, handler_table, scopes_pcs, scopes_data,
> speculations, jvmci_data`. It amounts for about 30% (optimized VM) of space
> in CodeCache.
>
> Use HotSpot's `os::malloc()` to allocate memory in C heap for immutable
> nmethod's data. Bail out compilation if allocation failed.
>
> Shuffle fields order and change some fields size from 4 to 2 bytes to avoid
> nmethod's header size increase.
>
> Tested tier1-5, stress,xcomp
>
> Our performance testing does not show difference.
>
> Example of updated `-XX:+PrintNMethodStatistics` output is in JBS comment.
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/NMethod.java line 528:
> 526: private int getScopesDataOffset() { return (int)
> scopesDataOffsetField .getValue(addr); }
> 527: private int getScopesPCsOffset(){ return (int)
> scopesPCsOffsetField .getValue(addr); }
> 528: private int getDependenciesOffset() { return (int) 0; }
Suggestion:
No longer used.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18984#discussion_r1581671710
RFR: 8331087: Move immutable nmethod data from CodeCache
Move immutable nmethod's data from CodeCache to C heap. It includes `dependencies, nul_chk_table, handler_table, scopes_pcs, scopes_data, speculations, jvmci_data`. It amounts for about 30% (optimized VM) of space in CodeCache. Use HotSpot's `os::malloc()` to allocate memory in C heap for immutable nmethod's data. Bail out compilation if allocation failed. Shuffle fields order and change some fields size from 4 to 2 bytes to avoid nmethod's header size increase. Tested tier1-5, stress,xcomp - Commit messages: - 8331087: Move immutable nmethod data from CodeCache Changes: https://git.openjdk.org/jdk/pull/18984/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18984=00 Issue: https://bugs.openjdk.org/browse/JDK-8331087 Stats: 290 lines in 7 files changed: 149 ins; 31 del; 110 mod Patch: https://git.openjdk.org/jdk/pull/18984.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18984/head:pull/18984 PR: https://git.openjdk.org/jdk/pull/18984
Re: RFR: 8330852: All callers of JvmtiEnvBase::get_threadOop_and_JavaThread should pass current thread explicitly
On Fri, 26 Apr 2024 22:59:43 GMT, Alex Menkov wrote: > Some cleanup related to JvmtiEnvBase::get_threadOop_and_JavaThread method > > Testing: tier1-6 src/hotspot/share/prims/jvmtiEnvBase.cpp line 1976: > 1974: oop thread_obj = nullptr; > 1975: > 1976: jvmtiError err = > JvmtiEnvBase::get_threadOop_and_JavaThread(tlh.list(), target, current, > _thread, _obj); I think a good cleanup would be to also replace `current` with `current_thread`, although I'm not sure how common each are. I see 3 `current` references in this webrev. - PR Review Comment: https://git.openjdk.org/jdk/pull/18986#discussion_r1581610079
Re: RFR: 8331087: Move immutable nmethod data from CodeCache
On Sat, 27 Apr 2024 00:02:16 GMT, Dean Long wrote: >> src/hotspot/share/code/nmethod.cpp line 117: >> >>> 115: result = static_cast(thing); \ >>> 116: assert(static_cast(result) == thing, "failed: %d != %d", >>> static_cast(result), thing); >>> 117: >> >> I replaced `checked_cast<>()` with this macro because of next issues: >> - The existing assert points to `utilities/checkedCast.hpp` file where this >> method is located and not where failed cast. It does not help when it is >> used several times in one method (for example, in `nmethod()` constructors). >> - The existing assert does not print values > > I thought @kimbarrett had a draft PR to address the error reporting issue, > but I can't seem to find it. To solve the general problem, I think we need a > version of vmassert() that takes `char* file, int lineno` as arguments, and a > macro wrapper for checked_cast() that passes `__FILE__` and `__LINEN__` from > the caller. Yes, it would be perfect separate RFE. - PR Review Comment: https://git.openjdk.org/jdk/pull/18984#discussion_r1581641784
Re: RFR: 8331087: Move immutable nmethod data from CodeCache
On Fri, 26 Apr 2024 21:16:03 GMT, Vladimir Kozlov wrote: > Move immutable nmethod's data from CodeCache to C heap. It includes > `dependencies, nul_chk_table, handler_table, scopes_pcs, scopes_data, > speculations, jvmci_data`. It amounts for about 30% (optimized VM) of space > in CodeCache. > > Use HotSpot's `os::malloc()` to allocate memory in C heap for immutable > nmethod's data. Bail out compilation if allocation failed. > > Shuffle fields order and change some fields size from 4 to 2 bytes to avoid > nmethod's header size increase. > > Tested tier1-5, stress,xcomp > > Our performance testing does not show difference. > > Example of updated `-XX:+PrintNMethodStatistics` output is in JBS comment. src/hotspot/share/code/nmethod.cpp line 1332: > 1330: #if INCLUDE_JVMCI > 1331: _speculations_offset = _scopes_data_offset; > 1332: _jvmci_data_offset = _speculations_offset; Why not use 0 for all these? - PR Review Comment: https://git.openjdk.org/jdk/pull/18984#discussion_r1581642931
Re: RFR: 8331087: Move immutable nmethod data from CodeCache
On Fri, 26 Apr 2024 21:16:03 GMT, Vladimir Kozlov wrote: > Move immutable nmethod's data from CodeCache to C heap. It includes > `dependencies, nul_chk_table, handler_table, scopes_pcs, scopes_data, > speculations, jvmci_data`. It amounts for about 30% (optimized VM) of space > in CodeCache. > > Use HotSpot's `os::malloc()` to allocate memory in C heap for immutable > nmethod's data. Bail out compilation if allocation failed. > > Shuffle fields order and change some fields size from 4 to 2 bytes to avoid > nmethod's header size increase. > > Tested tier1-5, stress,xcomp > > Our performance testing does not show difference. > > Example of updated `-XX:+PrintNMethodStatistics` output is in JBS comment. Thank you, @dean-long, for review. I will collect (hopefully) more comments for next update before testing and pushing it. - PR Comment: https://git.openjdk.org/jdk/pull/18984#issuecomment-2080291257
Re: RFR: 8331087: Move immutable nmethod data from CodeCache
On Sat, 27 Apr 2024 00:48:49 GMT, Dean Long wrote:
>> Move immutable nmethod's data from CodeCache to C heap. It includes
>> `dependencies, nul_chk_table, handler_table, scopes_pcs, scopes_data,
>> speculations, jvmci_data`. It amounts for about 30% (optimized VM) of space
>> in CodeCache.
>>
>> Use HotSpot's `os::malloc()` to allocate memory in C heap for immutable
>> nmethod's data. Bail out compilation if allocation failed.
>>
>> Shuffle fields order and change some fields size from 4 to 2 bytes to avoid
>> nmethod's header size increase.
>>
>> Tested tier1-5, stress,xcomp
>>
>> Our performance testing does not show difference.
>>
>> Example of updated `-XX:+PrintNMethodStatistics` output is in JBS comment.
>
> src/hotspot/share/code/nmethod.cpp line 1332:
>
>> 1330: #if INCLUDE_JVMCI
>> 1331: _speculations_offset = _scopes_data_offset;
>> 1332: _jvmci_data_offset = _speculations_offset;
>
> Why not use 0 for all these?
Right. Before all these offsets were assigned to _dependencies_offset which was
not 0.
But now we can use 0 for all of them since "_dependencies_offset" is 0.
> src/hotspot/share/code/nmethod.cpp line 1484:
>
>> 1482: // Calculate positive offset as distance between the start of
>> stubs section
>> 1483: // (which is also the end of instructions section) and the start
>> of the handler.
>> 1484: CHECKED_CAST(_unwind_handler_offset, int16_t, (_stub_offset -
>> code_offset() - offsets->value(CodeOffsets::UnwindHandler)));
>
> Suggestion:
>
> int unwind_handler_offset = code_offset() +
> offsets->value(CodeOffsets::UnwindHandler);
> CHECKED_CAST(_unwind_handler_offset, int16_t, _stub_offset -
> unwind_handler_offset);
Will do.
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/NMethod.java line
> 528:
>
>> 526: private int getScopesDataOffset() { return (int)
>> scopesDataOffsetField .getValue(addr); }
>> 527: private int getScopesPCsOffset(){ return (int)
>> scopesPCsOffsetField .getValue(addr); }
>> 528: private int getDependenciesOffset() { return (int) 0; }
>
> Suggestion:
>
>
> No longer used.
Will remove.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18984#discussion_r1581667051
PR Review Comment: https://git.openjdk.org/jdk/pull/18984#discussion_r1581668080
PR Review Comment: https://git.openjdk.org/jdk/pull/18984#discussion_r1581679317
Re: RFR: 8330852: All callers of JvmtiEnvBase::get_threadOop_and_JavaThread should pass current thread explicitly
On Fri, 26 Apr 2024 23:36:41 GMT, Chris Plummer wrote: >> Some cleanup related to JvmtiEnvBase::get_threadOop_and_JavaThread method >> >> Testing: tier1-6 > > src/hotspot/share/prims/jvmtiEnvBase.cpp line 1976: > >> 1974: oop thread_obj = nullptr; >> 1975: >> 1976: jvmtiError err = >> JvmtiEnvBase::get_threadOop_and_JavaThread(tlh.list(), target, current, >> _thread, _obj); > > I think a good cleanup would be to also replace `current` with > `current_thread`, although I'm not sure how common each are. I see 3 > `current` references in this webrev. Looks like in JVMTI `current_thread` is more common (and `current` is usually used in runtime :) - PR Review Comment: https://git.openjdk.org/jdk/pull/18986#discussion_r1581628063
Re: RFR: 8331087: Move immutable nmethod data from CodeCache
On Fri, 26 Apr 2024 21:16:03 GMT, Vladimir Kozlov wrote: > Move immutable nmethod's data from CodeCache to C heap. It includes > `dependencies, nul_chk_table, handler_table, scopes_pcs, scopes_data, > speculations, jvmci_data`. It amounts for about 30% (optimized VM) of space > in CodeCache. > > Use HotSpot's `os::malloc()` to allocate memory in C heap for immutable > nmethod's data. Bail out compilation if allocation failed. > > Shuffle fields order and change some fields size from 4 to 2 bytes to avoid > nmethod's header size increase. > > Tested tier1-5, stress,xcomp > > Our performance testing does not show difference. > > Example of updated `-XX:+PrintNMethodStatistics` output is in JBS comment. src/hotspot/share/code/nmethod.cpp line 1484: > 1482: // Calculate positive offset as distance between the start of > stubs section > 1483: // (which is also the end of instructions section) and the start > of the handler. > 1484: CHECKED_CAST(_unwind_handler_offset, int16_t, (_stub_offset - > code_offset() - offsets->value(CodeOffsets::UnwindHandler))); Suggestion: int unwind_handler_offset = code_offset() + offsets->value(CodeOffsets::UnwindHandler); CHECKED_CAST(_unwind_handler_offset, int16_t, _stub_offset - unwind_handler_offset); - PR Review Comment: https://git.openjdk.org/jdk/pull/18984#discussion_r1581644356
Re: RFR: 8330969: scalability issue with loaded JVMTI agent [v2]
On Fri, 26 Apr 2024 07:45:50 GMT, Serguei Spitsyn wrote: >> This is a fix of the following JVMTI scalability issue. A closed benchmark >> with millions of virtual threads shows 3X-4X overhead when a JVMTI agent has >> been loaded. For instance, this is observable when an app is executed under >> control of the Oracle Studio `collect` utility. >> For performance analysis, experiments and numbers, please, see the comment >> below this description. >> >> The fix is to replace the global counter `_VTMS_transition_count` with the >> mark bit `_VTMS_transition_mark` in each `JavaThread`'. >> >> Testing: >> - Tested with mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: fixed minor issues: renamed function, corrected comment, removed > typo in assert src/hotspot/share/prims/jvmtiThreadState.cpp line 366: > 364: attempts--; > 365: } > 366: DEBUG_ONLY(if (attempts == 0) break;) Previously `_VTMS_transition_count` considered all threads at the same time. Now you are iterating through the threads and looking at a flag in each one. Is it guaranteed that once the `_VTMS_transition_mark` flag has been verified not to be set in a thread it won't get set while still iterating in the threads loop? src/hotspot/share/prims/jvmtiThreadState.cpp line 433: > 431: // Avoid using MonitorLocker on performance critical path, use > 432: // two-level synchronization with lock-free operations on counters. > 433: assert(!thread->VTMS_transition_mark(), "sanity check"); The "counters" comment needs to be updated. src/hotspot/share/prims/jvmtiThreadState.cpp line 456: > 454: // Slow path: undo unsuccessful optimistic counter incrementation. > 455: // It can cause an extra waiting cycle for VTMS transition disablers. > 456: thread->set_VTMS_transition_mark(false); The "optimistic counter incrementation" comment needs updating. - PR Review Comment: https://git.openjdk.org/jdk/pull/18937#discussion_r1581460754 PR Review Comment: https://git.openjdk.org/jdk/pull/18937#discussion_r1581463641 PR Review Comment: https://git.openjdk.org/jdk/pull/18937#discussion_r1581462878
Re: RFR: 8331087: Move immutable nmethod data from CodeCache
On Fri, 26 Apr 2024 21:16:03 GMT, Vladimir Kozlov wrote:
> Move immutable nmethod's data from CodeCache to C heap. It includes
> `dependencies, nul_chk_table, handler_table, scopes_pcs, scopes_data,
> speculations, jvmci_data`. It amounts for about 30% (optimized VM) of space
> in CodeCache.
>
> Use HotSpot's `os::malloc()` to allocate memory in C heap for immutable
> nmethod's data. Bail out compilation if allocation failed.
>
> Shuffle fields order and change some fields size from 4 to 2 bytes to avoid
> nmethod's header size increase.
>
> Tested tier1-5, stress,xcomp
src/hotspot/share/code/nmethod.cpp line 117:
> 115: result = static_cast(thing); \
> 116: assert(static_cast(result) == thing, "failed: %d != %d",
> static_cast(result), thing);
> 117:
I replaced `checked_cast<>()` with this macro because of next issues:
- The existing assert points to `utilities/checkedCast.hpp` file where this
method is located and not where failed cast. It does not help when it is used
several times in one method (for example, in `nmethod()` constructors).
- The existing assert does not print values
src/hotspot/share/code/nmethod.cpp line 1324:
> 1322:
> 1323: // native wrapper does not have read-only data but we need unique
> not null address
> 1324: _immutable_data = data_end();
I can't use nullptr because VM expects not null address when it checks, for
example, `dependencies_begin()` even so sizes are 0.
I used `data_end()` instead of nullptr in other places too.
src/hotspot/share/code/nmethod.hpp line 583:
> 581: int dependencies_size () const { return int(
> dependencies_end () - dependencies_begin ()); }
> 582: int handler_table_size () const { return int(
> handler_table_end() - handler_table_begin()); }
> 583: int nul_chk_table_size () const { return int(
> nul_chk_table_end() - nul_chk_table_begin()); }
Shift by one space to aline code.
test/hotspot/jtreg/compiler/c1/TestLinearScanOrderMain.java line 29:
> 27: * @compile TestLinearScanOrder.jasm
> 28: * @run main/othervm -Xcomp -XX:+TieredCompilation -XX:TieredStopAtLevel=1
> 29: * -XX:+IgnoreUnrecognizedVMOptions
> -XX:NMethodSizeLimit=655360
This test caught one `check_cast<>` issue during development but only on
aarch64.
On x64 the test bailed out compilation before that because default
`NMethodSizeLimit` was not big enough ((64*K)*wordSize = 524288).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18984#discussion_r1581561933
PR Review Comment: https://git.openjdk.org/jdk/pull/18984#discussion_r1581565762
PR Review Comment: https://git.openjdk.org/jdk/pull/18984#discussion_r1581558637
PR Review Comment: https://git.openjdk.org/jdk/pull/18984#discussion_r1581557756
Re: RFR: 8330694: Rename 'HeapRegion' to 'G1HeapRegion' [v4]
> follow up 8267941 Lei Zaakjyu has updated the pull request incrementally with one additional commit since the last revision: fix indentation - Changes: - all: https://git.openjdk.org/jdk/pull/18871/files - new: https://git.openjdk.org/jdk/pull/18871/files/f02334fd..a76a71de Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18871=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18871=02-03 Stats: 34 lines in 8 files changed: 0 ins; 2 del; 32 mod Patch: https://git.openjdk.org/jdk/pull/18871.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18871/head:pull/18871 PR: https://git.openjdk.org/jdk/pull/18871
