Re: RFR: 8330969: scalability issue with loaded JVMTI agent [v2]
On Wed, 15 May 2024 06:00:46 GMT, Serguei Spitsyn wrote: >> I'm not sure this answered Chris' query properly. Or I'm reading Chris' >> query wrong. >> >> Perhaps this is not what Chris had in mind, but I'm wondering what happens >> in some >> Thread-A when it is checked and passed by but then Thread-A sets the flag in >> itself >> after the for-loop has passed it by. Does that Thread-A flag value get lost? > >> Perhaps this is not what Chris had in mind, but I'm wondering what happens >> in some >> Thread-A when it is checked and passed by but then Thread-A sets the flag in >> itself >> after the for-loop has passed it by. Does that Thread-A flag value get lost? > > Thank you for the question. > The Thread-A sets the flag optimistically and then re-checks if > `sync_protocol_enabled()` and any disabler exists. It can be global disbaler > (`_VTMS_transition_disable_for_all_count > 0`) or disabler of `Thread-A` only > (`java_lang_Thread::VTMS_transition_disable_count(vth()) > 0`). If any > disabler exists then `Thread-A` clears the optimistic settings and goes with > the pessimistic approach under protection of `JvmtiVTMSTransition_lock`. > > Please, let me know if you still have questions. This algorithm sounds correct. Thanks for closing the loop on my belated comment. - PR Review Comment: https://git.openjdk.org/jdk/pull/18937#discussion_r1603957324
Re: RFR: 8330969: scalability issue with loaded JVMTI agent [v2]
On Fri, 10 May 2024 22:09:01 GMT, Daniel D. Daugherty wrote: > Perhaps this is not what Chris had in mind, but I'm wondering what happens in > some > Thread-A when it is checked and passed by but then Thread-A sets the flag in > itself > after the for-loop has passed it by. Does that Thread-A flag value get lost? Thank you for the question. The Thread-A sets the flag optimistically and then re-checks if `sync_protocol_enabled()` and any disabler exists. It can be global disbaler (`_VTMS_transition_disable_for_all_count > 0`) or disabler of `Thread-A` only (`java_lang_Thread::VTMS_transition_disable_count(vth()) > 0`). If any disabler exists then `Thread-A` clears the optimistic settings and goes with the pessimistic approach under protection of `JvmtiVTMSTransition_lock`. Please, let me know if you still have questions. - PR Review Comment: https://git.openjdk.org/jdk/pull/18937#discussion_r1600987604
Re: RFR: 8330969: scalability issue with loaded JVMTI agent [v2]
On Tue, 30 Apr 2024 01:49:31 GMT, Serguei Spitsyn wrote: >> 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? > > Thank you for the comment. It is thinking in a right direction. > Each `JavaThread` set the `VTM_transition_mark` only once and then checks for > disable counters: > - `_VTMS_transition_disable_for_all_count` > - `java_lang_Thread::VTMS_transition_disable_count(vth())` > > If any of the disable counters is not zero then each `JavaThread` clears the > optimistically set mark and continues under protection of the > `JvmtiVTMSTransition_lock`. I'm not sure this answered Chris' query properly. Or I'm reading Chris' query wrong. Perhaps this is not what Chris had in mind, but I'm wondering what happens in some Thread-A when it is checked and passed by but then Thread-A sets the flag in itself after the for-loop has passed it by. Does that Thread-A flag value get lost? - PR Review Comment: https://git.openjdk.org/jdk/pull/18937#discussion_r1597266296
Re: RFR: 8330969: scalability issue with loaded JVMTI agent [v2]
On Fri, 26 Apr 2024 19:34:55 GMT, Chris Plummer wrote: >> 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? Thank you for the comment. It is thinking in a right direction. Each `JavaThread` set the `VTM_transition_mark` only once and then checks for disable counters: - `_VTMS_transition_disable_for_all_count` - `java_lang_Thread::VTMS_transition_disable_count(vth())` If any of the disable counters is not zero then each `JavaThread` clears the optimistically set mark and continues under protection of the `JvmtiVTMSTransition_lock`. - PR Review Comment: https://git.openjdk.org/jdk/pull/18937#discussion_r1584025182
Re: RFR: 8330969: scalability issue with loaded JVMTI agent [v2]
On Fri, 26 Apr 2024 19:38:40 GMT, Chris Plummer wrote: >> 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 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. Nice catch, thanks. Fixed now. > 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. Nice catch, thanks. Fixed now. - PR Review Comment: https://git.openjdk.org/jdk/pull/18937#discussion_r1584018624 PR Review Comment: https://git.openjdk.org/jdk/pull/18937#discussion_r1584018405
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: 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&pr=18937&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18937&range=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
