Re: RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold [v2]
On Fri, 13 May 2022 12:27:16 GMT, Zhengyu Gu wrote: > LGTM Thanks @zhengyu123 for the review. - PR: https://git.openjdk.java.net/jdk/pull/8691
Re: RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold [v2]
> Hi all, > > Some tests fail with Shenandoah GC after JDK-8282191. > The reason is that the assert in `ShenandoahControlThread::request_gc` misses > the case of `GCCause::_codecache_GC_threshold`. > It would be better to fix it. > > Thanks. > Best regards, > Jie Jie Fu has updated the pull request incrementally with one additional commit since the last revision: Revert the test change - Changes: - all: https://git.openjdk.java.net/jdk/pull/8691/files - new: https://git.openjdk.java.net/jdk/pull/8691/files/8f094d32..dc96246c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8691=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8691=00-01 Stats: 15 lines in 1 file changed: 0 ins; 15 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8691.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8691/head:pull/8691 PR: https://git.openjdk.java.net/jdk/pull/8691
Re: RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold [v2]
On Fri, 13 May 2022 13:18:55 GMT, David Holmes wrote: >>> I think I agree with @AlanBateman - in the sense that this seems to go down >>> a slippery slope where every test would need to be executed against all >>> possible GCs. AFAIK, there are no other foreign tests doing this. >> >> I think it's a waste of time to write a separate test for this bug. >> >> I can't understand why you are against adding one more test config in the >> foreign test. >> Yes, it caught the bug in ShenandoahGC but who knows it wouldn't find some >> other bugs in the foreign api in the future. >> If you are still against the newly added test config, I can revert the test >> change. >> Thanks. > > My comment seems to have never made it through. The problem with what your > are doing is two-fold: > 1. When running the test suite specifically to test xGC for whatever x you > are now forcing a test run for Shenandoah. > 2. When x is Shenandoah then you run this test twice. > > You don't need a config just to run this with Shenandoah - you run the test > suite with Shenandoah. The test change had been reverted. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8691
Re: RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold
On Fri, 13 May 2022 13:02:52 GMT, Jie Fu wrote: >> I assume you are running the tests with: >>make run-tests TEST_OPTS_JAVA_OPTIONS="-XX:+UseShenandoahGC" >> in which case, all of the tests you select to run will be run with that GC. >> >> What you have is not wrong but wouldn't be a better to add a new test to >> test/hotspot/jtreg/gc rather than relying on test in test/jdk/java/foreign? > >> I think I agree with @AlanBateman - in the sense that this seems to go down >> a slippery slope where every test would need to be executed against all >> possible GCs. AFAIK, there are no other foreign tests doing this. > > I think it's a waste of time to write a separate test for this bug. > > I can't understand why you are against adding one more test config in the > foreign test. > Yes, it caught the bug in ShenandoahGC but who knows it wouldn't find some > other bugs in the foreign api in the future. > If you are still against the newly added test config, I can revert the test > change. > Thanks. My comment seems to have never made it through. The problem with what your are doing is two-fold: 1. When running the test suite specifically to test xGC for whatever x you are now forcing a test run for Shenandoah. 2. When x is Shenandoah then you run this test twice. You don't need a config just to run this with Shenandoah - you run the test suite with Shenandoah. - PR: https://git.openjdk.java.net/jdk/pull/8691
Re: RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold
On Fri, 13 May 2022 06:56:23 GMT, Alan Bateman wrote: >> Without `-XX:+UseShenandoahGC`, this bug wouldn't be exposed. >> >> What do you mean by `if you are testing with +ShenandoahGC then it will run >> already`? > > I assume you are running the tests with: >make run-tests TEST_OPTS_JAVA_OPTIONS="-XX:+UseShenandoahGC" > in which case, all of the tests you select to run will be run with that GC. > > What you have is not wrong but wouldn't be a better to add a new test to > test/hotspot/jtreg/gc rather than relying on test in test/jdk/java/foreign? > I think I agree with @AlanBateman - in the sense that this seems to go down a > slippery slope where every test would need to be executed against all > possible GCs. AFAIK, there are no other foreign tests doing this. I think it's a waste of time to write a separate test for this bug. I can't understand why you are against adding one more test config in the foreign test. Yes, it caught the bug in ShenandoahGC but who knows it wouldn't find some other bugs in the foreign api in the future. If you are still against the newly added test config, I can revert the test change. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8691
Re: RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold
On Fri, 13 May 2022 06:56:23 GMT, Alan Bateman wrote: >> Without `-XX:+UseShenandoahGC`, this bug wouldn't be exposed. >> >> What do you mean by `if you are testing with +ShenandoahGC then it will run >> already`? > > I assume you are running the tests with: >make run-tests TEST_OPTS_JAVA_OPTIONS="-XX:+UseShenandoahGC" > in which case, all of the tests you select to run will be run with that GC. > > What you have is not wrong but wouldn't be a better to add a new test to > test/hotspot/jtreg/gc rather than relying on test in test/jdk/java/foreign? I think I agree with @AlanBateman - in the sense that this seems to go down a slippery slope where every test would need to be executed against all possible GCs. AFAIK, there are no other foreign tests doing this. - PR: https://git.openjdk.java.net/jdk/pull/8691
Re: RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold
On Fri, 13 May 2022 02:43:55 GMT, Jie Fu wrote: > Hi all, > > Some tests fail with Shenandoah GC after JDK-8282191. > The reason is that the assert in `ShenandoahControlThread::request_gc` misses > the case of `GCCause::_codecache_GC_threshold`. > It would be better to fix it. > > Thanks. > Best regards, > Jie LGTM - Marked as reviewed by zgu (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8691
Re: RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold
On Fri, 13 May 2022 06:56:23 GMT, Alan Bateman wrote: > I assume you are running the tests with: make run-tests > TEST_OPTS_JAVA_OPTIONS="-XX:+UseShenandoahGC" in which case, all of the tests > you select to run will be run with that GC. Yes, you're right. > What you have is not wrong but wouldn't be a better to add a new test to > test/hotspot/jtreg/gc rather than relying on test in test/jdk/java/foreign? I would suggest reusing the existing test in java/foreign. This is because this bug was first triggered after `Implementation of Foreign Function & Memory API (Preview)` integration. And I don't want a copy-paste code duplication in a new test file. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8691
Re: RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold
On Fri, 13 May 2022 06:47:20 GMT, Jie Fu wrote: >> test/jdk/java/foreign/TestIntrinsics.java line 48: >> >>> 46: * -XX:+UseShenandoahGC >>> 47: * TestIntrinsics >>> 48: */ >> >> Is this needed? The parameters looks the same as the first test description >> so if you are testing with +ShenandoahGC then it will run already. > > Without `-XX:+UseShenandoahGC`, this bug wouldn't be exposed. > > What do you mean by `if you are testing with +ShenandoahGC then it will run > already`? I assume you are running the tests with: make run-tests TEST_OPTS_JAVA_OPTIONS="-XX:+UseShenandoahGC" in which case, all of the tests you select to run will be run with that GC. What you have is not wrong but wouldn't be a better to add a new test to test/hotspot/jtreg/gc rather than relying on test in test/jdk/java/foreign? - PR: https://git.openjdk.java.net/jdk/pull/8691
Re: RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold
On Fri, 13 May 2022 06:36:42 GMT, Alan Bateman wrote: >> Hi all, >> >> Some tests fail with Shenandoah GC after JDK-8282191. >> The reason is that the assert in `ShenandoahControlThread::request_gc` >> misses the case of `GCCause::_codecache_GC_threshold`. >> It would be better to fix it. >> >> Thanks. >> Best regards, >> Jie > > test/jdk/java/foreign/TestIntrinsics.java line 48: > >> 46: * -XX:+UseShenandoahGC >> 47: * TestIntrinsics >> 48: */ > > Is this needed? The parameters looks the same as the first test description > so if you are testing with +ShenandoahGC then it will run already. Without `-XX:+UseShenandoahGC`, this bug wouldn't be exposed. What do you mean by `if you are testing with +ShenandoahGC then it will run already`? - PR: https://git.openjdk.java.net/jdk/pull/8691
Re: RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold
On Fri, 13 May 2022 02:43:55 GMT, Jie Fu wrote: > Hi all, > > Some tests fail with Shenandoah GC after JDK-8282191. > The reason is that the assert in `ShenandoahControlThread::request_gc` misses > the case of `GCCause::_codecache_GC_threshold`. > It would be better to fix it. > > Thanks. > Best regards, > Jie test/jdk/java/foreign/TestIntrinsics.java line 48: > 46: * -XX:+UseShenandoahGC > 47: * TestIntrinsics > 48: */ Is this needed? The parameters looks the same as the first test description so if you are testing with +ShenandoahGC then it will run already. - PR: https://git.openjdk.java.net/jdk/pull/8691
RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold
Hi all, Some tests fail with Shenandoah GC after JDK-8282191. The reason is that the assert in `ShenandoahControlThread::request_gc` misses the case of `GCCause::_codecache_GC_threshold`. It would be better to fix it. Thanks. Best regards, Jie - Commit messages: - ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold Changes: https://git.openjdk.java.net/jdk/pull/8691/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8691=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286681 Stats: 17 lines in 2 files changed: 17 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8691.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8691/head:pull/8691 PR: https://git.openjdk.java.net/jdk/pull/8691
