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 [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 [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
