Re: RFR: 8320215: HeapDumper can use DumpWriter buffer during merge
On Fri, 19 Apr 2024 00:10:12 GMT, Alex Menkov wrote: > The fix updates HeapMerger to use writer buffer (no need to copy memory, also > writer buffer is 1MB instead of 4KB). > Additionally fixed small issue in FileWriter (looks like `ssize_t` instead of > `size_t` is a typo, the argument should be unsigned) > > Testing: all HeapDump-related tests on Oracle supported platforms Looks good. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18850#pullrequestreview-2030232272
Re: RFR: 8322043: HeapDumper should use parallel dump by default [v5]
On Wed, 17 Apr 2024 20:42:54 GMT, Alex Menkov wrote: >> The fix makes VM heap dumping parallel by default. >> `jcmd GC.heap_dump` and `jmap -dump` had parallel dumping by default, the >> fix affects `HotSpotDiagnosticMXBean.dumpHeap()`, >> `-XX:+HeapDumpBeforeFullGC`, `-XX:+HeapDumpAfterFullGC` and >> `-XX:+HeapDumpOnOutOfMemoryError`. >> >> Testing: >> - manually tested different heap dump scenarios with `-Xlog:heapdump`; >> - tier1,tier2,hs-tier5-svc; >> - all reg.tests that use heap dump. > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > removed unneeded cast One nit, otherwise looks good. Thanks src/hotspot/share/services/diagnosticCommand.cpp line 523: > 521: // and makes it easier to browse. > 522: HeapDumper dumper(!_all.value() /* request GC if _all is false*/); > 523: dumper.dump(_filename.value(), output(), (int)level, > _overwrite.value(), (uint)parallel); I'm not sure what the prevalent style is for cast operators, but I don't see any point doing this in this PR when the file is otherwise untouched. Thanks. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18748#pullrequestreview-2030177251 PR Review Comment: https://git.openjdk.org/jdk/pull/18748#discussion_r1584110521
Re: RFR: 8322043: HeapDumper should use parallel dump by default [v5]
On Wed, 17 Apr 2024 20:42:54 GMT, Alex Menkov wrote: >> The fix makes VM heap dumping parallel by default. >> `jcmd GC.heap_dump` and `jmap -dump` had parallel dumping by default, the >> fix affects `HotSpotDiagnosticMXBean.dumpHeap()`, >> `-XX:+HeapDumpBeforeFullGC`, `-XX:+HeapDumpAfterFullGC` and >> `-XX:+HeapDumpOnOutOfMemoryError`. >> >> Testing: >> - manually tested different heap dump scenarios with `-Xlog:heapdump`; >> - tier1,tier2,hs-tier5-svc; >> - all reg.tests that use heap dump. > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > removed unneeded cast This looks good. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18748#pullrequestreview-2030062605
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 This looks good in general. Thank you for taking care about it. The Runtime team suggested to use the name `current` for the current thread. The plan is to unify the JVMTI code as well. So, I'd suggest to rename all occurrences of the `current_thread` that you touched to the `current`. It will be a good step in a right direction. I'm already doing it for some time. - PR Review: https://git.openjdk.org/jdk/pull/18986#pullrequestreview-2030037342
Re: RFR: 8330852: All callers of JvmtiEnvBase::get_threadOop_and_JavaThread should pass current thread explicitly
On Sat, 27 Apr 2024 00:01:16 GMT, Alex Menkov wrote: >> 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 :) The plan is to unify this with the approach used by the Runtime team. - PR Review Comment: https://git.openjdk.org/jdk/pull/18986#discussion_r1584032225
Re: RFR: 8330969: scalability issue with loaded JVMTI agent [v3]
> 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: correct comments related to VTMS transition counters - Changes: - all: https://git.openjdk.org/jdk/pull/18937/files - new: https://git.openjdk.org/jdk/pull/18937/files/03bcfecb..173840b5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18937=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18937=01-02 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 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: 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: 8330694: Rename 'HeapRegion' to 'G1HeapRegion' [v4]
On Mon, 29 Apr 2024 08:14:03 GMT, Thomas Schatzl wrote: > mach5 higher tier SA tests are fine. What are your plans for the remaining SA > renames (would highly recommend to add) and the G1HeapRegion related helper > classes? I suggest the related helper classes be done in further followups, not make this change even larger. - PR Comment: https://git.openjdk.org/jdk/pull/18871#issuecomment-2083822517
Integrated: 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`. 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. Call `vm_exit_out_of_memory()` 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. This pull request has now been integrated. Changeset: bdcc2400 Author:Vladimir Kozlov URL: https://git.openjdk.org/jdk/commit/bdcc2400db63e604d76f9b5bd3c876271743f69f Stats: 311 lines in 5 files changed: 163 ins; 42 del; 106 mod 8331087: Move immutable nmethod data from CodeCache Reviewed-by: thartmann, dlong, dnsimon - PR: https://git.openjdk.org/jdk/pull/18984
Re: RFR: 8331087: Move immutable nmethod data from CodeCache [v2]
On Sun, 28 Apr 2024 23:37:22 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`. 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. Call `vm_exit_out_of_memory()` 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. > > Vladimir Kozlov has updated the pull request incrementally with one > additional commit since the last revision: > > Address comments. Moved jvmci_data back to mutable data section. Thank you, Dean, Doug and Tobias for reviews. - PR Comment: https://git.openjdk.org/jdk/pull/18984#issuecomment-2083102730
Re: RFR: 8331087: Move immutable nmethod data from CodeCache [v2]
On Mon, 29 Apr 2024 06:33:09 GMT, Tobias Hartmann wrote: > Looks good to me. Did you measure any impact on performance (potentially due > to improved code density)? Thank you, @TobiHartmann, for review. > What's left for [JDK-7072317](https://bugs.openjdk.org/browse/JDK-7072317)? Make Relocation info (10% of nmethod size) immutable by moving all encoded pointers (external words and others, which we need to patch in Leyden when loading cached code) from it into separate mutable section. > I wonder if the `CHECKED_CAST` changes shouldn't go into a separate RFE. No, I want clear information which cast failed instead of trying to reproduce very intermittent failure like this: [JDK-8331253](https://bugs.openjdk.org/browse/JDK-8331253). When you have several `checked_cast` in one method it is impossible to find which failed without this macro. - PR Comment: https://git.openjdk.org/jdk/pull/18984#issuecomment-2082953366
Re: RFR: 8318682: SA decoding of scalar replaced objects is broken [v6]
On Wed, 17 Jan 2024 19:52:32 GMT, Tom Rodriguez wrote: >> The changes for JDK-8287061 didn't update the SA decoding logic and there >> are other places where the decoding has gotten out of sync with HotSpot. >> Some of them can't be tested because they are part of JVMCI but I've added a >> directed test for the JDK-8287061 code and a more brute force test that >> tries to decode everything. > > Tom Rodriguez has updated the pull request incrementally with one additional > commit since the last revision: > > Review comments Let's merge this soon Tom. - PR Comment: https://git.openjdk.org/jdk/pull/17407#issuecomment-2082605948
Re: RFR: 8331087: Move immutable nmethod data from CodeCache [v2]
On Sun, 28 Apr 2024 23:37:22 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`. 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. Call `vm_exit_out_of_memory()` 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. > > Vladimir Kozlov has updated the pull request incrementally with one > additional commit since the last revision: > > Address comments. Moved jvmci_data back to mutable data section. Marked as reviewed by dnsimon (Reviewer). JVMCI changes now look good to me. - PR Review: https://git.openjdk.org/jdk/pull/18984#pullrequestreview-2027884309 PR Comment: https://git.openjdk.org/jdk/pull/18984#issuecomment-2082185011
Re: RFR: 8330694: Rename 'HeapRegion' to 'G1HeapRegion' [v4]
On Sat, 27 Apr 2024 02:34:21 GMT, Lei Zaakjyu wrote: >> follow up 8267941 > > Lei Zaakjyu has updated the pull request incrementally with one additional > commit since the last revision: > > fix indentation mach5 higher tier SA tests are fine. What are your plans for the remaining SA renames (would highly recommend to add) and the G1HeapRegion related helper classes? - PR Comment: https://git.openjdk.org/jdk/pull/18871#issuecomment-2082124530
Re: RFR: 8331087: Move immutable nmethod data from CodeCache [v2]
On Sun, 28 Apr 2024 23:37:22 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`. 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. Call `vm_exit_out_of_memory()` 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. > > Vladimir Kozlov has updated the pull request incrementally with one > additional commit since the last revision: > > Address comments. Moved jvmci_data back to mutable data section. Marked as reviewed by dlong (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18984#pullrequestreview-2027786061
Re: RFR: 8331087: Move immutable nmethod data from CodeCache
On Sun, 28 Apr 2024 07:02:40 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`. 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. Call `vm_exit_out_of_memory()` 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. > > It only makes sense if the immutable data heap is not also used for other > critical resources. If malloc or metaspace were used as the immutable data > heap, normally failures in those heaps are fatal, because other critical > resources (monitors, classes, etc) are allocated from there, so any failure > means the JVM is about to die. There's no reason to find a fall-back method > to allocate a new nmethod in that case. > Just to be clear @dean-long , you're saying failure to allocate immutable > data in the C heap should result in a fatal error? Makes sense to me as the > VM must indeed be very close to crashing anyway in that case. It also, > obviates the need for propagating `out_of_memory_error` to JVMCI code. I hadn't thought it through that far, actually. I was only pointing out that the proposed fall-back: > increase nmethod size and put immutable data there (as before). isn't worth the trouble. But making the C heap failure fatal immediately is reasonable, especially if it simplifies JVMCI error reporting. - PR Comment: https://git.openjdk.org/jdk/pull/18984#issuecomment-2082083104
Re: RFR: 8331087: Move immutable nmethod data from CodeCache [v2]
On Sun, 28 Apr 2024 23:37:22 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`. 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. Call `vm_exit_out_of_memory()` 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. > > Vladimir Kozlov has updated the pull request incrementally with one > additional commit since the last revision: > > Address comments. Moved jvmci_data back to mutable data section. Looks good to me. Did you measure any impact on performance (potentially due to improved code density)? What's left for [JDK-7072317](https://bugs.openjdk.org/browse/JDK-7072317)? I wonder if the `CHECKED_CAST` changes shouldn't go into a separate RFE. - Marked as reviewed by thartmann (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18984#pullrequestreview-2027660174
