RFR: 8329332: Remove CompiledMethod and CodeBlobLayout classes
Revert [JDK-8152664](https://bugs.openjdk.org/browse/JDK-8152664) RFE [changes](https://github.com/openjdk/jdk/commit/b853eb7f5ca24eeeda18acbb14287f706499c365) which was used for AOT [JEP 295](https://openjdk.org/jeps/295) implementation in JDK 9. The code was left in HotSpot assuming it will help in a future. But during work on Leyden we decided to not use it. In Leyden cached compiled code will be restored in CodeCache as normal nmethods: no need to change VM's runtime and GC code to process them. I may work on optimizing `CodeBlob` and `nmethod` fields layout to reduce header size in separate changes. In these changes I did simple fields reordering to keep small (1 byte) fields together. I do not see (and not expected) performance difference with these changes. Tested tier1-5, xcomp, stress. Running performance testing. I need help with testing on platforms which Oracle does not support. - Commit messages: - remove trailing whitespace - 8329332: Remove CompiledMethod and CodeBlobLayout classes Changes: https://git.openjdk.org/jdk/pull/18554/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18554=00 Issue: https://bugs.openjdk.org/browse/JDK-8329332 Stats: 3885 lines in 118 files changed: 1281 ins; 1723 del; 881 mod Patch: https://git.openjdk.org/jdk/pull/18554.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18554/head:pull/18554 PR: https://git.openjdk.org/jdk/pull/18554
Re: RFR: 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout [v4]
On Fri, 29 Mar 2024 23:02:03 GMT, Serguei Spitsyn wrote: >> So that would mean that the native side would always wait for 100 seconds? >> Or will it wait for some increment of time upto a maximum of 100 seconds? > > It wait for some increment of time upto a maximum of 100 seconds. I'm good with that. Thanks for clarifying. - PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1544999877
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 Thank you for explaining, Alex. I see the complexity of the "simple" solution. :) My guess is that all workarounds are going to be ugly, so I'd suggest to use the simplest one (your initial version). - PR Comment: https://git.openjdk.org/jdk/pull/18540#issuecomment-2027811158
Re: RFR: 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout [v4]
On Fri, 29 Mar 2024 22:18:24 GMT, Daniel D. Daugherty wrote: >> Thank you for the example and for catching the typo. The timeout factor also >> needs to be passed to the native side. I think, this fragment is not worth >> this kind of extra complexity. One approach would be to just make it big >> enough, eg. make it 100 seconds instead of 10. Another - to get rid of this >> trap at all. What would you prefer? > > So that would mean that the native side would always wait for 100 seconds? > Or will it wait for some increment of time upto a maximum of 100 seconds? It wait for some increment of time upto a maximum of 100 seconds. - PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1544937616
Re: RFR: 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout [v4]
On Fri, 29 Mar 2024 20:44:48 GMT, Serguei Spitsyn wrote:
>> runtime/8176717/TestInheritFD.java has an example of what I'm talking about:
>>
>> public static float timeoutFactor =
>> Float.parseFloat(System.getProperty("test.timeout.factor", "1.0"));
>> public static long subProcessTimeout = (long)(15L * timeoutFactor);
>>
>> so you fetch the test.timeout.factor value and then you scale your delay
>> value.
>>
>> Also:
>>
>> nit typo: s/waitig/waiting/
>
> Thank you for the example and for catching the typo. The timeout factor also
> needs to be passed to the native side. I think, this fragment is not worth
> this kind of extra complexity. One approach would be to just make it big
> enough, eg. make it 100 seconds instead of 10. Another - to get rid of this
> trap at all. What would you prefer?
So that would mean that the native side would always wait for 100 seconds?
Or will it wait for some increment of time upto a maximum of 100 seconds?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1544915772
Re: RFR: 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout [v6]
On Fri, 29 Mar 2024 18:44:35 GMT, Chris Plummer wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: move log message to the right place
>
> test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/libPopFrameTest.cpp
> line 164:
>
>> 162: fatal(jni, "Main: ensureAtBreakpoint: waited 10 sec");
>> 163: }
>> 164: LOG("Main: ensureAtBreakpoint: waitig 100 millis\n");
>
> Suggestion:
>
> LOG("Main: ensureAtBreakpoint: waiting 100 millis\n");
Thanks, Chris. Will fix it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1544856030
Re: RFR: 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout [v4]
On Fri, 29 Mar 2024 13:29:15 GMT, Daniel D. Daugherty
wrote:
>> Thanks for the comments, Chris and Dan. Updated as Chris suggested. I've
>> added this with `-Xcomp` consideration as the worst case scenario in mind.
>> Now, I think it is more save to make it 10 seconds instead of one. Is it
>> going to be good enough? In fact, I've added this for manual testing to save
>> time in waiting for test completion when it is deadlocked. Also, this is
>> better for diagnosability.
>
> runtime/8176717/TestInheritFD.java has an example of what I'm talking about:
>
> public static float timeoutFactor =
> Float.parseFloat(System.getProperty("test.timeout.factor", "1.0"));
> public static long subProcessTimeout = (long)(15L * timeoutFactor);
>
> so you fetch the test.timeout.factor value and then you scale your delay
> value.
>
> Also:
>
> nit typo: s/waitig/waiting/
Thank you for the example and for catching the typo. The timeout factor also
needs to be passed to the native side. I think, this fragment is not worth this
kind of extra complexity. One approach would be to just make it big enough, eg.
make it 100 seconds instead of 10. Another - to get rid of this trap at all.
What would you prefer?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1544855730
Re: RFR: JDK-8328137: PreserveAllAnnotations can cause failure of class retransformation
On Fri, 29 Mar 2024 07:08:58 GMT, Serguei Spitsyn wrote:
> > 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).
>
> This solution looks okay in general but still is a little bit confusing. It
> feels like saving just one bit would not add much complexity but would even
> simplify the implementation as it will become straight-forward. At least,
> there would be no need in this additional function with the
> `fallback_attr_name` parameter.
It's a bit more complicated than storing 1 bit.
Lets consider ClassFileParser::parse_classfile_record_attribute as an example.
There are 2 type of annotations for records: RuntimeVisibleAnnotations and
RuntimeVisibleTypeAnnotations.
For each type if PreserveAllAnnotations is set ClassFileParser additionally
handles invisible annotations
(RuntimeInvisibleAnnotations/RuntimeInvisibleTypeAnnotations).
Then it "assembles" annotations (concatenates visible and invisible annotations
into single array).
To restore original annotations we need to keep number of visible annotations
in the annotation array (so jvmtiClassReconstituter writes part of the
annotation array as visible, and the rest as invisible).
And we need to store this number for each annotation type (Annotations and
TypeAnnotations for records, Annotations and TypeAnnotations for class,
Annotations and TypeAnnotations for fields,
Annotations/TypeAnnotations/ParameterAnnotations for methods).
I considered another way to implement the workaround - add method
`bool is_symbol_in_cp(const char* symbol)`
But then each writing would become very verbose and more confusing (needs some
explanation in each place):
if (is_symbol_in_cp("RuntimeVisibleTypeAnnotations")) {
write_annotations_attribute("RuntimeVisibleTypeAnnotations",
component->type_annotations());
} else {
write_annotations_attribute("RuntimeInvisibleTypeAnnotations",
component->type_annotations());
}
-
PR Comment: https://git.openjdk.org/jdk/pull/18540#issuecomment-2027647051
Re: RFR: 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout [v6]
On Fri, 29 Mar 2024 04:15:43 GMT, Serguei Spitsyn wrote:
>> This PR fixes a synchronization issue in the test:
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest`
>>
>> The method `notifyAtBreakpoint()` can notify the `TestTask` thread when it
>> has not reached an expected breakpoint yet.
>> The fix is to add a call to the method `ensureAtBreakpoint()` one more time
>> in the `B2` sub-test. It is needed after the top-most frame was popped with
>> the JVMTI `PopFrame`, and the target thread needs to reach the breakpoint
>> again after its execution was resumed.
>>
>> The time is very intermittent. At least, I was not able to reproduce the
>> timeout failure in thousands of mach5 runs with the `-Xcomp` option.
>>
>> Testing:
>> - Run the test
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest` thousands
>> times in mach5
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: move log message to the right place
test/hotspot/jtreg/serviceability/jvmti/vthread/PopFrameTest/libPopFrameTest.cpp
line 164:
> 162: fatal(jni, "Main: ensureAtBreakpoint: waited 10 sec");
> 163: }
> 164: LOG("Main: ensureAtBreakpoint: waitig 100 millis\n");
Suggestion:
LOG("Main: ensureAtBreakpoint: waiting 100 millis\n");
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1544769747
Re: RFR: 8328665: serviceability/jvmti/vthread/PopFrameTest failed with a timeout [v4]
On Thu, 28 Mar 2024 23:29:53 GMT, Serguei Spitsyn wrote:
>> Caught this comment in passing. Delays like this should be scaled with
>> defaultTimeoutFactor so that test tasks that invoke tests with options
>> that can slow the test down, e.g., `-Xcomp`, can be accommodated.
>>
>> I believe the defaultTimeoutFact for `-Xcomp` test tasks gets bumped
>> from 4 to 10.
>
> Thanks for the comments, Chris and Dan. Updated as Chris suggested. I've
> added this with `-Xcomp` consideration as the worst case scenario in mind.
> Now, I think it is more save to make it 10 seconds instead of one. Is it
> going to be good enough? In fact, I've added this for manual testing to save
> time in waiting for test completion when it is deadlocked. Also, this is
> better for diagnosability.
runtime/8176717/TestInheritFD.java has an example of what I'm talking about:
public static float timeoutFactor =
Float.parseFloat(System.getProperty("test.timeout.factor", "1.0"));
public static long subProcessTimeout = (long)(15L * timeoutFactor);
so you fetch the test.timeout.factor value and then you scale your delay value.
Also:
nit typo: s/waitig/waiting/
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18419#discussion_r1544497738
Re: RFR: 8329103: assert(!thread->in_asgct()) failed during multi-mode profiling [v2]
On Fri, 29 Mar 2024 11:35:57 GMT, Andrei Pangin wrote: >> This fix makes `AsyncGetCallTrace` reentrant and async-signal-safe. >> Reentrancy is required in the cases when two or more profiling engines are >> running at the same time, e.g., when CPU and Wall clock profilers work >> together and therefore one profiler may interrupt another in the middle of >> getting a stack trace. >> >> Tested with async-profiler: >> >> java >> -agentpath:/path/to/libasyncProfiler.so=start,event=cpu,interval=1ms,wall=1ms,file=profile.jfr > > Andrei Pangin has updated the pull request incrementally with one additional > commit since the last revision: > > Rephrased comment about AsyncGetCallTrace reentrancy Marked as reviewed by sspitsyn (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18504#pullrequestreview-1968490876
Re: RFR: 8329103: assert(!thread->in_asgct()) failed during multi-mode profiling [v2]
On Fri, 29 Mar 2024 05:55:01 GMT, Serguei Spitsyn wrote:
>> Andrei Pangin has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Rephrased comment about AsyncGetCallTrace reentrancy
>
> src/hotspot/share/runtime/thread.hpp line 664:
>
>> 662: ThreadInAsgct(Thread* thread) : _thread(thread) {
>> 663: assert(thread != nullptr, "invariant");
>> 664: // AsyncGetCallTrace is reentrant - save the previous state.
>
> Nit: It is possible to rephrase this comment as follows:
>
> // Allow AsyncGetCallTrace to be reentrant - save the previous state.
> ``
Thank you for the review - I updated the comment.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18504#discussion_r1544400438
Re: RFR: 8329103: assert(!thread->in_asgct()) failed during multi-mode profiling [v2]
> This fix makes `AsyncGetCallTrace` reentrant and async-signal-safe. > Reentrancy is required in the cases when two or more profiling engines are > running at the same time, e.g., when CPU and Wall clock profilers work > together and therefore one profiler may interrupt another in the middle of > getting a stack trace. > > Tested with async-profiler: > > java > -agentpath:/path/to/libasyncProfiler.so=start,event=cpu,interval=1ms,wall=1ms,file=profile.jfr Andrei Pangin has updated the pull request incrementally with one additional commit since the last revision: Rephrased comment about AsyncGetCallTrace reentrancy - Changes: - all: https://git.openjdk.org/jdk/pull/18504/files - new: https://git.openjdk.org/jdk/pull/18504/files/af9a57fd..fc5a4831 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18504=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18504=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18504.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18504/head:pull/18504 PR: https://git.openjdk.org/jdk/pull/18504
Re: RFR: JDK-8328137: PreserveAllAnnotations can cause failure of class retransformation
On Thu, 28 Mar 2024 22:12:49 GMT, Alex Menkov wrote: > 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). This solution looks okay in general but still is a little bit confusing. It feels like saving just one bit would not add much complexity but would even simplify the implementation as it will become straight-forward. At least, there would be no need in this additional function with the `fallback_attr_name` parameter. - PR Comment: https://git.openjdk.org/jdk/pull/18540#issuecomment-2026785793
