Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
On Thu, 4 Apr 2024 00:05:20 GMT, Vladimir Kozlov wrote: >> Revert [JDK-8152664](https://bugs.openjdk.org/browse/JDK-8152664) RFE >> [changes](https://github.com/openjdk/jdk/commit/b853eb7f5ca24eeeda18acbb14287f706499c365) >> which was used for AOT [JEP 295](https://openjdk.org/jeps/295) >> implementation in JDK 9. The code was left in HotSpot assuming it will help >> in a future. But during work on Leyden we decided to not use it. In Leyden >> cached compiled code will be restored in CodeCache as normal nmethods: no >> need to change VM's runtime and GC code to process them. >> >> I may work on optimizing `CodeBlob` and `nmethod` fields layout to reduce >> header size in separate changes. In these changes I did simple fields >> reordering to keep small (1 byte) fields together. >> >> I do not see (and not expected) performance difference with these changes. >> >> Tested tier1-5, xcomp, stress. Running performance testing. >> >> I need help with testing on platforms which Oracle does not support. > > Vladimir Kozlov 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: > > - Address comments > - Merge branch 'master' into 8329332 > - Removed not_used state of nmethod > - remove trailing whitespace > - 8329332: Remove CompiledMethod and CodeBlobLayout classes GHA `linux-x64-hs-minimal` failure is not related to changes: 2024-04-04T00:07:46.9654262Z ##[warning]Failed to download action 'https://api.github.com/repos/actions/github-script/tarball/60a0d83039c74a4aee543508d2ffcb1c3799cdea'. Error: The request was canceled due to the configured HttpClient.Timeout of 100 seconds elapsing. 2024-04-04T00:07:46.9656929Z ##[warning]Back off 22.252 seconds before retry. 2024-04-04T00:08:52.1252710Z ##[error]The SSL connection could not be established, see inner exception. - PR Comment: https://git.openjdk.org/jdk/pull/18554#issuecomment-2035859221
Re: RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass [v3]
On Wed, 3 Apr 2024 23:50:47 GMT, Daniel D. Daugherty wrote: >> Coleen Phillimore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix spacing and punctuation. make log_info into log_debug. > > src/hotspot/share/oops/instanceKlass.cpp line 2313: > >> 2311: size_t size = idnum_allocated_count(); >> 2312: assert(size > (size_t)idnum, "should already have space"); >> 2313: jmeths = NEW_C_HEAP_ARRAY(jmethodID, size+1, mtClass); > > nit: spaces around operator `+` Got them. The good thing about the smaller function is it's easier to find these. - PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1550659977
Re: RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass [v4]
> This change simplifies the code that grows the jmethodID cache in > InstanceKlass. Instead of lazily, when there's a rare request for a > jmethodID for an obsolete method, the jmethodID cache is grown during the > RedefineClasses safepoint. The InstanceKlass's jmethodID cache is lazily > allocated when there's a jmethodID allocated, so not every InstanceKlass has > a cache, but the growth now only happens in a safepoint. This code will > become racy with the potential change for deallocating jmethodIDs. > > Tested with tier1-4, vmTestbase/nsk/jvmti java/lang/instrument tests (in case > they're not in tier1-4). Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Two more. - Changes: - all: https://git.openjdk.org/jdk/pull/18549/files - new: https://git.openjdk.org/jdk/pull/18549/files/26bd82d3..aab60e28 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18549=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18549=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18549.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18549/head:pull/18549 PR: https://git.openjdk.org/jdk/pull/18549
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v3]
> Revert [JDK-8152664](https://bugs.openjdk.org/browse/JDK-8152664) RFE > [changes](https://github.com/openjdk/jdk/commit/b853eb7f5ca24eeeda18acbb14287f706499c365) > which was used for AOT [JEP 295](https://openjdk.org/jeps/295) > implementation in JDK 9. The code was left in HotSpot assuming it will help > in a future. But during work on Leyden we decided to not use it. In Leyden > cached compiled code will be restored in CodeCache as normal nmethods: no > need to change VM's runtime and GC code to process them. > > I may work on optimizing `CodeBlob` and `nmethod` fields layout to reduce > header size in separate changes. In these changes I did simple fields > reordering to keep small (1 byte) fields together. > > I do not see (and not expected) performance difference with these changes. > > Tested tier1-5, xcomp, stress. Running performance testing. > > I need help with testing on platforms which Oracle does not support. Vladimir Kozlov 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: - Address comments - Merge branch 'master' into 8329332 - Removed not_used state of nmethod - remove trailing whitespace - 8329332: Remove CompiledMethod and CodeBlobLayout classes - Changes: - all: https://git.openjdk.org/jdk/pull/18554/files - new: https://git.openjdk.org/jdk/pull/18554/files/246ff68a..33768fb2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18554=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18554=01-02 Stats: 9283 lines in 197 files changed: 3058 ins; 4514 del; 1711 mod Patch: https://git.openjdk.org/jdk/pull/18554.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18554/head:pull/18554 PR: https://git.openjdk.org/jdk/pull/18554
Re: RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass [v3]
On Wed, 3 Apr 2024 23:05:24 GMT, Coleen Phillimore wrote: >> This change simplifies the code that grows the jmethodID cache in >> InstanceKlass. Instead of lazily, when there's a rare request for a >> jmethodID for an obsolete method, the jmethodID cache is grown during the >> RedefineClasses safepoint. The InstanceKlass's jmethodID cache is lazily >> allocated when there's a jmethodID allocated, so not every InstanceKlass has >> a cache, but the growth now only happens in a safepoint. This code will >> become racy with the potential change for deallocating jmethodIDs. >> >> Tested with tier1-4, vmTestbase/nsk/jvmti java/lang/instrument tests (in >> case they're not in tier1-4). > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Fix spacing and punctuation. make log_info into log_debug. Thanks for the fixes. There are a couple that you missed. src/hotspot/share/oops/instanceKlass.cpp line 2313: > 2311: size_t size = idnum_allocated_count(); > 2312: assert(size > (size_t)idnum, "should already have space"); > 2313: jmeths = NEW_C_HEAP_ARRAY(jmethodID, size+1, mtClass); nit: spaces around operator `+` src/hotspot/share/oops/instanceKlass.cpp line 2346: > 2344: // Allocate a larger one and copy entries to the new one. > 2345: // They've already been updated to point to new methods where > applicable (i.e., not obsolete). > 2346: jmethodID* new_cache = NEW_C_HEAP_ARRAY(jmethodID, size+1, > mtClass); nit: spaces around operator `+` - Marked as reviewed by dcubed (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18549#pullrequestreview-1978305784 PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1550653423 PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1550653504
Re: RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass [v2]
On Wed, 3 Apr 2024 13:25:36 GMT, Coleen Phillimore wrote: >> This change simplifies the code that grows the jmethodID cache in >> InstanceKlass. Instead of lazily, when there's a rare request for a >> jmethodID for an obsolete method, the jmethodID cache is grown during the >> RedefineClasses safepoint. The InstanceKlass's jmethodID cache is lazily >> allocated when there's a jmethodID allocated, so not every InstanceKlass has >> a cache, but the growth now only happens in a safepoint. This code will >> become racy with the potential change for deallocating jmethodIDs. >> >> Tested with tier1-4, vmTestbase/nsk/jvmti java/lang/instrument tests (in >> case they're not in tier1-4). > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Refactoring suggested by Serguei. Thanks for reviewing this Dan. I've fixed the nits you pointed out. I ran tier1-4 which includes JDI tests, but I'll run 5, 6 tonight. - PR Review: https://git.openjdk.org/jdk/pull/18549#pullrequestreview-1978241218 PR Comment: https://git.openjdk.org/jdk/pull/18549#issuecomment-2035752492
Re: RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass [v2]
On Wed, 3 Apr 2024 20:46:55 GMT, Daniel D. Daugherty wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Refactoring suggested by Serguei.
>
> src/hotspot/share/oops/method.cpp line 2200:
>
>> 2198:
>> 2199: ResourceMark rm;
>> 2200: log_info(jmethod)("Creating jmethodID for Method %s",
>> m->external_name());
>
> Hmmm... will this be too noisy for `info` level?
I forget that there's a default where -Xlog nothing will print all the info
level messages. I changed it to debug.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1550615588
Re: RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass [v3]
> This change simplifies the code that grows the jmethodID cache in > InstanceKlass. Instead of lazily, when there's a rare request for a > jmethodID for an obsolete method, the jmethodID cache is grown during the > RedefineClasses safepoint. The InstanceKlass's jmethodID cache is lazily > allocated when there's a jmethodID allocated, so not every InstanceKlass has > a cache, but the growth now only happens in a safepoint. This code will > become racy with the potential change for deallocating jmethodIDs. > > Tested with tier1-4, vmTestbase/nsk/jvmti java/lang/instrument tests (in case > they're not in tier1-4). Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Fix spacing and punctuation. make log_info into log_debug. - Changes: - all: https://git.openjdk.org/jdk/pull/18549/files - new: https://git.openjdk.org/jdk/pull/18549/files/6576d14d..26bd82d3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18549=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18549=01-02 Stats: 13 lines in 3 files changed: 0 ins; 0 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/18549.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18549/head:pull/18549 PR: https://git.openjdk.org/jdk/pull/18549
Re: RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass [v2]
On Wed, 3 Apr 2024 13:25:36 GMT, Coleen Phillimore wrote:
>> This change simplifies the code that grows the jmethodID cache in
>> InstanceKlass. Instead of lazily, when there's a rare request for a
>> jmethodID for an obsolete method, the jmethodID cache is grown during the
>> RedefineClasses safepoint. The InstanceKlass's jmethodID cache is lazily
>> allocated when there's a jmethodID allocated, so not every InstanceKlass has
>> a cache, but the growth now only happens in a safepoint. This code will
>> become racy with the potential change for deallocating jmethodIDs.
>>
>> Tested with tier1-4, vmTestbase/nsk/jvmti java/lang/instrument tests (in
>> case they're not in tier1-4).
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Refactoring suggested by Serguei.
Okay I've crawled thru the changes twice and I went back thru
the bug history for this code and added some notes and links
to the bug ID.
Modulo the nits that I flagged, I think the changes are fine. Making
cache growth only happen in the RedefineClasses safepoint is
definite improvement.
I see that you've run JVM/TI and JLI tests. You should also run JDI
tests. Basically for a low level fix like this that affects JVM/TI, you
should run Mach5 Tier[1-6].
src/hotspot/share/oops/instanceKlass.cpp line 2272:
> 2270: jmethodID InstanceKlass::update_jmethod_id(jmethodID* jmeths, Method*
> method, int idnum) {
> 2271: if (method->is_old() && !method->is_obsolete()) {
> 2272: // If the method passed in is old (but not obsolete), use the
> current version
nit: should end with a period.
src/hotspot/share/oops/instanceKlass.cpp line 2277:
> 2275: }
> 2276: jmethodID new_id = Method::make_jmethod_id(class_loader_data(),
> method);
> 2277: Atomic::release_store([idnum+1], new_id);
nit: spaces around operator `+`
src/hotspot/share/oops/instanceKlass.cpp line 2304:
> 2302: //
> 2303: // If the RedefineClasses() API has been used, then this cache grows
> 2304: // in the redefinition safepoint.
Much easier to reason about. Thanks for simplifying it.
src/hotspot/share/oops/instanceKlass.cpp line 2314:
> 2312: assert(size > (size_t)idnum, "should already have space");
> 2313: jmeths = NEW_C_HEAP_ARRAY(jmethodID, size+1, mtClass);
> 2314: memset(jmeths, 0, (size+1)*sizeof(jmethodID));
nit: spaces around operator `+` (two places)
nit: spaces around operator `*`
src/hotspot/share/oops/instanceKlass.cpp line 2325:
> 2323: }
> 2324:
> 2325: jmethodID id = Atomic::load_acquire([idnum+1]);
nit: spaces around operator `+`
src/hotspot/share/oops/instanceKlass.cpp line 2328:
> 2326: if (id == nullptr) {
> 2327: MutexLocker ml(JmethodIdCreation_lock,
> Mutex::_no_safepoint_check_flag);
> 2328: id = jmeths[idnum+1];
nit: spaces around operator `+`
src/hotspot/share/oops/instanceKlass.cpp line 2343:
> 2341: size_t size = idnum_allocated_count();
> 2342: size_t old_size = (size_t)cache[0];
> 2343: if (old_size < size+1) {
nit: spaces around operator `+`
src/hotspot/share/oops/instanceKlass.cpp line 2344:
> 2342: size_t old_size = (size_t)cache[0];
> 2343: if (old_size < size+1) {
> 2344: // allocate a larger one and copy entries to the new one.
nit typo: s/allocate/Allocate/
src/hotspot/share/oops/instanceKlass.cpp line 2345:
> 2343: if (old_size < size+1) {
> 2344: // allocate a larger one and copy entries to the new one.
> 2345: // They've already been updated to point to new methods where
> applicable (ie. not obsolete)
nit typo: s/ie./i.e.,/
Please add a period at the end of the sentence.
src/hotspot/share/oops/instanceKlass.cpp line 2347:
> 2345: // They've already been updated to point to new methods where
> applicable (ie. not obsolete)
> 2346: jmethodID* new_cache = NEW_C_HEAP_ARRAY(jmethodID, size+1,
> mtClass);
> 2347: memset(new_cache, 0, (size+1)*sizeof(jmethodID));
nit: spaces around operator `+` (two places)
nit: spaces around operator `*`
src/hotspot/share/oops/instanceKlass.cpp line 2348:
> 2346: jmethodID* new_cache = NEW_C_HEAP_ARRAY(jmethodID, size+1,
> mtClass);
> 2347: memset(new_cache, 0, (size+1)*sizeof(jmethodID));
> 2348: // cache size is stored in element[0], other elements offset by
> one
nit typo: s/cache/Cache/
Please add a period at the end.
src/hotspot/share/oops/instanceKlass.cpp line 2384:
> 2382: int idnum = method->method_idnum();
> 2383: jmethodID* jmeths = methods_jmethod_ids_acquire();
> 2384: return (jmeths != nullptr) ? jmeths[idnum+1] : nullptr;
nit: spaces around operator `+`
src/hotspot/share/oops/method.cpp line 2200:
> 2198:
> 2199: ResourceMark rm;
> 2200: log_info(jmethod)("Creating jmethodID for Method %s",
> m->external_name());
Hmmm... will this be too noisy for `info` level?
src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 4353:
> 4351:
Re: RFR: JDK-8322042: HeapDumper should perform merge on the current thread instead of VMThread [v2]
On Tue, 2 Apr 2024 21:13:33 GMT, Alex Menkov wrote: >> The fix updated HeapDumper to always perform merge on the current thread. >> >> Testing: tier1-5, all HeapDump-related tests >> Covered heap dumping scenarios: >> - `jcmd GC.heap_dump` command; >> - `HotSpotDiagnosticMXBean.dumpHeap()`; >> - `HeapDumpBeforeFullGC`, `HeapDumpAfterFullGC` VM options; >> - `HeapDumpOnOutOfMemoryError` VM option. > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > comment Marked as reviewed by kevinw (Reviewer). Thanks Alex, happy you've looked into that. 8-) - PR Review: https://git.openjdk.org/jdk/pull/18571#pullrequestreview-1978117624 PR Comment: https://git.openjdk.org/jdk/pull/18571#issuecomment-2035648620
Re: RFR: JDK-8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set [v10]
On Wed, 3 Apr 2024 13:37:56 GMT, Christoph Langer wrote: > Couldn't we just have an unconditional default for the GC.heap_dump > `filename` option? This would simplify the documentation. E.g. we could write > `If not specified, defaults to java_pid.hprof in the working directory > or value configured through -XX:HeapDumpPath` I've had similar thoughts while considering this PR. I'm not sure why currently there is no default. HeapDumpPath is not quite as simple as you document it above. It can be a filename or a directory, and when it is a directory it has its own default file naming. This should be captured in the jcmd help. - PR Comment: https://git.openjdk.org/jdk/pull/18190#issuecomment-2035550491
Re: RFR: JDK-8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set [v10]
On Wed, 3 Apr 2024 09:04:34 GMT, Matthias Baesken wrote: > > There's still the question of whether or not it is even appropriate to have > > -XX options taking the place of jcmd options. > > Some people (like our cloud support colleagues and also some who commented) > would like this approach, some find it not very useful. Seems there are also > already other globals flags playing a role in diagnosticCommand coding (e.g. > RecordDynamicDumpInfo, maybe more) so it is not completely 'new' that the > jcmd commands are influenced by globals flag. Looking at just the _names_ of > HeapDumpPath and also HeapDumpGzipLevel I would think that they play a role > for all heap dumps and not just some. That's a little different. For some reason (I'm not sure of the details) it was decided to not allow a dynamic dump with VM.cds unless it was explicitly enabled on the command line with the -XX:+ RecordDynamicDumpInfo option. This is different than having an jvm option that is used in place of an omitted jcmd option. - PR Comment: https://git.openjdk.org/jdk/pull/18190#issuecomment-2035541597
Re: RFR: JDK-8322042: HeapDumper should perform merge on the current thread instead of VMThread [v2]
On Wed, 3 Apr 2024 10:09:20 GMT, Kevin Walls wrote: > Are we saying there is never any need to perform the merge in a VM Operation? > Originally (JDK-8306441) it's either done in the attach thread, or a VM > operation if we are in another thread. But maybe that was just being cautious. Correct. Originally it was implemented for AttachListener thread only for safety (and heap dump using jcmd was point of interest). I didn't find issues with concurrent dumping (actually with concurrent merging, dumping itself is performed in VM_HeapDumper at safepoint). - PR Comment: https://git.openjdk.org/jdk/pull/18571#issuecomment-2035530652
Re: RFR: JDK-8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set [v10]
On Wed, 3 Apr 2024 08:03:22 GMT, Matthias Baesken wrote: > > There's also a question of whether currently missing doc updates for > > HeapDumpGzipLevel should be made part of this PR > > because it complicates back porting. > > It should most likely be a separate PR (the title of this one does not match, > and there are different backporting needs). Then you end up with the same issue that this PR has with 7 docs to update (and the "descriptive" default for the jcmd). I'm not advocating one approach over the other. I'm just pointing out that either approach has it's warts, and they rooted in the changes done for this PR. - PR Comment: https://git.openjdk.org/jdk/pull/18190#issuecomment-2035525083
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v2]
On Wed, 3 Apr 2024 17:55:38 GMT, Stefan Karlsson wrote:
>> No, `CodeCache::blob_count()` uses different macro `FOR_ALL_HEAPS(heap)`
>> because it looks for all code blobs, not only nmethods.
>>
>> `CodeCache::nmethod_count()` is the only place where `FOR_ALL_NMETHOD_HEAPS
>> ` was used. So I decided to remove the macro.
>
> I didn't say that blob_count used `FOR_ALL_NMETHODS_HEAP`. I wrote "one of
> these macros". I still think this adds an inconsistency to the code that I
> don't think is beneficial.
>
> With that said, can't this be written as:
>
> for (CodeHeap* heap : *_nmethod_heaps) {
>
>
> Maybe yet another opportunity for cleanups.
I like it and will do it in JDK-8329628.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1550378690
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v2]
On Wed, 3 Apr 2024 17:50:15 GMT, Stefan Karlsson wrote: >> No special rules here. I simply want to see all `virtual` methods explicitly >> and `override` is required by C++. >> I would like to keep it this way in these changes. I am investigating >> possibility to convert all these virtual methods to normal one to remove >> virtual table and virtual pointer (8 bytes) from CodeBlob class. > > `override` is not required by C++. You do however mark all virtual methods > with `override` if any of the functions are marked with `override`. I think > it would be good to have a HotSpot code style discussion about this (but not > in this RFE). I put `virtual/override` cleanup in CodeBlob as additional suggestion in followup RFE JDK-8329628. - PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1550370826
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v2]
On Mon, 1 Apr 2024 21:07:31 GMT, Vladimir Kozlov wrote: >> Revert [JDK-8152664](https://bugs.openjdk.org/browse/JDK-8152664) RFE >> [changes](https://github.com/openjdk/jdk/commit/b853eb7f5ca24eeeda18acbb14287f706499c365) >> which was used for AOT [JEP 295](https://openjdk.org/jeps/295) >> implementation in JDK 9. The code was left in HotSpot assuming it will help >> in a future. But during work on Leyden we decided to not use it. In Leyden >> cached compiled code will be restored in CodeCache as normal nmethods: no >> need to change VM's runtime and GC code to process them. >> >> I may work on optimizing `CodeBlob` and `nmethod` fields layout to reduce >> header size in separate changes. In these changes I did simple fields >> reordering to keep small (1 byte) fields together. >> >> I do not see (and not expected) performance difference with these changes. >> >> Tested tier1-5, xcomp, stress. Running performance testing. >> >> I need help with testing on platforms which Oracle does not support. > > Vladimir Kozlov has updated the pull request incrementally with one > additional commit since the last revision: > > Removed not_used state of nmethod I filed RFEs: [JDK-8329628](https://bugs.openjdk.org/browse/JDK-8329628): Additional changes after JDK-8329332 [JDK-8329629](https://bugs.openjdk.org/browse/JDK-8329629): GC interfaces should work directly against nmethod instead of CodeBlob - PR Comment: https://git.openjdk.org/jdk/pull/18554#issuecomment-2035421134
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v2]
On Wed, 3 Apr 2024 16:00:01 GMT, Stefan Karlsson wrote: >> Vladimir Kozlov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Removed not_used state of nmethod > > Nice! > > We've wanted to clean up some interfaces between the CodeCache and the GC > code by using nmethod closures instead of CodeBlob closures. This change (and > the Sweeper removal) makes it possible to do those cleanups. > > I've made a superficial pass over the patch to and left a few comments. Most > of those comments are things that would be nice to fix, but could also be > left as follow-up RFEs (if they are deemed to be worthy ideas to pursue). Thank you, @stefank, for great review. I addressed all your comments locally and with run testing in mach5 before pushing it. Except your suggestion about `find_blob_not_null()` - should be separate RFE. The same for suggestion "GC interfaces to work directly against nmethod instead of CodeBlob". - PR Comment: https://git.openjdk.org/jdk/pull/18554#issuecomment-2035354740
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v2]
On Wed, 3 Apr 2024 15:35:49 GMT, Stefan Karlsson wrote:
>> Vladimir Kozlov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Removed not_used state of nmethod
>
> src/hotspot/share/gc/x/xUnload.cpp line 78:
>
>> 76: class XIsUnloadingBehaviour : public IsUnloadingBehaviour {
>> 77: public:
>> 78: virtual bool has_dead_oop(nmethod* method) const {
>
> This now takes an `nmethod` argument, but still calls as_nmethod(). I think
> that should be removed from this, and all similar functions here in the GC
> code. If you want, I can do that as a follow-up RFE.
I decided to fix it in these changes.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1550285238
Re: RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass [v2]
On Wed, 3 Apr 2024 13:25:36 GMT, Coleen Phillimore wrote: >> This change simplifies the code that grows the jmethodID cache in >> InstanceKlass. Instead of lazily, when there's a rare request for a >> jmethodID for an obsolete method, the jmethodID cache is grown during the >> RedefineClasses safepoint. The InstanceKlass's jmethodID cache is lazily >> allocated when there's a jmethodID allocated, so not every InstanceKlass has >> a cache, but the growth now only happens in a safepoint. This code will >> become racy with the potential change for deallocating jmethodIDs. >> >> Tested with tier1-4, vmTestbase/nsk/jvmti java/lang/instrument tests (in >> case they're not in tier1-4). > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Refactoring suggested by Serguei. Still looks good - Marked as reviewed by amenkov (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18549#pullrequestreview-1977650540
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v2]
On Wed, 3 Apr 2024 15:49:00 GMT, Stefan Karlsson wrote:
>> Vladimir Kozlov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Removed not_used state of nmethod
>
> src/hotspot/share/runtime/frame.cpp line 208:
>
>> 206: address frame::raw_pc() const {
>> 207: if (is_deoptimized_frame()) {
>> 208: nmethod* nm = cb()->as_nmethod_or_null();
>
> Prexisting: It's weird that this code is using the `_or_null()` version when
> the code below does not null check the returned value.
Before [JDK-6921352](https://bugs.openjdk.org/browse/JDK-6921352) it was:
return ((nmethod*) cb())->deopt_handler_begin() - pc_return_offset;
I will add assert with check for null. We definitely expect here only nmethod.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1550243895
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v2]
On Wed, 3 Apr 2024 16:38:13 GMT, Vladimir Kozlov wrote:
>> src/hotspot/share/code/codeCache.cpp line 1009:
>>
>>> 1007: int CodeCache::nmethod_count() {
>>> 1008: int count = 0;
>>> 1009: for (GrowableArrayIterator heap =
>>> _nmethod_heaps->begin(); heap != _nmethod_heaps->end(); ++heap) {
>>
>> Is there a reason why FOR_ALL_NMETHOD_HEAPS wasn't good fit here? I'm
>> wondering since the similar `CodeCache::blob_count()` still uses one of
>> these macros.
>
> No, `CodeCache::blob_count()` uses different macro `FOR_ALL_HEAPS(heap)`
> because it looks for all code blobs, not only nmethods.
>
> `CodeCache::nmethod_count()` is the only place where `FOR_ALL_NMETHOD_HEAPS `
> was used. So I decided to remove the macro.
I didn't say that blob_count used `FOR_ALL_NMETHODS_HEAP`. I wrote "one of
these macros". I still think this adds an inconsistency to the code that I
don't think is beneficial.
With that said, can't this be written as:
for (CodeHeap* heap : *_nmethod_heaps) {
Maybe yet another opportunity for cleanups.
>> src/hotspot/share/code/nmethod.cpp line 812:
>>
>>> 810: // By calling this nmethod entry barrier, it plays along and acts
>>> 811: // like any other nmethod found on the stack of a thread (fewer
>>> surprises).
>>> 812: nmethod* nm = as_nmethod_or_null();
>>
>> Calling as_nmethod_or_null() from within functions in the nmethod class is
>> suspicious. Shouldn't all such usages be removed? (I'm fine with doing that
>> as a separate change)
>
> Good catch! The code was moved from CompiledMethod where it made sense but
> now it is not needed. Here the change I will make:
>
> // like any other nmethod found on the stack of a thread (fewer
> surprises).
> -nmethod* nm = as_nmethod_or_null();
> -if (nm != nullptr && bs_nm->is_armed(nm)) {
> +nmethod* nm = this;
> +if (bs_nm->is_armed(nm)) {
>bool alive = bs_nm->nmethod_entry_barrier(nm);
Sounds good.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1550216628
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1550217712
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v2]
On Wed, 3 Apr 2024 16:29:03 GMT, Vladimir Kozlov wrote:
>> src/hotspot/share/code/codeBlob.hpp line 409:
>>
>>> 407:
>>> 408: // GC/Verification support
>>> 409: virtual void preserve_callee_argument_oops(frame fr, const
>>> RegisterMap *reg_map, OopClosure* f) override { /* nothing to do */ }
>>
>> In the GC code we usually have either virtual OR override, but not both.
>> Could we skip `virtual` here? Or does the compiler code usually use both?
>
> No special rules here. I simply want to see all `virtual` methods explicitly
> and `override` is required by C++.
> I would like to keep it this way in these changes. I am investigating
> possibility to convert all these virtual methods to normal one to remove
> virtual table and virtual pointer (8 bytes) from CodeBlob class.
`override` is not required by C++. You do however mark all virtual methods with
`override` if any of the functions are marked with `override`. I think it would
be good to have a HotSpot code style discussion about this (but not in this
RFE).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1550206804
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v2]
On Wed, 3 Apr 2024 15:30:00 GMT, Stefan Karlsson wrote:
>> Vladimir Kozlov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Removed not_used state of nmethod
>
> src/hotspot/share/compiler/compileBroker.cpp line 1379:
>
>> 1377: if (osr_bci == InvocationEntryBci) {
>> 1378: // standard compilation
>> 1379: nmethod* method_code = method->code();
>
> Isn't the `method_code->is_nmethod()` redundant now?
An other good catch! It leads me to chase all redundant `is_nmethod()` and
`as_nmethod_*()` calls.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1550187397
Integrated: JDK-8327474 Review use of java.io.tmpdir in jdk tests
On Mon, 18 Mar 2024 16:47:24 GMT, Bill Huang wrote: > This task addresses an essential aspect of our testing infrastructure: the > proper handling and cleanup of temporary files and socket files created > during test execution. The motivation behind these changes is to prevent the > accumulation of unnecessary files in the default temporary directory, which > can affect the system's storage and potentially influence subsequent test > runs. > > Our review identified that several tests create temporary files or socket > files without ensuring their removal post-execution. > - Direct calls to java.io.File.createTempFile and > java.nio.file.Files.createTempFile without adequate cleanup. > - Tests using NIO socket channels with StandardProtocolFamily.UNIX, not > explicitly removing socket files post-use. This pull request has now been integrated. Changeset: 375bfac8 Author:Bill Huang URL: https://git.openjdk.org/jdk/commit/375bfac8e7ff3f871e2d986876f91a5fba200c83 Stats: 193 lines in 11 files changed: 66 ins; 13 del; 114 mod 8327474: Review use of java.io.tmpdir in jdk tests Reviewed-by: michaelm, jpai - PR: https://git.openjdk.org/jdk/pull/18352
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v2]
On Wed, 3 Apr 2024 15:12:31 GMT, Stefan Karlsson wrote:
>> Vladimir Kozlov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Removed not_used state of nmethod
>
> src/hotspot/share/code/nmethod.cpp line 812:
>
>> 810: // By calling this nmethod entry barrier, it plays along and acts
>> 811: // like any other nmethod found on the stack of a thread (fewer
>> surprises).
>> 812: nmethod* nm = as_nmethod_or_null();
>
> Calling as_nmethod_or_null() from within functions in the nmethod class is
> suspicious. Shouldn't all such usages be removed? (I'm fine with doing that
> as a separate change)
Good catch! The code was moved from CompiledMethod where it made sense but now
it is not needed. Here the change I will make:
// like any other nmethod found on the stack of a thread (fewer surprises).
-nmethod* nm = as_nmethod_or_null();
-if (nm != nullptr && bs_nm->is_armed(nm)) {
+nmethod* nm = this;
+if (bs_nm->is_armed(nm)) {
bool alive = bs_nm->nmethod_entry_barrier(nm);
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1550118967
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v2]
On Wed, 3 Apr 2024 15:01:22 GMT, Stefan Karlsson wrote:
>> Vladimir Kozlov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Removed not_used state of nmethod
>
> src/hotspot/share/code/codeCache.cpp line 1009:
>
>> 1007: int CodeCache::nmethod_count() {
>> 1008: int count = 0;
>> 1009: for (GrowableArrayIterator heap =
>> _nmethod_heaps->begin(); heap != _nmethod_heaps->end(); ++heap) {
>
> Is there a reason why FOR_ALL_NMETHOD_HEAPS wasn't good fit here? I'm
> wondering since the similar `CodeCache::blob_count()` still uses one of these
> macros.
No, `CodeCache::blob_count()` uses different macro `FOR_ALL_HEAPS(heap)`
because it looks for all code blobs, not only nmethods.
`CodeCache::nmethod_count()` is the only place where `FOR_ALL_NMETHOD_HEAPS `
was used. So I decided to remove the macro.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1550087255
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v2]
On Wed, 3 Apr 2024 14:44:03 GMT, Stefan Karlsson wrote:
>> Vladimir Kozlov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Removed not_used state of nmethod
>
> src/hotspot/share/code/codeBlob.hpp line 409:
>
>> 407:
>> 408: // GC/Verification support
>> 409: virtual void preserve_callee_argument_oops(frame fr, const
>> RegisterMap *reg_map, OopClosure* f) override { /* nothing to do */ }
>
> In the GC code we usually have either virtual OR override, but not both.
> Could we skip `virtual` here? Or does the compiler code usually use both?
No special rules here. I simply want to see all `virtual` methods explicitly
and `override` is required by C++.
I would like to keep it this way in these changes. I am investigating
possibility to convert all these virtual methods to normal one to remove
virtual table and virtual pointer (8 bytes) from CodeBlob class.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18554#discussion_r1550071713
Re: RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass [v2]
On Wed, 3 Apr 2024 13:25:36 GMT, Coleen Phillimore wrote: >> This change simplifies the code that grows the jmethodID cache in >> InstanceKlass. Instead of lazily, when there's a rare request for a >> jmethodID for an obsolete method, the jmethodID cache is grown during the >> RedefineClasses safepoint. The InstanceKlass's jmethodID cache is lazily >> allocated when there's a jmethodID allocated, so not every InstanceKlass has >> a cache, but the growth now only happens in a safepoint. This code will >> become racy with the potential change for deallocating jmethodIDs. >> >> Tested with tier1-4, vmTestbase/nsk/jvmti java/lang/instrument tests (in >> case they're not in tier1-4). > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Refactoring suggested by Serguei. Looks good to me. Nice simplification. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18549#pullrequestreview-1977358735
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v2]
On Wed, 3 Apr 2024 02:55:52 GMT, Vladimir Ivanov wrote: > What about `CompiledMethod_lock`? There's no `CompiledMethod` anymore, but > the lock name still refers to it. It was different changes [JDK-8226705](https://bugs.openjdk.org/browse/JDK-8226705). Renaming it will complicate these changes more than I wanted. I can do it in separate RFE. - PR Comment: https://git.openjdk.org/jdk/pull/18554#issuecomment-2035031413
Re: RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes [v2]
On Mon, 1 Apr 2024 21:07:31 GMT, Vladimir Kozlov wrote:
>> Revert [JDK-8152664](https://bugs.openjdk.org/browse/JDK-8152664) RFE
>> [changes](https://github.com/openjdk/jdk/commit/b853eb7f5ca24eeeda18acbb14287f706499c365)
>> which was used for AOT [JEP 295](https://openjdk.org/jeps/295)
>> implementation in JDK 9. The code was left in HotSpot assuming it will help
>> in a future. But during work on Leyden we decided to not use it. In Leyden
>> cached compiled code will be restored in CodeCache as normal nmethods: no
>> need to change VM's runtime and GC code to process them.
>>
>> I may work on optimizing `CodeBlob` and `nmethod` fields layout to reduce
>> header size in separate changes. In these changes I did simple fields
>> reordering to keep small (1 byte) fields together.
>>
>> I do not see (and not expected) performance difference with these changes.
>>
>> Tested tier1-5, xcomp, stress. Running performance testing.
>>
>> I need help with testing on platforms which Oracle does not support.
>
> Vladimir Kozlov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Removed not_used state of nmethod
Nice!
We've wanted to clean up some interfaces between the CodeCache and the GC code
by using nmethod closures instead of CodeBlob closures. This change (and the
Sweeper removal) makes it possible to do those cleanups.
I've made a superficial pass over the patch to and left a few comments. Most of
those comments are things that would be nice to fix, but could also be left as
follow-up RFEs (if they are deemed to be worthy ideas to pursue).
src/hotspot/os/posix/signals_posix.cpp line 27:
> 25: #include "precompiled.hpp"
> 26: #include "code/codeCache.hpp"
> 27: #include "code/nmethod.hpp"
The include line needs to move down.
src/hotspot/share/code/codeBlob.hpp line 77:
> 75: // - data space
> 76:
> 77: enum CodeBlobKind : u1 {
It will probably be safer to change this to an enum class, so that the compiler
will help us if we mess up with the argument order when this is used in
function calls. I see that this patch switches the parameter order of some
functions, so I think it could be worth trying out.
src/hotspot/share/code/codeBlob.hpp line 409:
> 407:
> 408: // GC/Verification support
> 409: virtual void preserve_callee_argument_oops(frame fr, const RegisterMap
> *reg_map, OopClosure* f) override { /* nothing to do */ }
In the GC code we usually have either virtual OR override, but not both. Could
we skip `virtual` here? Or does the compiler code usually use both?
src/hotspot/share/code/codeBlob.hpp line 429:
> 427:SingletonBlob(
> 428: const char* name,
> 429: CodeBlobKind kind,
There's an alignment issue after this change.
src/hotspot/share/code/codeCache.cpp line 1009:
> 1007: int CodeCache::nmethod_count() {
> 1008: int count = 0;
> 1009: for (GrowableArrayIterator heap = _nmethod_heaps->begin();
> heap != _nmethod_heaps->end(); ++heap) {
Is there a reason why FOR_ALL_NMETHOD_HEAPS wasn't good fit here? I'm wondering
since the similar `CodeCache::blob_count()` still uses one of these macros.
src/hotspot/share/code/nmethod.cpp line 812:
> 810: // By calling this nmethod entry barrier, it plays along and acts
> 811: // like any other nmethod found on the stack of a thread (fewer
> surprises).
> 812: nmethod* nm = as_nmethod_or_null();
Calling as_nmethod_or_null() from within functions in the nmethod class is
suspicious. Shouldn't all such usages be removed? (I'm fine with doing that as
a separate change)
src/hotspot/share/code/nmethod.cpp line 1009:
> 1007: // Fill in default values for various flag fields
> 1008: void nmethod::init_defaults() {
> 1009: { // avoid uninitialized fields, even for short time periods
Should these curly braces be removed?
src/hotspot/share/code/nmethod.cpp line 2164:
> 2162: DTRACE_METHOD_UNLOAD_PROBE(method());
> 2163:
> 2164: // If a JVMTI agent has enabled the nmethod Unload event then
I think the event is still called CompiledMethodUnload, so this line should
probably be reverted.
src/hotspot/share/code/nmethod.hpp line 50:
> 48: class ScopeDesc;
> 49: class CompiledIC;
> 50: class MetadataClosure;
Maybe merge (and sort) this together with the other forward declarations?
src/hotspot/share/code/nmethod.hpp line 905:
> 903:
> 904: // printing support
> 905: void print() const override;
Here and a few other places you only use override and not also virtual. This is
inconsistent with other functions in this class. (FWIW, I prefer this style
with only the override qualifier).
src/hotspot/share/code/nmethod.inline.hpp line 60:
> 58: // (b) it is a deopt PC
> 59:
> 60: inline address nmethod::get_deopt_original_pc(const frame* fr) {
While reading this PR I wonder if this really belongs in the `nmethod` class or
if it would make more sense to have it as a member function in the `frame`
Re: RFR: JDK-8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set [v10]
On Wed, 27 Mar 2024 13:44:42 GMT, Matthias Baesken wrote: >> Currently jcmd command GC.heap_dump only works with an additionally provided >> file name. >> Syntax : GC.heap_dump [options] >> >> In case the JVM has the XX - flag HeapDumpPath set, we should support an >> additional mode where the is optional. >> In case the filename is NOT set, we take the HeapDumpPath (file or >> directory); >> >> new syntax : >> GC.heap_dump [options] .. has precedence over second option >> GC.heap_dump [options] …in case -XX: HeapDumpPath=p is set >> >> This would be a simplification e.g. for support cases where a filename or >> directory is set at JVM startup with -XX: HeapDumpPath=p and writing to the >> path is intended/recommended for usage also in the jcmd case. > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > adjust java.1 man page Couldn't we just have an unconditional default for the GC.heap_dump `filename` option? This would simplify the documentation. E.g. we could write `If not specified, defaults to java_pid.hprof in the working directory or value configured through -XX:HeapDumpPath` As for the general value of this feature, I think it can simplify the handling of certain (cloud) deployment configurations where you have a dedicated filesystem that you want to write these kind of dumps to. It might also be nice if there was a default value for filename instead of the obligation to specify something. And, for those who can't see the upside of it all, I at least can't see no harm... - PR Comment: https://git.openjdk.org/jdk/pull/18190#issuecomment-2034639628
Re: RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass [v2]
On Wed, 3 Apr 2024 12:42:30 GMT, Coleen Phillimore wrote:
>> src/hotspot/share/oops/instanceKlass.cpp line 2335:
>>
>>> 2333: jmethodID new_id = Method::make_jmethod_id(class_loader_data(),
>>> method);
>>> 2334: Atomic::release_store([idnum+1], new_id);
>>> 2335: return new_id;
>>
>> Nit: It feels like the function `InstanceKlass::get_jmethod_id()` can be
>> more simplified with a small restructuring:
>>
>> jmethodID update_jmethod_id(jmethodID* jmeths, Method* method, int idnum) {
>> // method_with_idnum
>> if (method->is_old() && !method->is_obsolete()) {
>> // The method passed in is old (but not obsolete), we need to use the
>> current version
>> method = method_with_idnum((int)idnum);
>> assert(method != nullptr, "old and but not obsolete, so should exist");
>> }
>> jmethodID new_id = Method::make_jmethod_id(class_loader_data(), method);
>> Atomic::release_store([idnum+1], new_id);
>> return new_id;
>> }
>>
>> jmethodID InstanceKlass::get_jmethod_id(const methodHandle& method_h) {
>> Method* method = method_h();
>> int idnum = method_h->method_idnum();
>> jmethodID* jmeths = methods_jmethod_ids_acquire();
>>
>> <... big comment ...>
>> MutexLocker ml(JmethodIdCreation_lock, Mutex::_no_safepoint_check_flag);
>> if (jmeths == nullptr) {
>> jmeths = methods_jmethod_ids_acquire();
>>
>> if (jmeths == nullptr) { // Still null?
>> size_t size = idnum_allocated_count();
>> assert(size > (size_t)idnum, "should already have space");
>> jmeths = NEW_C_HEAP_ARRAY(jmethodID, size+1, mtClass);
>> memset(jmeths, 0, (size+1)*sizeof(jmethodID));
>> // cache size is stored in element[0], other elements offset by one
>> jmeths[0] = (jmethodID)size;
>> jmethodID new_id = update_jmethod_id(jmeths, method, idnum);
>>
>> // publish jmeths
>> release_set_methods_jmethod_ids(jmeths);
>> return new_id;
>> }
>> }
>> jmethodID id = Atomic::load_acquire([idnum+1]);
>> if (id == nullptr) {
>> id = jmeths[idnum+1];
>>
>> if (id == nullptr) { // Still null?
>> jmethodID new_id = update_jmethod_id(jmeths, method, idnum);
>> return new_id;
>> }
>> }
>> return id;
>> }
>
> Yes this refactoring looks nice. Nice to have only one place that checks for
> !is_obsolete.
Thank you for the suggestion, I reran the jvmti tests locally.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1549733870
Re: RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass [v2]
> This change simplifies the code that grows the jmethodID cache in > InstanceKlass. Instead of lazily, when there's a rare request for a > jmethodID for an obsolete method, the jmethodID cache is grown during the > RedefineClasses safepoint. The InstanceKlass's jmethodID cache is lazily > allocated when there's a jmethodID allocated, so not every InstanceKlass has > a cache, but the growth now only happens in a safepoint. This code will > become racy with the potential change for deallocating jmethodIDs. > > Tested with tier1-4, vmTestbase/nsk/jvmti java/lang/instrument tests (in case > they're not in tier1-4). Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Refactoring suggested by Serguei. - Changes: - all: https://git.openjdk.org/jdk/pull/18549/files - new: https://git.openjdk.org/jdk/pull/18549/files/e38f..6576d14d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18549=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18549=00-01 Stats: 32 lines in 2 files changed: 12 ins; 16 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/18549.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18549/head:pull/18549 PR: https://git.openjdk.org/jdk/pull/18549
Re: RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass
On Wed, 3 Apr 2024 02:41:06 GMT, Serguei Spitsyn wrote:
>> This change simplifies the code that grows the jmethodID cache in
>> InstanceKlass. Instead of lazily, when there's a rare request for a
>> jmethodID for an obsolete method, the jmethodID cache is grown during the
>> RedefineClasses safepoint. The InstanceKlass's jmethodID cache is lazily
>> allocated when there's a jmethodID allocated, so not every InstanceKlass has
>> a cache, but the growth now only happens in a safepoint. This code will
>> become racy with the potential change for deallocating jmethodIDs.
>>
>> Tested with tier1-4, vmTestbase/nsk/jvmti java/lang/instrument tests (in
>> case they're not in tier1-4).
>
> src/hotspot/share/oops/instanceKlass.cpp line 2335:
>
>> 2333: jmethodID new_id = Method::make_jmethod_id(class_loader_data(),
>> method);
>> 2334: Atomic::release_store([idnum+1], new_id);
>> 2335: return new_id;
>
> Nit: It feels like the function `InstanceKlass::get_jmethod_id()` can be more
> simplified with a small restructuring:
>
> jmethodID update_jmethod_id(jmethodID* jmeths, Method* method, int idnum) {
> // method_with_idnum
> if (method->is_old() && !method->is_obsolete()) {
> // The method passed in is old (but not obsolete), we need to use the
> current version
> method = method_with_idnum((int)idnum);
> assert(method != nullptr, "old and but not obsolete, so should exist");
> }
> jmethodID new_id = Method::make_jmethod_id(class_loader_data(), method);
> Atomic::release_store([idnum+1], new_id);
> return new_id;
> }
>
> jmethodID InstanceKlass::get_jmethod_id(const methodHandle& method_h) {
> Method* method = method_h();
> int idnum = method_h->method_idnum();
> jmethodID* jmeths = methods_jmethod_ids_acquire();
>
> <... big comment ...>
> MutexLocker ml(JmethodIdCreation_lock, Mutex::_no_safepoint_check_flag);
> if (jmeths == nullptr) {
> jmeths = methods_jmethod_ids_acquire();
>
> if (jmeths == nullptr) { // Still null?
> size_t size = idnum_allocated_count();
> assert(size > (size_t)idnum, "should already have space");
> jmeths = NEW_C_HEAP_ARRAY(jmethodID, size+1, mtClass);
> memset(jmeths, 0, (size+1)*sizeof(jmethodID));
> // cache size is stored in element[0], other elements offset by one
> jmeths[0] = (jmethodID)size;
> jmethodID new_id = update_jmethod_id(jmeths, method, idnum);
>
> // publish jmeths
> release_set_methods_jmethod_ids(jmeths);
> return new_id;
> }
> }
> jmethodID id = Atomic::load_acquire([idnum+1]);
> if (id == nullptr) {
> id = jmeths[idnum+1];
>
> if (id == nullptr) { // Still null?
> jmethodID new_id = update_jmethod_id(jmeths, method, idnum);
> return new_id;
> }
> }
> return id;
> }
Yes this refactoring looks nice. Nice to have only one place that checks for
!is_obsolete.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18549#discussion_r1549664452
Integrated: 8236736: Change notproduct JVM flags to develop flags
On Thu, 28 Mar 2024 22:53:22 GMT, Coleen Phillimore wrote: > Remove the notproduct distinction for command line options, rather than > trying to wrestle the macros to fix the bug that they've been treated as > develop options for some time now. This simplifies the command line option > macros. > > Tested with tier1-4, tier1 on Oracle platforms. Also built shenandoah. This pull request has now been integrated. Changeset: bea493bc Author:Coleen Phillimore URL: https://git.openjdk.org/jdk/commit/bea493bcb86370dc3fb00d86c545f01fc614e000 Stats: 282 lines in 43 files changed: 1 ins; 102 del; 179 mod 8236736: Change notproduct JVM flags to develop flags Reviewed-by: iklam, kvn, kbarrett - PR: https://git.openjdk.org/jdk/pull/18541
Re: RFR: 8236736: Change notproduct JVM flags to develop flags [v4]
On Tue, 2 Apr 2024 19:47:23 GMT, Coleen Phillimore wrote: >> Remove the notproduct distinction for command line options, rather than >> trying to wrestle the macros to fix the bug that they've been treated as >> develop options for some time now. This simplifies the command line option >> macros. >> >> Tested with tier1-4, tier1 on Oracle platforms. Also built shenandoah. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Remove redundant test case. Thanks for the reviews, Ioi, Vladimir, Kim and Stefan. - PR Comment: https://git.openjdk.org/jdk/pull/18541#issuecomment-2034433313
Re: RFR: JDK-8322042: HeapDumper should perform merge on the current thread instead of VMThread [v2]
On Tue, 2 Apr 2024 21:13:33 GMT, Alex Menkov wrote: >> The fix updated HeapDumper to always perform merge on the current thread. >> >> Testing: tier1-5, all HeapDump-related tests >> Covered heap dumping scenarios: >> - `jcmd GC.heap_dump` command; >> - `HotSpotDiagnosticMXBean.dumpHeap()`; >> - `HeapDumpBeforeFullGC`, `HeapDumpAfterFullGC` VM options; >> - `HeapDumpOnOutOfMemoryError` VM option. > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > comment Are we saying there is never any need to perform the merge in a VM Operation? Originally (JDK-8306441) it's either done in the attach thread, or a VM operation if we are in another thread. But maybe that was just being cautious. Keeping only the invoking thread busy, rather than a VM Operation, sounds like a good goal. I'm interested in if we can hit a problem by causing multiple heap dumps at once, e.g. jcmd, the app calling the MXBean, and maybe even the OnOOM trigger as well, could all happen at the same time. 8-) Using the MXBean, we may be able to make multiple heap dumps concurrently (either multiple MXBean invocations, or one MXBean invocation while a jcmd runs...). jcmd keeps the atttach listener thread busy, so we can't have concurrent dumps from that method. I think that explains the original code, where all other methods use the VM Operation. - PR Review: https://git.openjdk.org/jdk/pull/18571#pullrequestreview-1976273140
Re: RFR: JDK-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.
The build changes look okay.
Do you have any plan of going through all the Java modules and fixing the
issues, or opening JBS issues to have them fixed? Or will these lint warnings
remain disabled for the foreseeable future?
-
Marked as reviewed by ihse (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18527#pullrequestreview-1976255818
Re: RFR: JDK-8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set [v10]
On Fri, 29 Mar 2024 04:15:24 GMT, Chris Plummer wrote: > There's still the question of whether or not it is even appropriate to have > -XX options taking the place of jcmd options. Some people (like our cloud support colleagues and also some who commented) would like this approach, some find it not very useful. Seems there are also already other globals flags playing a role in diagnosticCommand coding (e.g. RecordDynamicDumpInfo, maybe more) so it is not completely 'new' that the jcmd commands are influenced by globals flag. Looking at just the _names_ of HeapDumpPath and also HeapDumpGzipLevel I would think that they play a role for all heap dumps and not just some. - PR Comment: https://git.openjdk.org/jdk/pull/18190#issuecomment-2033985633
Re: RFR: JDK-8327769: jcmd GC.heap_dump without options should write to location given by -XX:HeapDumpPath, if set [v10]
On Fri, 29 Mar 2024 04:15:24 GMT, Chris Plummer wrote: > There's also a question of whether currently missing doc updates for > HeapDumpGzipLevel should be made part of this PR > because it complicates back porting. It should most likely be a separate PR (the title of this one does not match, and there are different backporting needs). - PR Comment: https://git.openjdk.org/jdk/pull/18190#issuecomment-2033834669
