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: 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: 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
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, 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. > > 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. Update: 1. Addressed @dean-long first comments. 2. Based on discussion with Doug and Tom (see comments in [JDK-8331087](https://bugs.openjdk.org/browse/JDK-8331087)), moved `jvmci_data` back to nmethod's mutable data section. 3. Replaced my allocation failure handling code with call to `vm_exit_out_of_memory()`. I verified (with `UseNewCode` hack`) that out of memory is correctly reported in fastdebug and product VMs: # # There is insufficient memory for the Java Runtime Environment to continue. # Native memory allocation (malloc) failed to allocate 64 bytes. Error detail: nmethod: no space for immutable data # An error report file with more information is saved as: # /scratch/kvn/jdk_git/hs_err_pid4086275.log - PR Comment: https://git.openjdk.org/jdk/pull/18984#issuecomment-2081701059
Re: RFR: 8331087: Move immutable nmethod data from CodeCache [v2]
> 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. 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. - Changes: - all: https://git.openjdk.org/jdk/pull/18984/files - new: https://git.openjdk.org/jdk/pull/18984/files/6b1f69d9..1824f46c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18984=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18984=00-01 Stats: 98 lines in 5 files changed: 39 ins; 36 del; 23 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: 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, 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. > > 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. - PR Comment: https://git.openjdk.org/jdk/pull/18984#issuecomment-2081427477
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. 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. - PR Comment: https://git.openjdk.org/jdk/pull/18984#issuecomment-2081364009
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. @dean-long and @dougxc I am thinking may be I should not bailout when `malloc` (or other space reservation in a future) failed to allocate memory for immutable data. But instead increase nmethod size and put immutable data there (as before). Then we bailout only when CodeCache is full as before and we don't need `out_of_memory` failure reason. May be only record that in logs (when they are enabled). What do you think? - PR Comment: https://git.openjdk.org/jdk/pull/18984#issuecomment-2081297482
Re: RFR: 8331087: Move immutable nmethod data from CodeCache
On Sat, 27 Apr 2024 20:48:38 GMT, Doug Simon 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.hpp line 476: > >> 474: passed, >> 475: code_cache_full, >> 476: out_of_memory > > Maybe `out_of_c_heap_memory` would be clearer? Or > `out_of_immutable_data_memory` if immutable data may not always be malloc'ed. May be `no_space_for_immutable_data`. - PR Review Comment: https://git.openjdk.org/jdk/pull/18984#discussion_r1581998799
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/jvmci/jvmciRuntime.cpp line 2178: > 2176: > nmethod_mirror_name, > 2177: > failed_speculations); > 2178: nmethod::ResultStatus result_status; Please propagate the new `out_of_memory` result throughout JVMCI (e.g. in `JVMCI::CodeInstallResult` enum and `HotSpotVMConfig.getCodeInstallResultDescription` method). - PR Review Comment: https://git.openjdk.org/jdk/pull/18984#discussion_r1581906300
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.hpp line 476: > 474: passed, > 475: code_cache_full, > 476: out_of_memory Maybe `out_of_c_heap_memory` would be clearer? Or `out_of_immutable_data_memory` if immutable data may not always be malloc'ed. - PR Review Comment: https://git.openjdk.org/jdk/pull/18984#discussion_r1581904919
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
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: 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 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: 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 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: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
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
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
