Re: RFR: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected [v3]
On Wed, 6 Dec 2023 02:33:21 GMT, David Holmes wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> fixed trailing whitespace > > src/hotspot/share/prims/jvmtiThreadState.cpp line 561: > >> 559: // it is an important optimization to create JvmtiThreadState objects >> lazily. >> 560: // This optimization is disabled when watchpoint capabilities are >> present. It is to >> 561: // work around a bug with virtual thread frames which can be not >> deoptimized in time. > > Suggestion: "This optimization is *also* disabled when ..." > > The phrase "which can be not deoptimized in time." is unclear. Are we racing > with deoptimization? Than you for the suggestion. In fact, I did not finish with the scalability related optimizations and will continue in 23. Will correct this comment as you suggest when there is any chance. > The phrase "which can be not deoptimized in time." is unclear. Are we racing > with deoptimization? Yes, this comment can be not fully correct as you noted. I do not fully understand optimization vs deoptimization mechanisms. I've already spent a lot of time trying to isolate this deoptimization issue but still need to continue this work in 23. Good news is that it can be reliably reproducible but only with a full run any of 4-6 tier. It is not reproducible locally yet. My understanding is that the deoptimization needs some time to happen. We mark frames as needed to deoptimize and they are really deoptimized upon return of the execution control. However, there can be some subtle details depending on the execution path. There can be more then one bug in this area. - PR Review Comment: https://git.openjdk.org/jdk/pull/16961#discussion_r1417929323
Re: RFR: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected [v2]
On Tue, 5 Dec 2023 23:01:20 GMT, Daniel D. Daugherty wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: extended comment to cover the watchpoint extra checks > > Thumbs up. This is a trivial fix. > > You'll need to fix the whitespace complaint before integration. > @dcubed-ojdk I would not consider this a trivial fix at all - the need to add > the additional conditions is not at all obvious! > And even if they were, that would make this a small/simple fix, not "trivial" > as defined for the "one review needed" rule. Sorry, David. It was kind of obvious to me as this tweak is a work around the field watch related regression. Of course, it would better to wait for you to finish review but it was not clear if you are going to complete it or not. As you know, it is very uncomfortable to do last minute push even if it is trivial. :( - PR Comment: https://git.openjdk.org/jdk/pull/16961#issuecomment-1843587781
Re: RFR: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected [v3]
On Tue, 5 Dec 2023 23:36:46 GMT, Serguei Spitsyn wrote: >> This is a trivial fix for a regression caused by: >> [8308614](https://bugs.openjdk.org/browse/JDK-8308614) Enabling JVMTI >> ClassLoad event slows down vthread creation by factor 10 >> >> The fix of 8308614 just triggered a known issue: >> [8316283](https://bugs.openjdk.org/browse/JDK-8316283) field watch events >> are not always posted with -Xcomp option >> >> The fix is just a work around with the extra checks with the >> `JvmtiExport::should_post_field_access()` and >> `JvmtiExport::should_post_field_modification()`. >> >> Testing: >> - The test `runtime/jni/FastGetField/FastGetField.java` does not fail >> anymore with this fix >> - In progress: Test with tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > fixed trailing whitespace src/hotspot/share/prims/jvmtiThreadState.cpp line 561: > 559: // it is an important optimization to create JvmtiThreadState objects > lazily. > 560: // This optimization is disabled when watchpoint capabilities are > present. It is to > 561: // work around a bug with virtual thread frames which can be not > deoptimized in time. Suggestion: "This optimization is *also* disabled when ..." The phrase "which can be not deoptimized in time." is unclear. Are we racing with deoptimization? - PR Review Comment: https://git.openjdk.org/jdk/pull/16961#discussion_r1416579588
Re: RFR: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected [v2]
On Tue, 5 Dec 2023 23:01:20 GMT, Daniel D. Daugherty wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: extended comment to cover the watchpoint extra checks > > Thumbs up. This is a trivial fix. > > You'll need to fix the whitespace complaint before integration. @dcubed-ojdk I would not consider this a trivial fix at all - the need to add the additional conditions is not at all obvious! And even if they were, that would make this a small/simple fix, not "trivial" as defined for the "one review needed" rule. - PR Comment: https://git.openjdk.org/jdk/pull/16961#issuecomment-1841982060
Re: RFR: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected [v3]
> This is a trivial fix for a regression caused by: > [8308614](https://bugs.openjdk.org/browse/JDK-8308614) Enabling JVMTI > ClassLoad event slows down vthread creation by factor 10 > > The fix of 8308614 just triggered a known issue: > [8316283](https://bugs.openjdk.org/browse/JDK-8316283) field watch events > are not always posted with -Xcomp option > > The fix is just a work around with the extra checks with the > `JvmtiExport::should_post_field_access()` and > `JvmtiExport::should_post_field_modification()`. > > Testing: > - The test `runtime/jni/FastGetField/FastGetField.java` does not fail anymore > with this fix > - In progress: Test with tiers 1-6 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: fixed trailing whitespace - Changes: - all: https://git.openjdk.org/jdk/pull/16961/files - new: https://git.openjdk.org/jdk/pull/16961/files/7d47fd12..34da9c6e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16961=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16961=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16961.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16961/head:pull/16961 PR: https://git.openjdk.org/jdk/pull/16961
Re: RFR: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected [v2]
On Tue, 5 Dec 2023 22:51:47 GMT, Serguei Spitsyn wrote: >> This is a trivial fix for a regression caused by: >> [8308614](https://bugs.openjdk.org/browse/JDK-8308614) Enabling JVMTI >> ClassLoad event slows down vthread creation by factor 10 >> >> The fix of 8308614 just triggered a known issue: >> [8316283](https://bugs.openjdk.org/browse/JDK-8316283) field watch events >> are not always posted with -Xcomp option >> >> The fix is just a work around with the extra checks with the >> `JvmtiExport::should_post_field_access()` and >> `JvmtiExport::should_post_field_modification()`. >> >> Testing: >> - The test `runtime/jni/FastGetField/FastGetField.java` does not fail >> anymore with this fix >> - In progress: Test with tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: extended comment to cover the watchpoint extra checks Dan, thank you a lot for quick review! I'll fix the whitespace issue. - PR Comment: https://git.openjdk.org/jdk/pull/16961#issuecomment-1841792398
Re: RFR: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected [v2]
On Tue, 5 Dec 2023 22:51:47 GMT, Serguei Spitsyn wrote: >> This is a trivial fix for a regression caused by: >> [8308614](https://bugs.openjdk.org/browse/JDK-8308614) Enabling JVMTI >> ClassLoad event slows down vthread creation by factor 10 >> >> The fix of 8308614 just triggered a known issue: >> [8316283](https://bugs.openjdk.org/browse/JDK-8316283) field watch events >> are not always posted with -Xcomp option >> >> The fix is just a work around with the extra checks with the >> `JvmtiExport::should_post_field_access()` and >> `JvmtiExport::should_post_field_modification()`. >> >> Testing: >> - The test `runtime/jni/FastGetField/FastGetField.java` does not fail >> anymore with this fix >> - In progress: Test with tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: extended comment to cover the watchpoint extra checks Thumbs up. This is a trivial fix. You'll need to fix the whitespace complaint before integration. - Marked as reviewed by dcubed (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16961#pullrequestreview-1766264031
Re: RFR: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected [v2]
> This is a trivial fix for a regression caused by: > [8308614](https://bugs.openjdk.org/browse/JDK-8308614) Enabling JVMTI > ClassLoad event slows down vthread creation by factor 10 > > The fix of 8308614 just triggered a known issue: > [8316283](https://bugs.openjdk.org/browse/JDK-8316283) field watch events > are not always posted with -Xcomp option > > The fix is just a work around with the extra checks with the > `JvmtiExport::should_post_field_access()` and > `JvmtiExport::should_post_field_modification()`. > > Testing: > - The test `runtime/jni/FastGetField/FastGetField.java` does not fail anymore > with this fix > - In progress: Test with tiers 1-6 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: extended comment to cover the watchpoint extra checks - Changes: - all: https://git.openjdk.org/jdk/pull/16961/files - new: https://git.openjdk.org/jdk/pull/16961/files/c08484ac..7d47fd12 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16961=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16961=00-01 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16961.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16961/head:pull/16961 PR: https://git.openjdk.org/jdk/pull/16961
Re: RFR: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected
On Tue, 5 Dec 2023 04:41:29 GMT, David Holmes wrote:
>> The fix is for a regression caused by:
>> [8308614](https://bugs.openjdk.org/browse/JDK-8308614) Enabling JVMTI
>> ClassLoad event slows down vthread creation by factor 10
>>
>> The fix of 8308614 just triggered a known issue:
>> [8316283](https://bugs.openjdk.org/browse/JDK-8316283) field watch events
>> are not always posted with -Xcomp option
>>
>> The fix is just a work around with the extra checks with the
>> `JvmtiExport::should_post_field_access()` and
>> `JvmtiExport::should_post_field_modification()`.
>>
>> Testing:
>> - The test `runtime/jni/FastGetField/FastGetField.java` does not fail
>> anymore with this fix
>> - In progress: Test with tiers 1-6
>
> src/hotspot/share/prims/jvmtiThreadState.cpp line 562:
>
>> 560: if (JvmtiThreadState::seen_interp_only_mode() ||
>> 561: JvmtiExport::should_post_field_access() ||
>> 562: JvmtiExport::should_post_field_modification()){
>
> The comment needs updating to explain the extra checks.
>
> Can't say I see the connection with
> [8316283](https://bugs.openjdk.org/browse/JDK-8316283) as no `-Xcomp` is
> involved in the current failures AFAICS.
Thank you for the suggestion. I'll add a comment.
The `-Xcomp` flag is not a root cause but a trigger. There is a general issue
that a frame can be not deoptimized. The `-Xcomp` is a stress that helps to
reproduce the issue as the `-Xcomp` option and `interp_only_mode` works in
opposite directions.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16961#discussion_r1415125933
Re: RFR: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected
On Tue, 5 Dec 2023 00:23:45 GMT, Serguei Spitsyn wrote:
> The fix is for a regression caused by:
> [8308614](https://bugs.openjdk.org/browse/JDK-8308614) Enabling JVMTI
> ClassLoad event slows down vthread creation by factor 10
>
> The fix of 8308614 just triggered a known issue:
> [8316283](https://bugs.openjdk.org/browse/JDK-8316283) field watch events
> are not always posted with -Xcomp option
>
> The fix is just a work around with the extra checks with the
> `JvmtiExport::should_post_field_access()` and
> `JvmtiExport::should_post_field_modification()`.
>
> Testing:
> - The test `runtime/jni/FastGetField/FastGetField.java` does not fail anymore
> with this fix
> - In progress: Test with tiers 1-6
src/hotspot/share/prims/jvmtiThreadState.cpp line 562:
> 560: if (JvmtiThreadState::seen_interp_only_mode() ||
> 561: JvmtiExport::should_post_field_access() ||
> 562: JvmtiExport::should_post_field_modification()){
The comment needs updating to explain the extra checks.
Can't say I see the connection with
[8316283](https://bugs.openjdk.org/browse/JDK-8316283) as no `-Xcomp` is
involved in the current failures AFAICS.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16961#discussion_r1414867564
RFR: 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected
The fix is for a regression caused by: [8308614](https://bugs.openjdk.org/browse/JDK-8308614) Enabling JVMTI ClassLoad event slows down vthread creation by factor 10 The fix of 8308614 just triggered a known issue: [8316283](https://bugs.openjdk.org/browse/JDK-8316283) field watch events are not always posted with -Xcomp option The fix is just a work around with the extra checks of the `JvmtiExport::should_post_field_access()` and `JvmtiExport::should_post_field_modification()`. Testing: - In progress: Test with tiers 1-6 - Commit messages: - 8321219: runtime/jni/FastGetField: assert(is_interpreted_frame()) failed: interpreted frame expected Changes: https://git.openjdk.org/jdk/pull/16961/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16961=00 Issue: https://bugs.openjdk.org/browse/JDK-8321219 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16961.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16961/head:pull/16961 PR: https://git.openjdk.org/jdk/pull/16961
