Re: RFR: 8332327: Return _methods_jmethod_ids field back in VMStructs
On Wed, 15 May 2024 21:12:03 GMT, Andrei Pangin wrote: > The fix for [JDK-8313332](https://bugs.openjdk.org/browse/JDK-8313332) has > [removed](https://github.com/openjdk/jdk/commit/21867c929a2f2c961148f2cd1e79d672ac278d27#diff-7d448441e80a0b784429d5d8aee343fcb131c224b8ec7bc70ea636f78d561ecd > ) `InstanceKlass::_methods_jmethod_ids` field from VMStructs. > > This broke > [async-profiler](https://github.com/async-profiler/async-profiler/), since > the profiler needs this field to obtain jmethodID in some corner cases. > > There was no actual need for removal, as the field is still there in > InstanceKlass. So, I propose to return the field back to restore the broken > functionality of async-profiler. This is a no risk change, because it only > exports an offset of one field and does not affect the JVM otherwise. This is fine to revert. I didn't see this field in SA so eagerly removed it. - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19254#pullrequestreview-2060857374
Re: RFR: 8329113: Deprecate -XX:+UseNotificationThread
On Fri, 19 Apr 2024 01:16:46 GMT, Alex Menkov wrote: > The fix deprecates -XX:+UseNotificationThread VM option > > Testing: tier1-3,hs-tier5-svc Looks good. Thank you for doing this. - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18852#pullrequestreview-2017009355
Re: RFR: 8330388: Remove invokedynamic cache index encoding [v2]
On Thu, 18 Apr 2024 16:22:31 GMT, Matias Saavedra Silva wrote: >> Before [JDK-8307190](https://bugs.openjdk.org/browse/JDK-8307190), >> [JDK-8309673](https://bugs.openjdk.org/browse/JDK-8309673), and >> [JDK-8301995](https://bugs.openjdk.org/browse/JDK-8301995), invokedynamic >> operands needed to be rewritten to encoded values to better distinguish indy >> entries from other cp cache entries. The above changes now distinguish >> between entries with `to_cp_index()` using the bytecode, which is now >> propagated by the callers. >> >> The encoding flips the bits of the index so the encoded index is always >> negative, leading to access errors if there is no matching decode call. >> These calls are removed with some methods adjusted to distinguish between >> indices with the bytecode. Verified with tier 1-5 tests. The changes show no >> issues when tested against libgraal. > > Matias Saavedra Silva has updated the pull request incrementally with one > additional commit since the last revision: > > Dean and Gilles comments Still good! - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18819#pullrequestreview-2015844122
Re: RFR: 8330388: Remove invokedynamic cache index encoding
On Wed, 17 Apr 2024 15:26:52 GMT, Matias Saavedra Silva wrote: > Before [JDK-8307190](https://bugs.openjdk.org/browse/JDK-8307190), > [JDK-8309673](https://bugs.openjdk.org/browse/JDK-8309673), and > [JDK-8301995](https://bugs.openjdk.org/browse/JDK-8301995), invokedynamic > operands needed to be rewritten to encoded values to better distinguish indy > entries from other cp cache entries. The above changes now distinguish > between entries with `to_cp_index()` using the bytecode, which is now > propagated by the callers. > > The encoding flips the bits of the index so the encoded index is always > negative, leading to access errors if there is no matching decode call. These > calls are removed with some methods adjusted to distinguish between indices > with the bytecode. Verified with tier 1-5 tests. To borrow @shipilev's comment from a different PR, Good Riddance! - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18819#pullrequestreview-2008602360
Re: RFR: 8329433: Reduce nmethod header size [v4]
On Tue, 16 Apr 2024 18:54:40 GMT, Vladimir Kozlov wrote: >> src/hotspot/share/code/codeBlob.cpp line 106: >> >>> 104: >>> 105: // Simple CodeBlob used for simple BufferBlob. >>> 106: CodeBlob::CodeBlob(const char* name, CodeBlobKind kind, int size, >>> uint16_t header_size) : >> >> Just a drive-by comment. You might be able to use delegating constructors >> for CodeBlob so you don't have to have the field initializations twice. >> Maybe the same for nmethod ? > > Thank you, @coleenp, foe looking on these changes. > > Which fields are initialized twice? Only `_oop_maps` is set to `nullptr` > before we proper build oop maps in first constructor. > > The only saving could be lines of code but then I would have to check that > `cb != nullptr` and do other additional checks which I don't think will save > much lines. > > Separation of `nmethod` constructor for native wrappers is helping clear see > the difference and I would like to keep them separate. We have > `init_defaults()` method for similar code and I can move more code into it > from both constructors. Delegating constructors are the answer to having some common 'init' functions. It would simply save lines of code that look the same in both constructor initializer lists. But it's a drive-by comment. - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567823393
Re: RFR: 8329433: Reduce nmethod header size [v4]
On Tue, 16 Apr 2024 03:31:25 GMT, Vladimir Kozlov wrote: >> This is part of changes which try to reduce size of `nmethod` and `codeblob` >> data vs code in CodeCache. >> These changes reduced size of `nmethod` header from 288 to 232 bytes. From >> 304 to 248 in optimized VM: >> >> Statistics for 1282 bytecoded nmethods for C2: >> total in heap = 5560352 (100%) >> header = 389728 (7.009053%) >> >> vs >> >> Statistics for 1322 bytecoded nmethods for C2: >> total in heap = 8307120 (100%) >> header = 327856 (3.946687%) >> >> >> Several unneeded fields in `nmethod` and `CodeBlob` were removed. Some >> fields were changed from `int` to `int16_t` with added corresponding asserts >> to make sure their values are fit into 16 bits. >> >> I did additional cleanup after recent `CompiledMethod` removal. >> >> Tested tier1-7,stress,xcomp and performance testing. > > Vladimir Kozlov has updated the pull request incrementally with one > additional commit since the last revision: > > Use 16-bits types for header_size and frame_complete_offset arguments src/hotspot/share/code/codeBlob.cpp line 106: > 104: > 105: // Simple CodeBlob used for simple BufferBlob. > 106: CodeBlob::CodeBlob(const char* name, CodeBlobKind kind, int size, > uint16_t header_size) : Just a drive-by comment. You might be able to use delegating constructors for CodeBlob so you don't have to have the field initializations twice. Maybe the same for nmethod ? - PR Review Comment: https://git.openjdk.org/jdk/pull/18768#discussion_r1567658758
Re: RFR: 8329655: Cleanup KlassObj and klassOop names after the PermGen removal [v2]
On Thu, 4 Apr 2024 12:19:03 GMT, Stefan Karlsson wrote: >> I'm not even sure what they want to say, really. Should be good to remove, >> and if anybody can make sense of it, record an issue in the bug-tracker? > > OK. I removed the %%%. I'll wait a little bit to see if someone else wants to > keep them for some reason, if not, I'll remove them. I think leaving these comments without the %%% seems fine. Describing this idea in a CR is a lot more difficult than seeing it in context as commentary, and unless the enhancement has other motivation, it won't be picked up. Leaving the comment as a clue seems useful. - PR Review Comment: https://git.openjdk.org/jdk/pull/18618#discussion_r1551686232
Re: RFR: 8329655: Cleanup KlassObj and klassOop names after the PermGen removal [v2]
On Thu, 4 Apr 2024 12:18:24 GMT, Stefan Karlsson wrote:
>> We have a few places that uses the terms `KlassObj` and `klassOop` when
>> referring to Klasses. This is old code from before the PermGen removal, when
>> Klasses also were Java objects.
>>
>> These names tripped me up when I was reading the heap heapInspection.cpp and
>> first though we were mixing the klass *mirror* objects and klass pointers in
>> the hash code calculation:
>>
>>// An aligned reference address (typically the least
>>// address in the perm gen) used for hashing klass
>>// objects.
>>HeapWord* _ref;
>> ...
>> _ref = (HeapWord*) Universe::boolArrayKlassObj();
>> ...
>> uint KlassInfoTable::hash(const Klass* p) {
>> return (uint)(((uintptr_t)p - (uintptr_t)_ref) >> 2);
>> }
>>
>>
>> I propose that we rename these functions (and stop casting the Klass* to a
>> (HeapWord*)).
>>
>> Tested with serviceability/dcmd/gc/ClassHistogramTest.java but will run this
>> through our lower tiers.
>
> Stefan Karlsson has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Review Roman
This is good. The Obj was confusing.
src/hotspot/share/memory/heapInspection.hpp line 111:
> 109:
> 110: // An aligned reference address (typically the least
> 111: // address in the perm gen) used for hashing klass
Rats I missed this.
-
Marked as reviewed by coleenp (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18618#pullrequestreview-1979901207
PR Review Comment: https://git.openjdk.org/jdk/pull/18618#discussion_r1551690175
Integrated: 8313332: Simplify lazy jmethodID cache in InstanceKlass
On Fri, 29 Mar 2024 15:25:48 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). This pull request has now been integrated. Changeset: 21867c92 Author:Coleen Phillimore URL: https://git.openjdk.org/jdk/commit/21867c929a2f2c961148f2cd1e79d672ac278d27 Stats: 229 lines in 7 files changed: 44 ins; 151 del; 34 mod 8313332: Simplify lazy jmethodID cache in InstanceKlass Reviewed-by: amenkov, sspitsyn, dcubed - PR: https://git.openjdk.org/jdk/pull/18549
Re: RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass [v4]
On Thu, 4 Apr 2024 00:07:34 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: > > Two more. I've run through tiers 5-7 with the patch now. Thank you for the reviews Alex, Serguei and Dan. - PR Comment: https://git.openjdk.org/jdk/pull/18549#issuecomment-2037185846
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: 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 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: 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 reviewing, Kim. Is your suggestion to not have a JVMFlag object for develop flags in PRODUCT builds? Presumably to save some footprint? I'm not sure we would win fighting the macros to accomplish this. - PR Comment: https://git.openjdk.org/jdk/pull/18541#issuecomment-2032981431
Re: RFR: 8236736: Change notproduct JVM flags to develop flags [v4]
> 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. - Changes: - all: https://git.openjdk.org/jdk/pull/18541/files - new: https://git.openjdk.org/jdk/pull/18541/files/00a241d3..3b5002d8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18541=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18541=02-03 Stats: 5 lines in 1 file changed: 0 ins; 5 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18541.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18541/head:pull/18541 PR: https://git.openjdk.org/jdk/pull/18541
Re: RFR: 8236736: Change notproduct JVM flags to develop flags [v3]
On Tue, 2 Apr 2024 17:58:16 GMT, Kim Barrett wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix a couple issues pointed out by Stefank.
>
> test/hotspot/jtreg/runtime/CommandLine/VMOptionWarning.java line 64:
>
>> 62: output = new OutputAnalyzer(pb.start());
>> 63: output.shouldNotHaveExitValue(0);
>> 64: output.shouldContain("Error: VM option 'CheckCompressedOops' is
>> develop and is available only in debug version of VM.");
>
> Seems like we don't need this test of the develop option CheckCompressedOops
> at all, since we have
> the immediately preceding test of the develop option VerifyStack.
You're right, we've already tested this.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18541#discussion_r1548488160
Re: RFR: JDK-8328137: PreserveAllAnnotations can cause failure of class retransformation
On Thu, 28 Mar 2024 22:12:49 GMT, Alex Menkov wrote: > PreserveAllAnnotations option causes class file parser to preserve > RuntimeInvisibleAnnotations so VM considers them as RuntimeVisibleAnnotations. > For class retransformation JvmtiClassFileReconstituter restores all > annotations as RuntimeVisibleAnnotations attributes. > This can cause problem is the class contains only > RuntimeInvisibleAnnotations, so corresponding RuntimeVisibleAnnotations > attribute name is not present in the class constant pool. > > Correct solution would be to store additional information about > RuntimeInvisibleAnnotations and restore them exactly as they were in the > original class (this should be done for all annotations: > RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations for class, fields > and records, > RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations/RuntimeInvisibleParameterAnnotations > for methods; need to ensure the information is correctly updated during > class redefinition & retransformation). > > I think it doesn't make sense to add all the complexity for almost no value > (I doubt anyone uses PreserveAllAnnotations, the flag looks like > experimental, we don't have any tests for it). > > The suggested fix adds workaround for this corner case - if "visible" > attribute name is not in the CP, the annotations are restored with > "invisible" attribute name. > > Testing: > - tier1,tier2,hs-tier5-svc > - all java/lang/instrument tests; > - all RedefineClasses/RetransformClasses tests At one point long ago, I was trying to understand why we have PreserveAllAnnotations and couldn't come up with a reason. For a class file reconstitutor, restoring the invisible annotations to the classfile and then feeding it back to the JVM should have no effect, since the VM doesn't do anything with these annotations. I see now why you get the original assert. I think this looks like a reasonable workaround for this problem. I wonder if we can deprecate PreserveAllAnnotations. Wonder what it's for? It would need a CSR because it's a product flag. - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18540#pullrequestreview-1974754738
Re: RFR: 8236736: Change notproduct JVM flags to develop flags [v3]
On Tue, 2 Apr 2024 17:25:12 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: > > Fix a couple issues pointed out by Stefank. Yes, I had to remember what optimized did. It gets all the options, but builds with optimization and doesn't turn on asserts. I only removed the notproduct flag distinction since it hasn't been distinct for years and it's confusing what we actually wanted it to do. - PR Comment: https://git.openjdk.org/jdk/pull/18541#issuecomment-2032794955
Re: RFR: 8236736: Change notproduct JVM flags to develop flags [v3]
On Tue, 2 Apr 2024 17:25:12 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: > > Fix a couple issues pointed out by Stefank. The optimized build still works as before (actually surprised it still builds). Since for a long time the notproduct options acted like develop options, they still do just the same for the optimized build. For optimized, all the develop and notproduct options are materialized. Now just develop, not distinguishing notproduct from that. The same code enabled in PRODUCT is still enabled. I haven't looked at that removing optimized bug in a while. - PR Comment: https://git.openjdk.org/jdk/pull/18541#issuecomment-2032704606
Re: RFR: 8236736: Change notproduct JVM flags to develop flags [v3]
> 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: Fix a couple issues pointed out by Stefank. - Changes: - all: https://git.openjdk.org/jdk/pull/18541/files - new: https://git.openjdk.org/jdk/pull/18541/files/19b8f6b6..00a241d3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18541=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18541=01-02 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18541.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18541/head:pull/18541 PR: https://git.openjdk.org/jdk/pull/18541
Re: RFR: 8236736: Change notproduct JVM flags to develop flags [v2]
On Tue, 2 Apr 2024 16:49:19 GMT, Stefan Karlsson wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Clean up notproduct from tests.
>
> src/hotspot/share/runtime/arguments.cpp line 3420:
>
>> 3418: static void apply_debugger_ergo() {
>> 3419: #ifndef PRODUCT
>> 3420: // UseDebuggerErgo is notproduct
>
> Now that the flag has been changed to a develop flag, it seems wrong that
> these are guarded by "#ifndef PRODUCT". Shouldn't this be changed to check
> for ASSERT instead?
Yes, ifdef ASSERT is more appropriate.
> src/hotspot/share/runtime/flags/jvmFlag.hpp line 118:
>
>> 116: EXPERIMENTAL_FLAG_BUT_LOCKED,
>> 117: DEVELOPER_FLAG_BUT_PRODUCT_BUILD,
>> 118: NOTPRODUCT_FLAG_BUT_PRODUCT_BUILD
>
> Should the ',' on the previous line be removed?
Yes, I guess our compilers don't complain about that anymore.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18541#discussion_r1548261310
PR Review Comment: https://git.openjdk.org/jdk/pull/18541#discussion_r1548269993
Re: RFR: 8236736: Change notproduct JVM flags to develop flags [v2]
On Tue, 2 Apr 2024 16:24:19 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: > > Clean up notproduct from tests. Thanks for looking through the changes, Stefan. - PR Review: https://git.openjdk.org/jdk/pull/18541#pullrequestreview-1974442423
Re: RFR: 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. Thank you for pointing out that I missed cleaning up the tests. One failed but I didn't see it. - PR Comment: https://git.openjdk.org/jdk/pull/18541#issuecomment-2032498785
Re: RFR: 8236736: Change notproduct JVM flags to develop flags [v2]
> 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: Clean up notproduct from tests. - Changes: - all: https://git.openjdk.org/jdk/pull/18541/files - new: https://git.openjdk.org/jdk/pull/18541/files/c3d9a1c8..19b8f6b6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18541=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18541=00-01 Stats: 37 lines in 4 files changed: 0 ins; 8 del; 29 mod Patch: https://git.openjdk.org/jdk/pull/18541.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18541/head:pull/18541 PR: https://git.openjdk.org/jdk/pull/18541
RFR: 8236736: Change notproduct JVM flags to develop flags
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. - Commit messages: - Missed one. - 8236736: Change notproduct JVM flags to develop flags Changes: https://git.openjdk.org/jdk/pull/18541/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18541=00 Issue: https://bugs.openjdk.org/browse/JDK-8236736 Stats: 239 lines in 39 files changed: 1 ins; 89 del; 149 mod Patch: https://git.openjdk.org/jdk/pull/18541.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18541/head:pull/18541 PR: https://git.openjdk.org/jdk/pull/18541
Re: RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass
On Fri, 29 Mar 2024 15:25:48 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). Thank you Alex! - PR Comment: https://git.openjdk.org/jdk/pull/18549#issuecomment-2031887624
RFR: 8313332: Simplify lazy jmethodID cache in InstanceKlass
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). - Commit messages: - Add Safepoint assert, don't use order accessors because they're not needed. - 8313332: Simplify lazy jmethodID cache in InstanceKlass Changes: https://git.openjdk.org/jdk/pull/18549/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18549=00 Issue: https://bugs.openjdk.org/browse/JDK-8313332 Stats: 222 lines in 7 files changed: 41 ins; 144 del; 37 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: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v3]
On Thu, 14 Mar 2024 22:09:55 GMT, Alex Menkov wrote: >> It does not matter what the `ClassFileParser` does. >> >>> ` JvmtiClassFileReconstituter` performs the reverse operation. >> >> True. It should know how to put the `attribute_count` value into the class >> file but it does not need to know how to calculate its value. What I do not >> like in your model is that there is no one single place which knows how to >> calculate this value and the existing and potential consumers should have >> this knowledge. >> >>> They may have different `attributute_count `(that's what this bug about). >> >> The bug is that the `attributute_count` field value from the class file >> stored in the `RecordComponent` does not match the calculated value because >> the invisible attribute was not saved (was ignored). My suggestion is not to >> store the `attributute_count` field value from the class file as it was >> before. The suggestion is to place the calculating function >> `attributute_count()` into the `RecordComponent` class. > >> True. It should know how to put the `attribute_count` value into the class >> file but it does not need to know how to calculate its value. What I do not >> like in your model is that there is no one single place which knows how to >> calculate this value and the existing and potential consumers should have >> this knowledge. > > It **must** know how to calculate the value. Because this is number of > `attribute_info_attributes` attributes that follow. And only > `JvmtiClassFileReconstituter` knows how many attributes it's going to write. > >> The bug is that the `attributute_count` field value from the class file >> stored in the `RecordComponent` does not match the calculated value because >> the invisible attribute was not saved (was ignored). > > I'd consider this as design issue with the current implementation - > `JvmtiClassFileReconstituter` gets the value from external source > (`RecordComponent`) and uses it without validation. > This approach is inconsistent with other `JvmtiClassFileReconstituter` code. > For other similar cases it calculates record counts in place (from size of > arrays, string length, from list enumerator, etc.). So this makes sense. You only want the number of attributes that are applicable to the RecordComponent, and that is the three attributes that you then write out in the class file reconstituter: generic_signature_index, annotations and type_annotations. - PR Review Comment: https://git.openjdk.org/jdk/pull/18161#discussion_r1539950465
Re: RFR: JDK-8315575: Retransform of record class with record component annotation fails with CFE [v7]
On Thu, 21 Mar 2024 18:10:49 GMT, Alex Menkov wrote: >> RecordComponent class has _attributes_count field. >> The only user of the field is JvmtiClassFileReconstituter. Incorrect value >> of the field causes producing incorrect data for Record attribute. >> Parsing Record attribute ClassFileParser skips unknown attributes and may >> skip RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations. >> Also annotations can be changed (added/removed) by class redefinition. >> The fix removes attributes_count from RecordComponent; >> JvmtiClassFileReconstituter calculates correct attributes_count generating >> class bytes. >> >> Testing: >> - tier1,tier2,hs-tier5-svc; >> - redefineClasses/retransformClasses tests: >>- test/jdk/java/lang/instrument >>- test/hotspot/jtreg/serviceability/jvmti/RedefineClasses >>- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses >>- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RetransformClasses > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > added comment This looks good. test/jdk/java/lang/instrument/RetransformRecordAnnotation.java line 30: > 28: * > 29: * @library /test/lib > 30: * @run shell MakeJAR.sh retransformAgent Does the presence of attributes mean you can't use the simpler RedefineClassHelper that the tests in test/hotspot/jtreg/serviceability/jvmti/RedefineClasses use? - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18161#pullrequestreview-1961478084 PR Review Comment: https://git.openjdk.org/jdk/pull/18161#discussion_r1539953736
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v3]
On Fri, 8 Mar 2024 06:17:06 GMT, David Holmes wrote:
>> Coleen Phillimore has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Missed a word.
>> - David's comment fixes.
>
> src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp line 121:
>
>> 119: {
>> 120: // To get a consistent list of classes we need MultiArray_lock to
>> ensure
>> 121: // array classes aren't created by another thread during this walk.
>> This walks through the
>
> Thanks for clarifying, though I'm not sure why would care given the set of
> classes could have changed by the time we return them anyway.
I think in this case, we might find an ObjArrayKlass without the mirror since
the Klass is added to the higher_dimension links before the mirror is created.
The lock keeps them both together.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1517716370
Integrated: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Tue, 6 Feb 2024 22:59:04 GMT, Coleen Phillimore wrote: > This change creates a new sort of native recursive lock that can be held > during JNI and Java calls, which can be used for synchronization while > creating objArrayKlasses at runtime. > > Passes tier1-7. This pull request has now been integrated. Changeset: 1877a487 Author:Coleen Phillimore URL: https://git.openjdk.org/jdk/commit/1877a4879598356b42fe80bdd5fa77ca8e8d Stats: 165 lines in 11 files changed: 87 ins; 43 del; 35 mod 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock Reviewed-by: dlong, dholmes, fparain - PR: https://git.openjdk.org/jdk/pull/17739
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v3]
On Thu, 7 Mar 2024 13:21:09 GMT, Coleen Phillimore wrote: >> This change creates a new sort of native recursive lock that can be held >> during JNI and Java calls, which can be used for synchronization while >> creating objArrayKlasses at runtime. >> >> Passes tier1-7. > > Coleen Phillimore has updated the pull request incrementally with two > additional commits since the last revision: > > - Missed a word. > - David's comment fixes. Thank you for reviewing, Dean, David and Fred. - PR Comment: https://git.openjdk.org/jdk/pull/17739#issuecomment-1985694821
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v2]
On Thu, 7 Mar 2024 01:44:22 GMT, David Holmes wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Dean's comments.
>
> src/hotspot/share/oops/instanceKlass.cpp line 1552:
>
>> 1550: RecursiveLocker rl(MultiArray_lock, THREAD);
>> 1551:
>> 1552: // This thread is the creator.
>
> This thread may not be the creator. The original comment was more apt - we
> typically say something like "Check if another thread beat us to it."
Fixed.
// Check if another thread created the array klass while we were waiting
for the lock.
> src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp line 121:
>
>> 119: {
>> 120: // To get a consistent list of classes we need MultiArray_lock to
>> ensure
>> 121: // array classes aren't created during this walk. This walks
>> through the
>
> Just curious but how can walking the set of classes trigger creation of array
> classes?
It can't. I'll reword that comment so it's clearer, with just a couple of
words.
// To get a consistent list of classes we need MultiArray_lock to ensure
// array classes aren't created by another thread this walk. This walks
through the
// InstanceKlass::_array_klasses links.
edit: added "during" to "by another thread during this walk"
> src/hotspot/share/runtime/mutexLocker.hpp line 335:
>
>> 333:
>> 334: // Instance of a RecursiveLock that may be held through Java heap
>> allocation, which may include calls to Java,
>> 335: // and JNI event notification for resource exhausted for metaspace or
>> heap.
>
> s/exhausted/exhaustion/
Fixed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1516126218
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1516128248
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1516130552
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v3]
On Thu, 7 Mar 2024 01:31:09 GMT, Coleen Phillimore wrote:
>> Semaphore seems simpler/cleaner and ready to use.
>
> It's such a rare race and unusual condition that we could execute more Java
> code, that I started out with just a shared bit. Then David suggested a
> semaphore that obeys the safepoint protocol, which seems a lot better. I've
> literally skimmed over OSThreadWaitState. It looks like
> Semaphore::wait_with_a_safepoint_check() uses it. I still don't know why it
> exists:
>
>
> // Note: the ThreadState is legacy code and is not correctly implemented.
> // Uses of ThreadState need to be replaced by the state in the JavaThread.
>
> enum ThreadState {
>
>
> Does a PlatformMutex handle priority-inheritance? It still feels like it
> would be overkill for this usage. I hope this RecursiveLocker does not
> become more widely used, where we would have to replace the simple
> implementation with something more difficult. We should discourage further
> use when possible.
Thanks for your last comment because I was worried there's a lot of other code
I should know about.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1516129405
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v2]
On Thu, 7 Mar 2024 01:38:56 GMT, Coleen Phillimore wrote: >> This change creates a new sort of native recursive lock that can be held >> during JNI and Java calls, which can be used for synchronization while >> creating objArrayKlasses at runtime. >> >> Passes tier1-7. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Dean's comments. David, thank you for your discussion while fixing this problem. - PR Review: https://git.openjdk.org/jdk/pull/17739#pullrequestreview-1922394063
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v3]
> This change creates a new sort of native recursive lock that can be held > during JNI and Java calls, which can be used for synchronization while > creating objArrayKlasses at runtime. > > Passes tier1-7. Coleen Phillimore has updated the pull request incrementally with two additional commits since the last revision: - Missed a word. - David's comment fixes. - Changes: - all: https://git.openjdk.org/jdk/pull/17739/files - new: https://git.openjdk.org/jdk/pull/17739/files/d64aa560..88fd6f13 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17739=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17739=01-02 Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/17739.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17739/head:pull/17739 PR: https://git.openjdk.org/jdk/pull/17739
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v2]
On Thu, 7 Mar 2024 00:54:30 GMT, David Holmes wrote:
>> OK. It's a good thing HotSpot doesn't need to worry about
>> priority-inheritance for mutexes.
>
> Semaphore seems simpler/cleaner and ready to use.
It's such a rare race and unusual condition that we could execute more Java
code, that I started out with just a shared bit. Then David suggested a
semaphore that obeys the safepoint protocol, which seems a lot better. I've
literally skimmed over OSThreadWaitState. It looks like
Semaphore::wait_with_a_safepoint_check() uses it. I still don't know why it
exists:
// Note: the ThreadState is legacy code and is not correctly implemented.
// Uses of ThreadState need to be replaced by the state in the JavaThread.
enum ThreadState {
Does a PlatformMutex handle priority-inheritance? It still feels like it would
be overkill for this usage. I hope this RecursiveLocker does not become more
widely used, where we would have to replace the simple implementation with
something more difficult. We should discourage further use when possible.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515364466
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v2]
On Thu, 7 Mar 2024 00:18:53 GMT, Dean Long wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Dean's comments.
>
> src/hotspot/share/oops/arrayKlass.cpp line 135:
>
>> 133: ResourceMark rm(THREAD);
>> 134: {
>> 135: // Ensure atomic creation of higher dimensions
>
> Isn't this comment still useful?
Yes, it's a useful comment, readded.
> src/hotspot/share/oops/arrayKlass.cpp line 144:
>
>> 142: ObjArrayKlass* ak =
>> 143: ObjArrayKlass::allocate_objArray_klass(class_loader_data(),
>> dim + 1, this, CHECK_NULL);
>> 144: ak->set_lower_dimension(this);
>
> Would it be useful to assert that the lower dimension has been set?
Yes, added.
> src/hotspot/share/oops/instanceKlass.cpp line 2765:
>
>> 2763: // To get a consistent list of classes we need MultiArray_lock to
>> ensure
>> 2764: // array classes aren't observed while they are being restored.
>> 2765: MutexLocker ml(MultiArray_lock);
>
> Doesn't this revert JDK-8274338?
You're right. I didn't believe my own comment when I removed this. Thank you
for pointing out the bug.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515346198
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515346716
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515349878
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v2]
On Thu, 7 Mar 2024 01:35:45 GMT, Coleen Phillimore wrote: >> This change creates a new sort of native recursive lock that can be held >> during JNI and Java calls, which can be used for synchronization while >> creating objArrayKlasses at runtime. >> >> Passes tier1-7. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Dean's comments. Thanks for your review Dean. I've retested tier1 with the changes and will send it for more. Thank you for finding the CDS bug in my change. - PR Review: https://git.openjdk.org/jdk/pull/17739#pullrequestreview-1921122133
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v2]
> This change creates a new sort of native recursive lock that can be held > during JNI and Java calls, which can be used for synchronization while > creating objArrayKlasses at runtime. > > Passes tier1-7. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Dean's comments. - Changes: - all: https://git.openjdk.org/jdk/pull/17739/files - new: https://git.openjdk.org/jdk/pull/17739/files/3625e6cc..d64aa560 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17739=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17739=00-01 Stats: 5 lines in 2 files changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17739.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17739/head:pull/17739 PR: https://git.openjdk.org/jdk/pull/17739
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Wed, 6 Mar 2024 23:09:34 GMT, Dean Long wrote:
>> This change creates a new sort of native recursive lock that can be held
>> during JNI and Java calls, which can be used for synchronization while
>> creating objArrayKlasses at runtime.
>>
>> Passes tier1-7.
>
> src/hotspot/share/runtime/mutex.cpp line 537:
>
>> 535: // can be called by jvmti by VMThread.
>> 536: if (current->is_Java_thread()) {
>> 537: _sem.wait_with_safepoint_check(JavaThread::cast(current));
>
> Why not use PlatformMutex + OSThreadWaitState instead of a semaphore?
Semaphore seemed simpler (?)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515294583
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Tue, 5 Mar 2024 23:13:30 GMT, Dean Long wrote: >> This change creates a new sort of native recursive lock that can be held >> during JNI and Java calls, which can be used for synchronization while >> creating objArrayKlasses at runtime. >> >> Passes tier1-7. > > Is the caller always a JavaThread? I'm wondering if your new recursive lock > class could use the existing ObjectMonitor. I thought David asked the same > question, but I can't find it. @dean-long An ObjectLocker on the mirror oop for InstanceKlass might work for this but as @dholmes-ora said we've been trying to purge the ObjectLocker code from the C++ code because it's complicated by the Java Object monitor project. The JOM project may throw exceptions from the ObjectLocker constructor or destructor and the C++ code doesn't currently know what to do. We removed the ObjectLockers around class linking and some JVMTI cases. There are some in class loading but with a small amount of work, they can be removed also. The caller is always a JavaThread, some lockers are at a safepoint for traversal but the multi-arrays are only created by JavaThreads. - PR Comment: https://git.openjdk.org/jdk/pull/17739#issuecomment-1980812019
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Tue, 13 Feb 2024 02:07:54 GMT, Coleen Phillimore wrote:
>> src/hotspot/share/oops/arrayKlass.cpp line 141:
>>
>>> 139: ObjArrayKlass::allocate_objArray_klass(class_loader_data(),
>>> dim + 1, this, CHECK_NULL);
>>> 140: // use 'release' to pair with lock-free load
>>> 141: release_set_higher_dimension(ak);
>>
>> Why has this code changed? I only expected to see the lock changed.
>
> The assert is dumb, leftover from when we didn't have C++ types (only
> klassOop). Of course it's an objArrayKlass, that's its type! The higher
> dimension should be set in the constructor of ObjArrayKlass. Every version
> of this change, I move this assignment there.
Changing this like this makes it more similar to the InstanceKlass::array_klass
function in structure, and there was unnecessary { } scopes in both and an
unnecessary ResourceMark.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1487058776
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Thu, 8 Feb 2024 21:30:48 GMT, David Holmes wrote:
>> This change creates a new sort of native recursive lock that can be held
>> during JNI and Java calls, which can be used for synchronization while
>> creating objArrayKlasses at runtime.
>>
>> Passes tier1-4.
>
> src/hotspot/share/classfile/classLoaderData.cpp line 412:
>
>> 410: // To call this, one must have the MultiArray_lock held, but the
>> _klasses list still has lock free reads.
>> 411: assert_locked_or_safepoint(MultiArray_lock);
>> 412:
>
> Do we no longer hold the lock or are you just missing the API to assert it is
> held?
We no longer hold the lock in the callers to iterate through CLD::_klasses
list. It was never needed. CLD::_klasses has iterators that are lock free
(atomics)
> src/hotspot/share/oops/arrayKlass.cpp line 141:
>
>> 139: ObjArrayKlass::allocate_objArray_klass(class_loader_data(),
>> dim + 1, this, CHECK_NULL);
>> 140: // use 'release' to pair with lock-free load
>> 141: release_set_higher_dimension(ak);
>
> Why has this code changed? I only expected to see the lock changed.
The assert is dumb, leftover from when we didn't have C++ types (only
klassOop). Of course it's an objArrayKlass, that's its type! The higher
dimension should be set in the constructor of ObjArrayKlass. Every version of
this change, I move this assignment there.
> src/hotspot/share/prims/jvmtiExport.cpp line 3151:
>
>> 3149: if (MultiArray_lock->owner() == thread) {
>> 3150: return false;
>> 3151: }
>
> So the recursive nature of the lock now means we don't have to bail out here
> - right?
Yes. If it were only the JNI upcall that was broken while holding the
MultiArray_lock, I think I could have used this same approach. Unfortunately,
its also throwing OOM for metaspace and for Java heap :(
> src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp line 107:
>
>> 105: // To get a consistent list of classes we need MultiArray_lock to
>> ensure
>> 106: // array classes aren't created.
>> 107: MutexLocker ma(MultiArray_lock);
>
> Unclear why this is no longer the case.
CLD::_klasses list is iterated through lock free with atomics. Older code used
to iterate through the system dictionary which only had InstanceKlasses and
then used to iterate through the arrays in InstanceKlass::_array_klasses.
Which required the MultiArray_lock protecting it. There used to be an
array_klasses_do() function. Now all the klasses are in the CLD::_klasses
list and traversed atomically.
> src/hotspot/share/runtime/recursiveLock.cpp line 45:
>
>> 43: _recursions++;
>> 44: assert(_recursions == 1, "should be");
>> 45: Atomic::release_store(&_owner, current); // necessary?
>
> No release necessary. The only thing written since the sem.wait is recursions
> and it is only updated or needed by the owning thread.
ok thanks.
> src/hotspot/share/runtime/recursiveLock.cpp line 54:
>
>> 52: _recursions--;
>> 53: if (_recursions == 0) {
>> 54: Atomic::release_store(&_owner, (Thread*)nullptr); // necessary?
>
> No release necessary.
ok, thanks.
> src/hotspot/share/runtime/recursiveLock.hpp line 33:
>
>> 31: // There are also no checks that the recursive lock is not held when
>> going to Java or to JNI, like
>> 32: // other JVM mutexes have. This should be used only for cases where the
>> alternatives with all the
>> 33: // nice safety features don't work.
>
> Mention that it does interact with safepoints properly for JavaThreads
Added a comment.
> src/hotspot/share/runtime/recursiveLock.hpp line 45:
>
>> 43: void unlock(Thread* current);
>> 44: };
>> 45:
>
> Should expose a `holdsLock` method to allow use in assertions.
Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1487056926
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1487058194
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1487060684
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1487062202
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1487063071
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1487063156
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1492431178
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1492430934
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Tue, 6 Feb 2024 22:59:04 GMT, Coleen Phillimore wrote: > This change creates a new sort of native recursive lock that can be held > during JNI and Java calls, which can be used for synchronization while > creating objArrayKlasses at runtime. > > Passes tier1-4. Thanks for looking at this WIP. - PR Review: https://git.openjdk.org/jdk/pull/17739#pullrequestreview-1876761135
RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
This change creates a new sort of native recursive lock that can be held during JNI and Java calls, which can be used for synchronization while creating objArrayKlasses at runtime. Passes tier1-4. - Commit messages: - 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock Changes: https://git.openjdk.org/jdk/pull/17739/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17739=00 Issue: https://bugs.openjdk.org/browse/JDK-8308745 Stats: 167 lines in 11 files changed: 87 ins; 47 del; 33 mod Patch: https://git.openjdk.org/jdk/pull/17739.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17739/head:pull/17739 PR: https://git.openjdk.org/jdk/pull/17739
Re: RFR: 8324799: Use correct extension for C++ test headers
On Wed, 28 Feb 2024 03:58:39 GMT, Guoxiong Li wrote: >> Please review this change that renames some test .h files to .hpp. These >> files contain C++ code and should be named accordingly. Some of them contain >> uses of NULL, which we change to nullptr. >> >> The renamed files are: >> >> test/hotspot/jtreg/vmTestbase/nsk/share/aod/aod.h >> test/hotspot/jtreg/vmTestbase/nsk/share/jni/jni_tools.h >> test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.h >> test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/JVMTITools.h >> test/hotspot/jtreg/vmTestbase/nsk/share/native/nsk_tools.h >> >> test/lib/jdk/test/lib/jvmti/jvmti_thread.h >> test/lib/jdk/test/lib/jvmti/jvmti_common.h >> test/lib/native/testlib_threads.h >> >> The #include updates were performed mostly mechanically, and builds would >> fail >> if there were mistakes. The exception is test >> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp, >> which was added after I'd done the mechanical update, so was updated by hand. >> >> The copyright updates were similarly performed mechanically. All but a >> handful >> of the including files have already had their copyrights updated recently, >> likely as part of JDK-8324681. >> >> Thus, the only "interesting" changes are to the renamed files. >> >> Testing: mach5 tier1 > > test/hotspot/jtreg/serviceability/jvmti/events/SingleStep/singlestep01/libsinglestep01.cpp > line 28: > >> 26: #include >> 27: #include >> 28: #include "jvmti_common.hpp" > > The copyright of this file is wrong. > >> * Copyright (c) 200 >> * git 3, 2022, Oracle and/or its affiliates. All rights reserved. >> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. It's weird that our jcheck tools didn't find the broken copyright. - PR Review Comment: https://git.openjdk.org/jdk/pull/18035#discussion_r1506027798
Re: RFR: 8324799: Use correct extension for C++ test headers
On Wed, 28 Feb 2024 01:18:50 GMT, Kim Barrett wrote: > Please review this change that renames some test .h files to .hpp. These > files contain C++ code and should be named accordingly. Some of them contain > uses of NULL, which we change to nullptr. > > The renamed files are: > > test/hotspot/jtreg/vmTestbase/nsk/share/aod/aod.h > test/hotspot/jtreg/vmTestbase/nsk/share/jni/jni_tools.h > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.h > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/JVMTITools.h > test/hotspot/jtreg/vmTestbase/nsk/share/native/nsk_tools.h > > test/lib/jdk/test/lib/jvmti/jvmti_thread.h > test/lib/jdk/test/lib/jvmti/jvmti_common.h > test/lib/native/testlib_threads.h > > The #include updates were performed mostly mechanically, and builds would fail > if there were mistakes. The exception is test > test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp, > which was added after I'd done the mechanical update, so was updated by hand. > > The copyright updates were similarly performed mechanically. All but a handful > of the including files have already had their copyrights updated recently, > likely as part of JDK-8324681. > > Thus, the only "interesting" changes are to the renamed files. > > Testing: mach5 tier1 I asked for these to be done together since it's the same scrolling for each of these. I added suggestions, but please feel free to ignore them since it would be prudent pull down and to rebuild after making them and it's not worth it. test/hotspot/jtreg/serviceability/jvmti/DynamicCodeGenerated/libDynamicCodeGenerated.cpp line 25: > 23: > 24: #include > 25: #include Is jvmti.h a C file? Sometimes it has <> sometimes it's included with "". I don't expect to see a change. I was just curious. test/hotspot/jtreg/serviceability/jvmti/vthread/FollowReferences/libVThreadStackRefTest.cpp line 26: > 24: #include > 25: #include > 26: #include Should this be in quotes rather than <> ? test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/libGetThreadStateMountedTest.cpp line 26: > 24: #include > 25: #include > 26: #include Another. test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassStatus/getclstat007/getclstat007.cpp line 28: > 26: #include "jvmti.h" > 27: #include "agent_common.hpp" > 28: #include "JVMTITools.hpp" There's a lower case jvmti_tools.hpp and an uppercase JVMTITools.hpp now. Maybe someone could do a cleanup of these tests (please!) test/hotspot/jtreg/vmTestbase/nsk/jvmti/RetransformClasses/retransform004/retransform004.cpp line 29: > 27: #include > 28: #include "agent_common.hpp" > 29: #include Suggestion: #include "jvmti_tools.hpp" - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18035#pullrequestreview-1906370538 PR Review Comment: https://git.openjdk.org/jdk/pull/18035#discussion_r1506026145 PR Review Comment: https://git.openjdk.org/jdk/pull/18035#discussion_r1506031049 PR Review Comment: https://git.openjdk.org/jdk/pull/18035#discussion_r1506031853 PR Review Comment: https://git.openjdk.org/jdk/pull/18035#discussion_r1506038573 PR Review Comment: https://git.openjdk.org/jdk/pull/18035#discussion_r1506045478
Re: RFR: 8324799: Use correct extension for C++ test headers
On Wed, 28 Feb 2024 14:13:58 GMT, Coleen Phillimore wrote: >> Please review this change that renames some test .h files to .hpp. These >> files contain C++ code and should be named accordingly. Some of them contain >> uses of NULL, which we change to nullptr. >> >> The renamed files are: >> >> test/hotspot/jtreg/vmTestbase/nsk/share/aod/aod.h >> test/hotspot/jtreg/vmTestbase/nsk/share/jni/jni_tools.h >> test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.h >> test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/JVMTITools.h >> test/hotspot/jtreg/vmTestbase/nsk/share/native/nsk_tools.h >> >> test/lib/jdk/test/lib/jvmti/jvmti_thread.h >> test/lib/jdk/test/lib/jvmti/jvmti_common.h >> test/lib/native/testlib_threads.h >> >> The #include updates were performed mostly mechanically, and builds would >> fail >> if there were mistakes. The exception is test >> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp, >> which was added after I'd done the mechanical update, so was updated by hand. >> >> The copyright updates were similarly performed mechanically. All but a >> handful >> of the including files have already had their copyrights updated recently, >> likely as part of JDK-8324681. >> >> Thus, the only "interesting" changes are to the renamed files. >> >> Testing: mach5 tier1 > > test/hotspot/jtreg/serviceability/jvmti/vthread/FollowReferences/libVThreadStackRefTest.cpp > line 26: > >> 24: #include >> 25: #include >> 26: #include > > Should this be in quotes rather than <> ? Suggestion: #include "jvmti_common.hpp" > test/hotspot/jtreg/serviceability/jvmti/vthread/GetThreadStateMountedTest/libGetThreadStateMountedTest.cpp > line 26: > >> 24: #include >> 25: #include >> 26: #include > > Another. Suggestion: #include "jvmti_common.hpp" - PR Review Comment: https://git.openjdk.org/jdk/pull/18035#discussion_r1506033122 PR Review Comment: https://git.openjdk.org/jdk/pull/18035#discussion_r1506033741
Re: RFR: 8326524: Rename agent_common.h
On Thu, 22 Feb 2024 19:38:26 GMT, Kim Barrett wrote: > Please review this trivial change that renames the file > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/agent_common/agent_common.h to > agent_common.hpp. > > The #include updates were performed mechanically, and builds would fail if > there were mistakes. > > The copyright updates were similarly performed mechanically. All but a handful > of the including files have already had their copyrights updated recently, > likely as part of JDK-8324681. The only change to the renamed file is a > copyright update, since no code changes were required. > > Testing: mach5 tier1 ok, I don't want to hold this up. - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17970#pullrequestreview-1900922282
Re: RFR: 8326524: Rename agent_common.h
On Fri, 23 Feb 2024 04:29:47 GMT, Kim Barrett wrote: >> test/hotspot/jtreg/vmTestbase/nsk/jvmti/Deallocate/dealloc001/dealloc001.cpp >> line 28: >> >>> 26: #include "jvmti.h" >>> 27: #include "agent_common.hpp" >>> 28: #include "JVMTITools.h" >> >> Why don't you change all of these together? It's the same or similar >> scrolling. > > As I said in our private chat, I'll look at doing that for at least some of > the remainder. I already had this one > queued up, tested, and ready to submit the PR when you made that suggestion. > But avoiding that scrolling > is why I described how those updates were done. If you believe the > (implicit) suggestion in the PR description, > you don't need to look at them at all, and can just find the renamed file in > the file list at the left in the review > window (so much less scrolling) and go look at it. Sure, but can you do the rest at the same time since they're in mostly the same files? - PR Review Comment: https://git.openjdk.org/jdk/pull/17970#discussion_r1502640501
Re: RFR: 8326524: Rename agent_common.h
On Thu, 22 Feb 2024 19:38:26 GMT, Kim Barrett wrote: > Please review this trivial change that renames the file > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/agent_common/agent_common.h to > agent_common.hpp. > > The #include updates were performed mechanically, and builds would fail if > there were mistakes. > > The copyright updates were similarly performed mechanically. All but a handful > of the including files have already had their copyrights updated recently, > likely as part of JDK-8324681. The only change to the renamed file is a > copyright update, since no code changes were required. > > Testing: mach5 tier1 I have a suggestion that'll make this worth scrolling just once. test/hotspot/jtreg/vmTestbase/nsk/jvmti/Deallocate/dealloc001/dealloc001.cpp line 28: > 26: #include "jvmti.h" > 27: #include "agent_common.hpp" > 28: #include "JVMTITools.h" Why don't you change all of these together? It's the same or similar scrolling. - PR Review: https://git.openjdk.org/jdk/pull/17970#pullrequestreview-1896852273 PR Review Comment: https://git.openjdk.org/jdk/pull/17970#discussion_r1499878024
Re: RFR: 8326090: Rename jvmti_aod.h
On Fri, 16 Feb 2024 21:42:18 GMT, Kim Barrett wrote: > Please review this trivial change that renames the file > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/aod/jvmti_aod.h to > jvmti_aod.hpp, > and replace uses of NULL in the file. > > The #include updates were performed mechanically, and build would fail if > there were mistakes. All of the including files have already had their > copyrights updated recently, as part of JDK-8324681. So the only interesting > (for some minimal value of "interesting") changes are in the renamed file. > > Testing: mach5 tier1 Looks good and trivial. - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17895#pullrequestreview-1890708971
Re: RFR: 8324680: Replace NULL with nullptr in JVMTI generated code
On Thu, 15 Feb 2024 08:46:43 GMT, Serguei Spitsyn wrote: > This enhancement replaces uses of NULL with nullptr in the XML-description > files for JVMTI. These are the files `hotsport/share/prims/jvmti.xml` and > `hotspot/share/prims/jvmti*.xls`. > > The following files are auto-generated from the `jvmti.xml` and `*.xsl files` > in the `build//variant-server/gensrc/jvmtifiles': `jvmti.h`, > `jvmti.html`, `jvmtiEnter.cpp`, `jvmtiEnterTrace.cpp`, `jvmtiEnv.hpp` > > This addresses a category of NULL uses that wasn't dealt with by: > [JDK-8299837](https://bugs.openjdk.org/browse/JDK-8299837). > > Testing: >- TBD: run mach5 tiers1-3 Looks good, thanks for fixing this. - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17866#pullrequestreview-1885068558
Re: RFR: 8252136: Several methods in hotspot are missing "static"
On Mon, 12 Feb 2024 12:43:09 GMT, Magnus Ihse Bursie wrote: > There are several places in hotspot where an internal function should have > been declared static, but isn't. > > These were discovered by trying to use the gcc option > `-Wmissing-declarations` and the corresponding clang option > `-Wmissing-prototypes`. These warnings check that a function either: > a) is declared static, or > b) has a declaration before its definition. > > The rationale of this is that functions are either internal to a compilation > unit, or exported to be linked by some other compilation unit. In the former > case, it should be marked static. In the latter case, it should be declared > in a header file, which should be included by the implementation as well. If > there is a discrepancy between the exported prototype and the implemented > prototype, this will be discovered during compilation (instead of as a > runtime error). Additionally, marking internal methods as static allows the > compiler to make better optimization, like inlining. > > This seem to be to be a sane assumption, and I think Hotspot (and the entire > JDK) would increase code quality by turning on these warnings. The absolute > majority of the code already adheres to these rules, but there are still some > places that needs to be fixed. > > This is the first part of addressing these issues, where all places that are > trivially missing static are fixed. > > I have discovered these by running with the warnings mentioned above turned > on. I have filtered out those places were an export was obviously missing. > The remaining warnings I have manually inspected. About 1/4 of them were > *_init() functions (which are directly declared in `init.cpp`) and another > 1/4 were documented as "use in debugger"; these I have not touched. I also > ignored functions with names suggesting it might be used in the debugger, > even if not documented as such, or any places that even seemed remotely > non-trivial. Finally I also reverted a few changes after it turned out that > gcc complained about unused functions. These places are actually dead code, > but it is not clear if they should be removed or if there is actually a call > missing somewhere (I believe it is a mix of both). In any case, I did not > want any such complexities in this PR. > > When the functions where marked static, gcc started complaining if they were > not used, since it consider it dead code. To address this, I had to add or > fix some `#ifdef`s. Since this code were not actually used unless these > criteria were fulfilled before either (it was just not discovered by the > compiler), I thi... Nice change. I looked at the runtime files, and surprised there weren't more. Thank you. - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17806#pullrequestreview-1875580015
Re: RFR: JDK-8311076: RedefineClasses doesn't check for ConstantPool overflow [v2]
On Fri, 9 Feb 2024 20:42:14 GMT, Alex Menkov wrote: >> The fix adds check that merged constant pool does not overflow u2 (two-byte >> unsigned). >> The check is added after merging `the_class` and `scratch_class` constant >> pools, but before rewriting constant pool references. >> >> testing: >> - sanity tier1; >> - all RedefineClasses/RetransformClasses tests: >>- test/jdk/java/lang/instrument >>- test/hotspot/jtreg/serviceability/jvmti/RedefineClasses >>- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses >>- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RetransformClasses > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > logging on cp overflow Yes, this should definitely be tagged noreg-hard. - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17759#pullrequestreview-1873283771
Re: RFR: JDK-8311076: RedefineClasses doesn't check for ConstantPool overflow
On Fri, 9 Feb 2024 02:34:26 GMT, Leonid Mesnik wrote:
>> The fix adds check that merged constant pool does not overflow u2 (two-byte
>> unsigned).
>> The check is added after merging `the_class` and `scratch_class` constant
>> pools, but before rewriting constant pool references.
>>
>> testing:
>> - sanity tier1;
>> - all RedefineClasses/RetransformClasses tests:
>>- test/jdk/java/lang/instrument
>>- test/hotspot/jtreg/serviceability/jvmti/RedefineClasses
>>- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses
>>- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RetransformClasses
>
> src/hotspot/share/prims/jvmtiRedefineClasses.cpp line 1828:
>
>> 1826: // ensure merged constant pool size does not overflow u2
>> 1827: if (merge_cp_length > 0x) {
>> 1828: return JVMTI_ERROR_INTERNAL;
>
> Doesn't it make a sense to add some logging there so user could easier
> understand the problem?
Seems like a good idea, you can use log_warning(redefine, class,
constantpool)("...")
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17759#discussion_r1483825615
Re: RFR: JDK-8311076: RedefineClasses doesn't check for ConstantPool overflow
On Wed, 7 Feb 2024 20:53:53 GMT, Alex Menkov wrote: > The fix adds check that merged constant pool does not overflow u2 (two-byte > unsigned). > The check is added after merging `the_class` and `scratch_class` constant > pools, but before rewriting constant pool references. > > testing: > - sanity tier1; > - all RedefineClasses/RetransformClasses tests: >- test/jdk/java/lang/instrument >- test/hotspot/jtreg/serviceability/jvmti/RedefineClasses >- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RedefineClasses >- test/hotspot/jtreg/vmTestbase/nsk/jvmti/RetransformClasses Thank you for fixing this. Looks good. - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17759#pullrequestreview-1871604187
Re: RFR: 8325367: Rename nsk_list.h
On Wed, 7 Feb 2024 20:48:18 GMT, Kim Barrett wrote: > Please review this trivial change that renames the file > test/hotspot/jtreg/vmTestbase/nsk/share/native/nsk_list.h to nsk_list.hpp. > > Testing: mach5 tier1 > Note that build would fail if #include updates were incorrect or incomplete. Yes, it's trivial. - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17758#pullrequestreview-1868882877
Re: RFR: 8325347: Rename native_thread.h
On Tue, 6 Feb 2024 22:08:18 GMT, Kim Barrett wrote: > Please review this trivial change that renames the file > test/hotspot/jtreg/vmTestbase/nsk/share/native/native_thread.h > to native_thread.hpp. and replaces uses of NULL in that file. > > Testing: mach5 tier1 Agree this looks trivial, if tests complete. edit: I think the test build would fail if one of these .h includes was missed. - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17737#pullrequestreview-1866446137
Withdrawn: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Wed, 31 Jan 2024 18:57:23 GMT, Coleen Phillimore wrote: > This change uses a claim token to allocate multi dimensional arrays rather > than holding MultiArray_lock around metaspace allocation. We can't hold a > mutex around metaspace allocation because it can create an OOM object and it > can also call into JVMTI for a resource exhausted event. Also, we were > creating mirrors and more metadata arrays while holding this lock. See the > bug for more details and other ideas considered and rejected. > > Tested with tier1-7. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/17660
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Thu, 1 Feb 2024 05:22:24 GMT, David Holmes wrote: >> This change uses a claim token to allocate multi dimensional arrays rather >> than holding MultiArray_lock around metaspace allocation. We can't hold a >> mutex around metaspace allocation because it can create an OOM object and it >> can also call into JVMTI for a resource exhausted event. Also, we were >> creating mirrors and more metadata arrays while holding this lock. See the >> bug for more details and other ideas considered and rejected. >> >> Tested with tier1-7. > > ~~Can't you just use a Monitor to implement the claim token, rather than this > lock-free approach? (Similar to how class initialization is handled.)~~ > > Sorry lost the forest in the trees. I'm going to do a bit of rework as discussed with @dholmes-ora . - PR Comment: https://git.openjdk.org/jdk/pull/17660#issuecomment-1929599218
RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
This change uses a claim token to allocate multi dimensional arrays rather than holding MultiArray_lock around metaspace allocation. We can't hold a mutex around metaspace allocation because it can create an OOM object and it can also call into JVMTI for a resource exhausted event. Also, we were creating mirrors and more metadata arrays while holding this lock. See the bug for more details and other ideas considered and rejected. Tested with tier1-7. - Commit messages: - Some more cleanups, and make token really recursive. - 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock Changes: https://git.openjdk.org/jdk/pull/17660/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17660=00 Issue: https://bugs.openjdk.org/browse/JDK-8308745 Stats: 156 lines in 10 files changed: 90 ins; 17 del; 49 mod Patch: https://git.openjdk.org/jdk/pull/17660.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17660/head:pull/17660 PR: https://git.openjdk.org/jdk/pull/17660
Integrated: 8324681: Replace NULL with nullptr in HotSpot jtreg test native code files
On Fri, 26 Jan 2024 16:40:32 GMT, Coleen Phillimore wrote: > This mechanically replaces NULL with nullptr in hpp/cpp native files in test > native code. This didn't attempt to change NULL in comments to say null > because nullptr is generally the right thing for the comment to say. It does > attempt to change NULL to "null" rather than "nullptr" in strings. Any > changes for "nullptr" to "null" in comments can be changed in a future RFE in > a smaller patch. I didn't see any when it was scrolling by to make my script > more complicated. > > Ran tier1-4 testing. This pull request has now been integrated. Changeset: a6bdee48 Author:Coleen Phillimore URL: https://git.openjdk.org/jdk/commit/a6bdee48f39993128d8095d40ab417f0102af0f4 Stats: 8218 lines in 750 files changed: 0 ins; 7 del; 8211 mod 8324681: Replace NULL with nullptr in HotSpot jtreg test native code files Reviewed-by: kevinw, kbarrett, dholmes - PR: https://git.openjdk.org/jdk/pull/17593
Re: RFR: 8324681: Replace NULL with nullptr in HotSpot jtreg test native code files [v5]
On Mon, 29 Jan 2024 13:47:10 GMT, Coleen Phillimore wrote: >> This mechanically replaces NULL with nullptr in hpp/cpp native files in test >> native code. This didn't attempt to change NULL in comments to say null >> because nullptr is generally the right thing for the comment to say. It >> does attempt to change NULL to "null" rather than "nullptr" in strings. Any >> changes for "nullptr" to "null" in comments can be changed in a future RFE >> in a smaller patch. I didn't see any when it was scrolling by to make my >> script more complicated. >> >> Ran tier1-4 testing. > > Coleen Phillimore has updated the pull request incrementally with two > additional commits since the last revision: > > - Fix some casts unnecessary with nullptr > - Fix copyrights macos-aarch64 build failure in GHA appears unrelated, internal testing passed. - PR Comment: https://git.openjdk.org/jdk/pull/17593#issuecomment-1915186403
Re: RFR: 8324681: Replace NULL with nullptr in HotSpot jtreg test native code files [v5]
On Mon, 29 Jan 2024 13:47:10 GMT, Coleen Phillimore wrote: >> This mechanically replaces NULL with nullptr in hpp/cpp native files in test >> native code. This didn't attempt to change NULL in comments to say null >> because nullptr is generally the right thing for the comment to say. It >> does attempt to change NULL to "null" rather than "nullptr" in strings. Any >> changes for "nullptr" to "null" in comments can be changed in a future RFE >> in a smaller patch. I didn't see any when it was scrolling by to make my >> script more complicated. >> >> Ran tier1-4 testing. > > Coleen Phillimore has updated the pull request incrementally with two > additional commits since the last revision: > > - Fix some casts unnecessary with nullptr > - Fix copyrights Thanks Kevin, Kim and David for wading through this. If there are other changes we can address them separately preserving your eyeballs. My copyright script was broken so I fixed it. I'll wait for GHA to make sure I didn't break anything before integrating. - PR Comment: https://git.openjdk.org/jdk/pull/17593#issuecomment-1914798074
Re: RFR: 8324681: Replace NULL with nullptr in HotSpot jtreg test native code files [v5]
> This mechanically replaces NULL with nullptr in hpp/cpp native files in test > native code. This didn't attempt to change NULL in comments to say null > because nullptr is generally the right thing for the comment to say. It does > attempt to change NULL to "null" rather than "nullptr" in strings. Any > changes for "nullptr" to "null" in comments can be changed in a future RFE in > a smaller patch. I didn't see any when it was scrolling by to make my script > more complicated. > > Ran tier1-4 testing. Coleen Phillimore has updated the pull request incrementally with two additional commits since the last revision: - Fix some casts unnecessary with nullptr - Fix copyrights - Changes: - all: https://git.openjdk.org/jdk/pull/17593/files - new: https://git.openjdk.org/jdk/pull/17593/files/6eb051ed..6ac8aa85 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17593=04 - incr: https://webrevs.openjdk.org/?repo=jdk=17593=03-04 Stats: 32 lines in 27 files changed: 0 ins; 0 del; 32 mod Patch: https://git.openjdk.org/jdk/pull/17593.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17593/head:pull/17593 PR: https://git.openjdk.org/jdk/pull/17593
Re: RFR: 8324681: Replace NULL with nullptr in HotSpot jtreg test native code files [v4]
> This mechanically replaces NULL with nullptr in hpp/cpp native files in test > native code. This didn't attempt to change NULL in comments to say null > because nullptr is generally the right thing for the comment to say. It does > attempt to change NULL to "null" rather than "nullptr" in strings. Any > changes for "nullptr" to "null" in comments can be changed in a future RFE in > a smaller patch. I didn't see any when it was scrolling by to make my script > more complicated. > > Ran tier1-4 testing. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Fix one nullptr in comments as found by @kevinw - Changes: - all: https://git.openjdk.org/jdk/pull/17593/files - new: https://git.openjdk.org/jdk/pull/17593/files/33786c7d..6eb051ed Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17593=03 - incr: https://webrevs.openjdk.org/?repo=jdk=17593=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17593.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17593/head:pull/17593 PR: https://git.openjdk.org/jdk/pull/17593
Re: RFR: 8324681: Replace NULL with nullptr in HotSpot jtreg test native code files [v3]
> This mechanically replaces NULL with nullptr in hpp/cpp native files in test > native code. This didn't attempt to change NULL in comments or strings to > just null. If you run into this and it bothers you after this push, you can > change it in a smaller patch. I didn't see any when it was scrolling by to > make my script more complicated. > > Ran tier1-4 testing. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Fix nullptr only contained in strings. - Changes: - all: https://git.openjdk.org/jdk/pull/17593/files - new: https://git.openjdk.org/jdk/pull/17593/files/e15a3a0b..33786c7d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17593=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17593=01-02 Stats: 19 lines in 3 files changed: 0 ins; 0 del; 19 mod Patch: https://git.openjdk.org/jdk/pull/17593.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17593/head:pull/17593 PR: https://git.openjdk.org/jdk/pull/17593
Re: RFR: 8324681: Replace NULL with nullptr in HotSpot jtreg test native code files [v2]
> This mechanically replaces NULL with nullptr in hpp/cpp native files in test > native code. This didn't attempt to change NULL in comments or strings to > just null. If you run into this and it bothers you after this push, you can > change it in a smaller patch. I didn't see any when it was scrolling by to > make my script more complicated. > > Ran tier1-4 testing. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Fix the comments to "null" - Changes: - all: https://git.openjdk.org/jdk/pull/17593/files - new: https://git.openjdk.org/jdk/pull/17593/files/079b8931..e15a3a0b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17593=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17593=00-01 Stats: 229 lines in 103 files changed: 0 ins; 0 del; 229 mod Patch: https://git.openjdk.org/jdk/pull/17593.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17593/head:pull/17593 PR: https://git.openjdk.org/jdk/pull/17593
RFR: 8324681: Replace NULL with nullptr in HotSpot jtreg test native code files
This mechanically replaces NULL with nullptr in hpp/cpp native files in test native code. This didn't attempt to change NULL in comments or strings to just null. If you run into this and it bothers you after this push, you can change it in a smaller patch. I didn't see any when it was scrolling by to make my script more complicated. Ran tier1-4 testing. - Commit messages: - 8324681: Replace NULL with nullptr in HotSpot jtreg test native code files Changes: https://git.openjdk.org/jdk/pull/17593/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17593=00 Issue: https://bugs.openjdk.org/browse/JDK-8324681 Stats: 8196 lines in 750 files changed: 0 ins; 7 del; 8189 mod Patch: https://git.openjdk.org/jdk/pull/17593.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17593/head:pull/17593 PR: https://git.openjdk.org/jdk/pull/17593
Re: RFR: 8324241: Always record evol_method deps to avoid excessive method flushing [v2]
On Mon, 22 Jan 2024 23:37:35 GMT, John R Rose wrote: >> src/hotspot/share/runtime/globals.hpp line 2013: >> >>> 2011: "Profile exception handlers") >>> \ >>> 2012: >>> \ >>> 2013: product(bool, AlwaysRecordEvolDependencies, true, DIAGNOSTIC, >>> \ >> >> As we record all dependencies not only evol_method ones, should we name it >> just: `AlwaysRecordDependencies`? > > That’s not exactly right either. `RecordAllDependencies` would be more like > it. Because: > > - We might record some dependencies that we know we need, and leave others > out. > - Or, we might record all dependencies, even ones we think we might not > need. > > (But we will need them all if somebody turns on JFR.) > > I like this change. Having a diagnostic switch means we can do a rough > triage if something seems to go wrong with this change, down the road. No, because you want to turn on/off evol_method independently of the other dependencies that the compiler is recording. - PR Review Comment: https://git.openjdk.org/jdk/pull/17509#discussion_r1462556836
Re: RFR: 8324241: Always record evol_method deps to avoid excessive method flushing [v2]
On Mon, 22 Jan 2024 21:26:41 GMT, Volker Simonis wrote:
>> Currently we don't record dependencies on redefined methods (i.e.
>> `evol_method` dependencies) in JIT compiled methods if none of the
>> `can_redefine_classes`, `can_retransform_classes` or
>> `can_generate_breakpoint_events` JVMTI capabalities is set. This means that
>> if a JVMTI agent which requests one of these capabilities is dynamically
>> attached, all the methods which have been JIT compiled until that point,
>> will be marked for deoptimization and flushed from the code cache. For
>> large, warmed-up applications this mean deoptimization and instant
>> recompilation of thousands if not then-thousands of methods, which can lead
>> to dramatic performance/latency drop-downs for several minutes.
>>
>> One could argue that dynamic agent attach is now deprecated anyway (see [JEP
>> 451: Prepare to Disallow the Dynamic Loading of
>> Agents](https://openjdk.org/jeps/451)) and this problem could be solved by
>> making the recording of `evol_method` dependencies dependent on the new
>> `-XX:+EnableDynamicAgentLoading` flag isntead of the concrete JVMTI
>> capabilities (because the presence of the flag indicates that an agent will
>> be loaded eventually).
>>
>> But there a single, however important exception to this rule and that's JFR.
>> JFR is advertised as low overhead profiler which can be enabled in
>> production at any time. However, when JFR is started dynamically (e.g.
>> through JCMD or JMX) it will silently load a HotSpot internl JVMTI agent
>> which requests the `can_retransform_classes` and retransforms some classes.
>> This will inevitably trigger the deoptimization of all compiled methods as
>> described above.
>>
>> I'd therefor like to propose to *always* and unconditionally record
>> `evol_method` dependencies in JIT compiled code by exporting the relevant
>> properties right at startup in `init_globals()`:
>> ```c++
>> jint init_globals() {
>>management_init();
>>JvmtiExport::initialize_oop_storage();
>> +#if INCLUDE_JVMTI
>> + JvmtiExport::set_can_hotswap_or_post_breakpoint(true);
>> + JvmtiExport::set_all_dependencies_are_recorded(true);
>> +#endif
>>
>>
>> My measurements indicate that the overhead of doing so is minimal (around 1%
>> increase of nmethod size) and justifies the benefit. E.g. a Spring Petclinic
>> application started with `-Xbatch -XX:+UnlockDiagnosticVMOptions
>> -XX:+LogCompilation` compiles about ~11500 methods (~9000 with C1 and ~2500
>> with C2) resulting in an aggregated nmethod size of around ~40bm.
>> Additionally recording `evol_method` dependencies only increases this size
>> be about 400kb
>
> Volker Simonis has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Guard the feature with a diagnostic option and update the comments in the
> code
This looks good.
-
Marked as reviewed by coleenp (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17509#pullrequestreview-1837600406
Re: RFR: 8324241: Always record evol_method deps to avoid excessive method flushing
On Sat, 20 Jan 2024 19:48:07 GMT, Volker Simonis wrote:
> Currently we don't record dependencies on redefined methods (i.e.
> `evol_method` dependencies) in JIT compiled methods if none of the
> `can_redefine_classes`, `can_retransform_classes` or
> `can_generate_breakpoint_events` JVMTI capabalities is set. This means that
> if a JVMTI agent which requests one of these capabilities is dynamically
> attached, all the methods which have been JIT compiled until that point, will
> be marked for deoptimization and flushed from the code cache. For large,
> warmed-up applications this mean deoptimization and instant recompilation of
> thousands if not then-thousands of methods, which can lead to dramatic
> performance/latency drop-downs for several minutes.
>
> One could argue that dynamic agent attach is now deprecated anyway (see [JEP
> 451: Prepare to Disallow the Dynamic Loading of
> Agents](https://openjdk.org/jeps/451)) and this problem could be solved by
> making the recording of `evol_method` dependencies dependent on the new
> `-XX:+EnableDynamicAgentLoading` flag isntead of the concrete JVMTI
> capabilities (because the presence of the flag indicates that an agent will
> be loaded eventually).
>
> But there a single, however important exception to this rule and that's JFR.
> JFR is advertised as low overhead profiler which can be enabled in production
> at any time. However, when JFR is started dynamically (e.g. through JCMD or
> JMX) it will silently load a HotSpot internl JVMTI agent which requests the
> `can_retransform_classes` and retransforms some classes. This will inevitably
> trigger the deoptimization of all compiled methods as described above.
>
> I'd therefor like to propose to *always* and unconditionally record
> `evol_method` dependencies in JIT compiled code by exporting the relevant
> properties right at startup in `init_globals()`:
> ```c++
> jint init_globals() {
>management_init();
>JvmtiExport::initialize_oop_storage();
> +#if INCLUDE_JVMTI
> + JvmtiExport::set_can_hotswap_or_post_breakpoint(true);
> + JvmtiExport::set_all_dependencies_are_recorded(true);
> +#endif
>
>
> My measurements indicate that the overhead of doing so is minimal (around 1%
> increase of nmethod size) and justifies the benefit. E.g. a Spring Petclinic
> application started with `-Xbatch -XX:+UnlockDiagnosticVMOptions
> -XX:+LogCompilation` compiles about ~11500 methods (~9000 with C1 and ~2500
> with C2) resulting in an aggregated nmethod size of around ~40bm.
> Additionally recording `evol_method` dependencies only increases this size be
> about 400kb. The ration remains about the same i...
I support adding this as a diagnostic option, therefore you won't have to make
the change in VM_RedefineClasses::flush_dependent_code.
-
PR Comment: https://git.openjdk.org/jdk/pull/17509#issuecomment-1904784430
Re: RFR: 8311846: Resolve duplicate 'Thread' related symbols with JDK static linking
On Wed, 17 Jan 2024 00:14:58 GMT, Jiangli Zhou wrote: > Please review this PR with a simple solution for resolving duplicate `Thread` > symbol issue. In https://github.com/openjdk/jdk/pull/14808 comments, there > was an alternative suggestion to redefine the symbol at build time, such as > using`-DThread=HotSpotThread`. That would not address issues when symbol were > references as string literals. https://github.com/openjdk/jdk/pull/14808 also > discussed using namespace for hotspot code, which can have multiple > benefits/motivations. We could explore further using namespace with more > consensus on that approach. > > Contributed by Chuck Rasbold and @jianglizhou. You could support one build by adding something like -DSUPPORTS_STATIC_LINK for both .so and .a builds for Google, then use that to protect the renaming. I don't know how bad "namespace hotspot" would be for debugging. At least for some of the common names. I suppose breakpoints would have to be specified in gdb as break at hotspot::Thread::is_owning_thread or something like that, and with a using namespace hotspot, it wouldn't be visible looking at the source code in that form. - PR Comment: https://git.openjdk.org/jdk/pull/17456#issuecomment-1900475932
Re: RFR: 8311846: Resolve duplicate 'Thread' related symbols with JDK static linking
On Wed, 17 Jan 2024 00:14:58 GMT, Jiangli Zhou wrote:
> Please review this PR with a simple solution for resolving duplicate `Thread`
> symbol issue. In https://github.com/openjdk/jdk/pull/14808 comments, there
> was an alternative suggestion to redefine the symbol at build time, such as
> using`-DThread=HotSpotThread`. That would not address issues when symbol were
> references as string literals. https://github.com/openjdk/jdk/pull/14808 also
> discussed using namespace for hotspot code, which can have multiple
> benefits/motivations. We could explore further using namespace with more
> consensus on that approach.
>
> Contributed by Chuck Rasbold and @jianglizhou.
I was reading through the other PR for StringTable and was wonder how difficult
it would be to wrap all of hotspot in namespace hotspot {}; using namespace
hotspot; It would need a JEP as discussed in the other PR.
Alternatively if there's a #ifdef you can use for renaming the Thread to
HotspotThread for static linking only, it might make this change less worrysome.
-
PR Comment: https://git.openjdk.org/jdk/pull/17456#issuecomment-1897336087
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v11]
On Mon, 4 Dec 2023 11:11:27 GMT, Thomas Stuefe wrote: > Okay so why does this happen and is it a reasonable thing to be happening? On > the surface it sounds wrong to deallocate anything associated with a live > classloader. If we didn't deallocate these old methods, there would be a memory leak when using class redefinition. It would be a lot simpler if we didn't have to do this though. - PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1838543829
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v11]
On Wed, 29 Nov 2023 11:49:31 GMT, Jaroslav Bachorik wrote: >> Please, review this fix for a corner case handling of `jmethodID` values. >> >> The issue is related to the interplay between `jmethodID` values and method >> redefinitions. Each `jmethodID` value is effectively a pointer to a `Method` >> instance. Once that method gets redefined, the `jmethodID` is updated to >> point to the last `Method` version. >> Unless the method is still on stack/running, in which case the original >> `jmethodID` will be redirected to the latest `Method` version and at the >> same time the 'previous' `Method` version will receive a new `jmethodID` >> pointing to that previous version. >> >> If we happen to capture stacktrace via `GetStackTrace` or >> `GetAllStackTraces` JVMTI calls while this previous `Method` version is >> still on stack we will have the corresponding frame identified by a >> `jmethodID` pointing to that version. >> However, sooner or later the 'previous' class version becomes eligible for >> cleanup at what time all contained `Method` instances. The cleanup process >> will not perform the `jmethodID` pointer maintenance and we will end up with >> pointers to deallocated memory. >> This is caused by the fact that the `jmethodID` lifecycle is bound to >> `ClassLoaderData` instance and all relevant `jmethodID`s will get >> batch-updated when the class loader is being released and all its classes >> are getting unloaded. >> >> This means that we need to make sure that if a `Method` instance is being >> deallocate the associated `jmethodID` (if any) must not point to the >> deallocated instance once we are finished. Unfortunately, we can not just >> update the `jmethodID` values in bulk when purging an old class version - >> the per `InstanceKlass` jmethodID cache is present only for the main class >> version and contains `jmethodID` values for both the old and current method >> versions. >> >> ~Therefore we need to perform `jmethodID` lookup when we are about to >> deallocate a `Method` instance and clean up the pointer only if that >> `jmethodID` is pointing to the `Method` instance which is being deallocated.~ >> >> Therefore, we need to perform `jmethodID` lookup for each method in an old >> class version that is getting purged, and null out the pointer of that >> `jmethodID` to break the link from `jmethodID` to the method instance that >> is about to get deallocated. >> >> _(For anyone interested, a much lengthier writeup is available in [my >> blog](https://jbachorik.github.io/posts/mysterious-jmethodid))_ > > Jaroslav Bachorik has updated the pull request incrementally with one > additional commit since the last revision: > > Restrict cleanup to obsolete methods only We deallocate the old Method* if nothing is referring to them (ie they're not running or being referenced for some other reason). Look at MetadataOnStackMark. The jmethodIDs to an obsolete method were a dangling pointer and we want to just null them out. The old Method* are attached to the scratch_version of the InstanceKlass so we essentially remove that and walk down to the Methods when none of them are found. - PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1838538568
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v11]
On Thu, 30 Nov 2023 06:06:48 GMT, David Holmes wrote: >> Thanks everyone involved in reviewing this PR! You were awesome and helped >> me drive the PR to better place than it started! > > @jbachorik this should not have been integrated yet! You only have one > review not the required two for hotspot changes. Further your one Reviewer > didn't even review the final version of the change! I did review the final version of the change. Can @dholmes-ora or @tstuefe review again and we'll open CRs for anything missed? - PR Comment: https://git.openjdk.org/jdk/pull/16662#issuecomment-1833758619
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v10]
On Wed, 29 Nov 2023 15:14:59 GMT, Jaroslav Bachorik wrote: >> I am going to take a look. > > Ok, I found it. > The reason for the jmethodID not being cleaned out is this assignment of a > new jmethodID to obsolete methods - > https://github.com/openjdk/jdk/blob/a2c5f1fc914ef5c28d044b75598f895cf6097138/src/hotspot/share/prims/jvmtiRedefineClasses.cpp#L3887 > > Since this is not done for EMCP (or more generic, non-obsolete) methods > restricting the cleanup process to obsolete methods still sounds like the > right thing to do. Excellent thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/16662#discussion_r1409626840
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v10]
On Wed, 29 Nov 2023 11:45:53 GMT, Jaroslav Bachorik
wrote:
>> src/hotspot/share/oops/instanceKlass.cpp line 4236:
>>
>>> 4234: if (method != nullptr) {
>>> 4235: method->clear_jmethod_id();
>>> 4236: }
>>
>> This loops through the methods in the InstanceKlass that was a previous
>> version klass, and clears the jmethodIDs for all the methods. Will it clear
>> the jmethodIDs for the EMCP methods also, and should it?
>> The jmethodID for EMCP methods are replaced with a the new version, so the
>> Method* in this list won't find a matching jmethodID. Maybe this can be
>> restricted to obsolete methods?
>
> Restricting to obsolete methods sounds like a good idea.
Can you confirm my observation above, that EMCP jmethodIDs are replaced? I
haven't looked at this code in a while. Thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16662#discussion_r1409283757
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes [v10]
On Mon, 27 Nov 2023 09:16:30 GMT, Jaroslav Bachorik
wrote:
>> Please, review this fix for a corner case handling of `jmethodID` values.
>>
>> The issue is related to the interplay between `jmethodID` values and method
>> redefinitions. Each `jmethodID` value is effectively a pointer to a `Method`
>> instance. Once that method gets redefined, the `jmethodID` is updated to
>> point to the last `Method` version.
>> Unless the method is still on stack/running, in which case the original
>> `jmethodID` will be redirected to the latest `Method` version and at the
>> same time the 'previous' `Method` version will receive a new `jmethodID`
>> pointing to that previous version.
>>
>> If we happen to capture stacktrace via `GetStackTrace` or
>> `GetAllStackTraces` JVMTI calls while this previous `Method` version is
>> still on stack we will have the corresponding frame identified by a
>> `jmethodID` pointing to that version.
>> However, sooner or later the 'previous' class version becomes eligible for
>> cleanup at what time all contained `Method` instances. The cleanup process
>> will not perform the `jmethodID` pointer maintenance and we will end up with
>> pointers to deallocated memory.
>> This is caused by the fact that the `jmethodID` lifecycle is bound to
>> `ClassLoaderData` instance and all relevant `jmethodID`s will get
>> batch-updated when the class loader is being released and all its classes
>> are getting unloaded.
>>
>> This means that we need to make sure that if a `Method` instance is being
>> deallocate the associated `jmethodID` (if any) must not point to the
>> deallocated instance once we are finished. Unfortunately, we can not just
>> update the `jmethodID` values in bulk when purging an old class version -
>> the per `InstanceKlass` jmethodID cache is present only for the main class
>> version and contains `jmethodID` values for both the old and current method
>> versions.
>>
>> ~Therefore we need to perform `jmethodID` lookup when we are about to
>> deallocate a `Method` instance and clean up the pointer only if that
>> `jmethodID` is pointing to the `Method` instance which is being deallocated.~
>>
>> Therefore, we need to perform `jmethodID` lookup for each method in an old
>> class version that is getting purged, and null out the pointer of that
>> `jmethodID` to break the link from `jmethodID` to the method instance that
>> is about to get deallocated.
>>
>> _(For anyone interested, a much lengthier writeup is available in [my
>> blog](https://jbachorik.github.io/posts/mysterious-jmethodid))_
>
> Jaroslav Bachorik has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Remove unnecessary assert
This looks really good. Thank you for the blog post, it helped understand the
problem, which is very convoluted. I like that the methodID is cleared with
purge_previous_version_list.
src/hotspot/share/oops/instanceKlass.cpp line 4236:
> 4234: if (method != nullptr) {
> 4235: method->clear_jmethod_id();
> 4236: }
This loops through the methods in the InstanceKlass that was a previous version
klass, and clears the jmethodIDs for all the methods. Will it clear the
jmethodIDs for the EMCP methods also, and should it?
The jmethodID for EMCP methods are replaced with a the new version, so the
Method* in this list won't find a matching jmethodID. Maybe this can be
restricted to obsolete methods?
-
Marked as reviewed by coleenp (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16662#pullrequestreview-1753984104
PR Review Comment: https://git.openjdk.org/jdk/pull/16662#discussion_r1408443056
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes
On Thu, 16 Nov 2023 03:11:04 GMT, Jaroslav Bachorik wrote: >> src/hotspot/share/oops/method.cpp line 2277: >> >>> 2275: } >>> 2276: } >>> 2277: >> >> Can this race with redefinition? > > The cleanup of previous versions is executed in VM_Operation at a safepoint - > therefore we should be safe against races with class redefinitions. > I am adding an assert to `clear_jmethod_id()` to check for being at a > safepoint. Yes, these are cleaned at a safepoint. - PR Review Comment: https://git.openjdk.org/jdk/pull/16662#discussion_r1399798259
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes
On Sat, 18 Nov 2023 00:23:44 GMT, Jaroslav Bachorik wrote: >> src/hotspot/share/oops/instanceKlass.cpp line 541: >> >>> 539: // The previous version will point to them so they're not >>> totally dangling >>> 540: assert (!method->on_stack(), "shouldn't be called with methods >>> on stack"); >>> 541: // Do the pointer maintenance before releasing the metadata, but >>> not for incomplete methods >> >> I'm confused by what you mean by method holder, which I think of as >> methodHandle. Or InstanceKlass is the holder of the methods. Maybe this >> should be more explicit that it's talking about clearing any associated >> jmethodIDs. > > The method holder is an `InstanceKlass` object which can be retrieved as > `method->method_holder()` (I apologize if I am using not completely correct > terms - this is what I grokked from the sources). And incomplete methods > created by the `ClassParser` from the class data stream will not have the > link to that `InstanceKlass` set up if the `ClassParser` is already having > its `_klass` field set to a non-null value. > > If we are talking about clearing any jmetbodIDs associated with an > `InstanceKlass` instance it is not really possible for old method versions > because only the current `InstanceKlass` version has the jmethodID cache > associated with it and it contains jmethodIDs pointing to bot the old and > current methods. I see, holder is the right word and concept. So the parameter means has_method_holder, in that the InstanceKlass has been fully parsed at the point of clearing the jmethodIDs. - PR Review Comment: https://git.openjdk.org/jdk/pull/16662#discussion_r1399785853
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes
On Tue, 14 Nov 2023 17:56:09 GMT, Jaroslav Bachorik
wrote:
> Please, review this fix for a corner case handling of `jmethodID` values.
>
> The issue is related to the interplay between `jmethodID` values and method
> redefinitions. Each `jmethodID` value is effectively a pointer to a `Method`
> instance. Once that method gets redefined, the `jmethodID` is updated to
> point to the last `Method` version.
> Unless the method is still on stack/running, in which case the original
> `jmethodID` will be redirected to the latest `Method` version and at the same
> time the 'previous' `Method` version will receive a new `jmethodID` pointing
> to that previous version.
>
> If we happen to capture stacktrace via `GetStackTrace` or `GetAllStackTraces`
> JVMTI calls while this previous `Method` version is still on stack we will
> have the corresponding frame identified by a `jmethodID` pointing to that
> version.
> However, sooner or later the 'previous' class version becomes eligible for
> cleanup at what time all contained `Method` instances. The cleanup process
> will not perform the `jmethodID` pointer maintenance and we will end up with
> pointers to deallocated memory.
> This is caused by the fact that the `jmethodID` lifecycle is bound to
> `ClassLoaderData` instance and all relevant `jmethodID`s will get
> batch-updated when the class loader is being released and all its classes are
> getting unloaded.
>
> This means that we need to make sure that if a `Method` instance is being
> deallocate the associated `jmethodID` (if any) must not point to the
> deallocated instance once we are finished. Unfortunately, we can not just
> update the `jmethodID` values in bulk when purging an old class version - the
> per `InstanceKlass` jmethodID cache is present only for the main class
> version and contains `jmethodID` values for both the old and current method
> versions.
>
> Therefore we need to perform `jmethodID` lookup when we are about to
> deallocate a `Method` instance and clean up the pointer only if that
> `jmethodID` is pointing to the `Method` instance which is being deallocated.
>
> _(For anyone interested, a much lengthier writeup is available in [my
> blog](https://jbachorik.github.io/posts/mysterious-jmethodid))_
Good analysis for a very subtle bug. I have a couple of comments, and maybe
the test can be simplified but approving the change.
src/hotspot/share/classfile/classFileParser.cpp line 5579:
> 5577:
> 5578: if (_methods != nullptr) {
> 5579: // Free methods - those methods are not fully wired and miss the
> method holder
How about saying: for methods whose InstanceKlass as method holder is not yet
created?
test/hotspot/jtreg/serviceability/jvmti/thread/GetStackTrace/GetStackTraceAndRetransformTest/GetStackTraceAndRetransformTest.java
line 53:
> 51: import java.util.List;
> 52: import java.util.concurrent.CyclicBarrier;
> 53: import java.util.concurrent.locks.LockSupport;
Do you need all these imports?
There's a simple RedefineClassHelper class that does most of the work, but
maybe you need the explicit agent code to reproduce the crash? See
test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineRunningMethodsWithBacktrace.java
as an example.
-
Marked as reviewed by coleenp (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/16662#pullrequestreview-1740746213
PR Review Comment: https://git.openjdk.org/jdk/pull/16662#discussion_r1399811821
PR Review Comment: https://git.openjdk.org/jdk/pull/16662#discussion_r1399810008
Re: RFR: 8313816: Accessing jmethodID might lead to spurious crashes
On Tue, 14 Nov 2023 17:56:09 GMT, Jaroslav Bachorik wrote: > Please, review this fix for a corner case handling of `jmethodID` values. > > The issue is related to the interplay between `jmethodID` values and method > redefinitions. Each `jmethodID` value is effectively a pointer to a `Method` > instance. Once that method gets redefined, the `jmethodID` is updated to > point to the last `Method` version. > Unless the method is still on stack/running, in which case the original > `jmethodID` will be redirected to the latest `Method` version and at the same > time the 'previous' `Method` version will receive a new `jmethodID` pointing > to that previous version. > > If we happen to capture stacktrace via `GetStackTrace` or `GetAllStackTraces` > JVMTI calls while this previous `Method` version is still on stack we will > have the corresponding frame identified by a `jmethodID` pointing to that > version. > However, sooner or later the 'previous' class version becomes eligible for > cleanup at what time all contained `Method` instances. The cleanup process > will not perform the `jmethodID` pointer maintenance and we will end up with > pointers to deallocated memory. > This is caused by the fact that the `jmethodID` lifecycle is bound to > `ClassLoaderData` instance and all relevant `jmethodID`s will get > batch-updated when the class loader is being released and all its classes are > getting unloaded. > > This means that we need to make sure that if a `Method` instance is being > deallocate the associated `jmethodID` (if any) must not point to the > deallocated instance once we are finished. Unfortunately, we can not just > update the `jmethodID` values in bulk when purging an old class version - the > per `InstanceKlass` jmethodID cache is present only for the main class > version and contains `jmethodID` values for both the old and current method > versions. > > Therefore we need to perform `jmethodID` lookup when we are about to > deallocate a `Method` instance and clean up the pointer only if that > `jmethodID` is pointing to the `Method` instance which is being deallocated. > > _(For anyone interested, a much lengthier writeup is available in [my > blog](https://jbachorik.github.io/posts/mysterious-jmethodid))_ src/hotspot/share/oops/instanceKlass.cpp line 541: > 539: // The previous version will point to them so they're not totally > dangling > 540: assert (!method->on_stack(), "shouldn't be called with methods on > stack"); > 541: // Do the pointer maintenance before releasing the metadata, but > not for incomplete methods I'm confused by what you mean by method holder, which I think of as methodHandle. Or InstanceKlass is the holder of the methods. Maybe this should be more explicit that it's talking about clearing any associated jmethodIDs. - PR Review Comment: https://git.openjdk.org/jdk/pull/16662#discussion_r1397970676
Integrated: 8309599: WeakHandle and OopHandle release should clear obj pointer
On Tue, 26 Sep 2023 12:47:42 GMT, Coleen Phillimore wrote: > This change makes WeakHandle and OopHandle release null out the obj pointer, > at the cost of making the release function non-const and some changes that > propagated from that. This enables ObjectMonitor code to test for null to > see if the obj was already released, and seems like the right thing to do. > See comments from related PR in the bug report. > Tested with tier1-4. This pull request has now been integrated. Changeset: 0c55887b Author: Coleen Phillimore URL: https://git.openjdk.org/jdk/commit/0c55887bfb131501a26ba431919d94f2ba08a6c1 Stats: 13 lines in 8 files changed: 2 ins; 1 del; 10 mod 8309599: WeakHandle and OopHandle release should clear obj pointer Reviewed-by: dholmes, kbarrett - PR: https://git.openjdk.org/jdk/pull/15920
Re: RFR: 8309599: WeakHandle and OopHandle release should clear obj pointer
On Thu, 28 Sep 2023 01:46:37 GMT, David Holmes wrote: > Hmmm okay - it seems fragile to have a psuedo-destructor in release(). I don't know what this comment means. It was fragile to *not* have release destroy the _obj pointer, which was the cause of the original confusion and problems while fixing the object monitor deflation code. Thanks Kim and David for the code reviews. - PR Comment: https://git.openjdk.org/jdk/pull/15920#issuecomment-1739005387
Re: RFR: 8309599: WeakHandle and OopHandle release should clear obj pointer
On Tue, 26 Sep 2023 12:47:42 GMT, Coleen Phillimore wrote:
> This change makes WeakHandle and OopHandle release null out the obj pointer,
> at the cost of making the release function non-const and some changes that
> propagated from that. This enables ObjectMonitor code to test for null to
> see if the obj was already released, and seems like the right thing to do.
> See comments from related PR in the bug report.
> Tested with tier1-4.
OopHandles and WeakHandles don't have destructors (or copy constructors and
assignment operators). Places where we release the storage would have to be
reworked significantly to have this sort of usage model: eg, releasing a String
from the StringTable is done with a callback from the CHT
static void free_node(void* context, void* memory, Value const& value) {
value.release(StringTable::_oop_storage);
FreeHeap(memory);
StringTable::item_removed();
}
-
PR Comment: https://git.openjdk.org/jdk/pull/15920#issuecomment-1737281090
Re: RFR: 8299915: Remove ArrayAllocatorMallocLimit and associated code [v4]
On Tue, 26 Sep 2023 08:35:10 GMT, Afshin Zafari wrote:
>> 1. `ArrayAllocatorMallocLimit` is removed. The test cases that tested it
>> also are removed.
>> 2. `AllocArrayAllocator` instances are replaced with `MallocArrayAllocator`.
>> 3. The signature of `CHeapBitMap::free(ptr, size)` is kept as it is, since
>> it is called in this way from `GrowableBitMap::resize`, where `T` can be
>> also `ArenaBitMap` and `ResourceBitMap`. However, it uses
>> `MallocArrayAllocator::free(ptr)` and ignores the `size`:
>> ```C++
>> void CHeapBitMap::free(bm_word_t* map, idx_t size_in_words) const {
>> MallocArrayAllocator::free(map);
>> }
>>
>> ### Test
>> tiers1-4 passed on all platforms.
>
> Afshin Zafari has updated the pull request incrementally with one additional
> commit since the last revision:
>
> MetaspaceSize and its lower bound is used.
This looks good to me.
-
Marked as reviewed by coleenp (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/15859#pullrequestreview-1645212245
Re: RFR: 8309599: WeakHandle and OopHandle release should clear obj pointer
On Tue, 26 Sep 2023 12:47:42 GMT, Coleen Phillimore wrote: > This change makes WeakHandle and OopHandle release null out the obj pointer, > at the cost of making the release function non-const and some changes that > propagated from that. This enables ObjectMonitor code to test for null to > see if the obj was already released, and seems like the right thing to do. > See comments from related PR in the bug report. > Tested with tier1-4. This change doesn't fix the problem that you had observed with PR #13721. (I agree with the comments in that PR). It's a general cleanup where after calling release() the ObjectMonitor code wants some way to know that it has done so, without adding a specific call to WeakHandle::set_null() to do what the callers expect release() to do. I don't really like making the callers non-const, but I like having a special set_null() API less. Even with object -> OM mapping, the OM will still need to weakly point to the object, unless the design is changed a lot in the last couple of weeks. - PR Comment: https://git.openjdk.org/jdk/pull/15920#issuecomment-1735587388
RFR: 8309599: WeakHandle and OopHandle release should clear obj pointer
This change makes WeakHandle and OopHandle release null out the obj pointer, at the cost of making the release function non-const and some changes that propagated from that. This enables ObjectMonitor code to test for null to see if the obj was already released, and seems like the right thing to do. See comments from related PR in the bug report. Tested with tier1-4. - Commit messages: - 8309599: WeakHandle and OopHandle release should clear obj pointer Changes: https://git.openjdk.org/jdk/pull/15920/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15920=00 Issue: https://bugs.openjdk.org/browse/JDK-8309599 Stats: 13 lines in 8 files changed: 2 ins; 1 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/15920.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15920/head:pull/15920 PR: https://git.openjdk.org/jdk/pull/15920
Re: RFR: 8316658: serviceability/jvmti/RedefineClasses/RedefineLeakThrowable.java fails intermittently
On Fri, 22 Sep 2023 08:36:10 GMT, Jean-Philippe Bempel wrote: > increase Metaspace size and loop count to avoid OOME in nominal case This seems okay if it helps the test run reliably. I don't know a better way to test the absence of a memory leak. If this fails again, maybe we need to rewrite RedefineClassHelper to only generate the class bytes once, not each time. This is a trivial change and seems to already have David's implicit approval in the CR, so I don't think you need to wait 24 hours. You can if you want though. - Marked as reviewed by coleenp (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15882#pullrequestreview-1640111084
