Re: RFR: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
On Fri, 15 Dec 2023 05:15:17 GMT, David Holmes wrote: >> Any non-zero exit value is acceptable. The intent here is to check that the >> process didn't complete normally. > > A non-zero non-crashing value. Just wondering what that actually would be? For instance, when we exit due to an uncaught exception, the exit value is 1. - PR Review Comment: https://git.openjdk.org/jdk/pull/17056#discussion_r1427705400
Re: RFR: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
On Wed, 13 Dec 2023 13:29:43 GMT, Jorn Vernee wrote:
>> test/jdk/java/foreign/TestStubAllocFailure.java line 51:
>>
>>> 49: runInNewProcess(UpcallRunner.class, true,
>>> List.of("-XX:ReservedCodeCacheSize=3M"), List.of())
>>> 50: .shouldNotHaveExitValue(0)
>>> 51: .shouldNotHaveFatalError();
>>
>> Just curious what non-zero exit value is actually expected here and below?
>
> Any non-zero exit value is acceptable. The intent here is to check that the
> process didn't complete normally.
A non-zero non-crashing value. Just wondering what that actually would be?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17056#discussion_r1427557140
Re: RFR: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
On Wed, 13 Dec 2023 07:08:53 GMT, David Holmes wrote:
>> Improve the test by being more lenient to related code cache exhaustion
>> errors. The important thing is that we don't terminate with a fatal error,
>> which the new code now checks for explicitly. The check for that is based on
>> what is done by
>> `./test/hotspot/jtreg/runtime/ErrorHandling/HsErrFileUtils.java` .
>>
>> The existing `UpcallTestHelper.Output` class that was previously used to
>> assert on stdout/stderr contents did not have the capability to look for
>> patterns in the output. So, I've taken the opportunity to replace it with
>> the more canonical `OutputAnalyzer` which comes from the test library.
>>
>> Finally, I've also added back the test for downcall stub allocation failure
>> which was removed as part of the initial patch because it was too
>> inconsistent [1]. With the new approach, it should pass reliably as well.
>>
>> Testing: `jdk_foreign` suite (which contains all the affected tests)
>>
>> [1]:
>> https://github.com/openjdk/jdk/pull/16311/commits/9a1360598a91871ce6ec48330849c0e4e0279c64
>
> test/jdk/java/foreign/TestStubAllocFailure.java line 51:
>
>> 49: runInNewProcess(UpcallRunner.class, true,
>> List.of("-XX:ReservedCodeCacheSize=3M"), List.of())
>> 50: .shouldNotHaveExitValue(0)
>> 51: .shouldNotHaveFatalError();
>
> Just curious what non-zero exit value is actually expected here and below?
Any non-zero exit value is acceptable. The intent here is to check that the
process didn't complete normally.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17056#discussion_r1425357636
Re: RFR: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
On Mon, 11 Dec 2023 13:01:25 GMT, Jorn Vernee wrote:
> Improve the test by being more lenient to related code cache exhaustion
> errors. The important thing is that we don't terminate with a fatal error,
> which the new code now checks for explicitly. The check for that is based on
> what is done by
> `./test/hotspot/jtreg/runtime/ErrorHandling/HsErrFileUtils.java` .
>
> The existing `UpcallTestHelper.Output` class that was previously used to
> assert on stdout/stderr contents did not have the capability to look for
> patterns in the output. So, I've taken the opportunity to replace it with the
> more canonical `OutputAnalyzer` which comes from the test library.
>
> Finally, I've also added back the test for downcall stub allocation failure
> which was removed as part of the initial patch because it was too
> inconsistent [1]. With the new approach, it should pass reliably as well.
>
> Testing: `jdk_foreign` suite (which contains all the affected tests)
>
> [1]:
> https://github.com/openjdk/jdk/pull/16311/commits/9a1360598a91871ce6ec48330849c0e4e0279c64
test/jdk/java/foreign/TestStubAllocFailure.java line 51:
> 49: runInNewProcess(UpcallRunner.class, true,
> List.of("-XX:ReservedCodeCacheSize=3M"), List.of())
> 50: .shouldNotHaveExitValue(0)
> 51: .shouldNotHaveFatalError();
Just curious what non-zero exit value is actually expected here and below?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17056#discussion_r1424930505
Re: RFR: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
On Tue, 12 Dec 2023 12:09:50 GMT, Maurizio Cimadamore
wrote:
>> Improve the test by being more lenient to related code cache exhaustion
>> errors. The important thing is that we don't terminate with a fatal error,
>> which the new code now checks for explicitly. The check for that is based on
>> what is done by
>> `./test/hotspot/jtreg/runtime/ErrorHandling/HsErrFileUtils.java` .
>>
>> The existing `UpcallTestHelper.Output` class that was previously used to
>> assert on stdout/stderr contents did not have the capability to look for
>> patterns in the output. So, I've taken the opportunity to replace it with
>> the more canonical `OutputAnalyzer` which comes from the test library.
>>
>> Finally, I've also added back the test for downcall stub allocation failure
>> which was removed as part of the initial patch because it was too
>> inconsistent [1]. With the now approach, it should pass reliably as well.
>>
>> Testing: `jdk_foreign` suite (which contains all the affected tests)
>>
>> [1]:
>> https://github.com/openjdk/jdk/pull/16311/commits/9a1360598a91871ce6ec48330849c0e4e0279c64
>
> test/lib/jdk/test/lib/process/OutputAnalyzer.java line 870:
>
>> 868: * Assert that we did not crash with a hard VM error (generating an
>> hs_err_pidXXX.log)
>> 869: */
>> 870: public void shouldNotHaveFatalError() {
>
> Looking at the usages, I wonder if this should be `checkFatalError(boolean)`
> and, similarly, for the exit value `checkExitValue(int value)` ?
Are you suggesting to rename the existing methods? I don't see
`checkExistValue(int value)` in `OutputAnalyzer`. The existing method naming
pattern for the assertions is always `should(Not)XXX` though, so that's what I
followed here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17056#discussion_r1424066457
Re: RFR: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
On Mon, 11 Dec 2023 13:01:25 GMT, Jorn Vernee wrote:
> Improve the test by being more lenient to related code cache exhaustion
> errors. The important thing is that we don't terminate with a fatal error,
> which the new code now checks for explicitly. The check for that is based on
> what is done by
> `./test/hotspot/jtreg/runtime/ErrorHandling/HsErrFileUtils.java` .
>
> The existing `UpcallTestHelper.Output` class that was previously used to
> assert on stdout/stderr contents did not have the capability to look for
> patterns in the output. So, I've taken the opportunity to replace it with the
> more canonical `OutputAnalyzer` which comes from the test library.
>
> Finally, I've also added back the test for downcall stub allocation failure
> which was removed as part of the initial patch because it was too
> inconsistent [1]. With the now approach, it should pass reliably as well.
>
> Testing: `jdk_foreign` suite (which contains all the affected tests)
>
> [1]:
> https://github.com/openjdk/jdk/pull/16311/commits/9a1360598a91871ce6ec48330849c0e4e0279c64
Marked as reviewed by mcimadamore (Reviewer).
test/lib/jdk/test/lib/process/OutputAnalyzer.java line 870:
> 868: * Assert that we did not crash with a hard VM error (generating an
> hs_err_pidXXX.log)
> 869: */
> 870: public void shouldNotHaveFatalError() {
Looking at the usages, I wonder if this should be `checkFatalError(boolean)`
and, similarly, for the exit value `checkExitValue(int value)` ?
-
PR Review: https://git.openjdk.org/jdk/pull/17056#pullrequestreview-1777325219
PR Review Comment: https://git.openjdk.org/jdk/pull/17056#discussion_r1423901980
Re: RFR: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
On Mon, 11 Dec 2023 13:01:25 GMT, Jorn Vernee wrote:
> Improve the test by being more lenient to related code cache exhaustion
> errors. The important thing is that we don't terminate with a fatal error,
> which the new code now checks for explicitly. The check for that is based on
> what is done by
> `./test/hotspot/jtreg/runtime/ErrorHandling/HsErrFileUtils.java` .
>
> The existing `UpcallTestHelper.Output` class that was previously used to
> assert on stdout/stderr contents did not have the capability to look for
> patterns in the output. So, I've taken the opportunity to replace it with the
> more canonical `OutputAnalyzer` which comes from the test library.
>
> Finally, I've also added back the test for downcall stub allocation failure
> which was removed as part of the initial patch because it was too
> inconsistent [1]. With the now approach, it should pass reliably as well.
>
> Testing: `jdk_foreign` suite (which contains all the affected tests)
>
> [1]:
> https://github.com/openjdk/jdk/pull/16311/commits/9a1360598a91871ce6ec48330849c0e4e0279c64
test/jdk/java/foreign/TestStubAllocFailure.java line 64:
> 62: public static void main(String[] args) throws Throwable {
> 63: FunctionDescriptor descriptor = FunctionDescriptor.ofVoid();
> 64: MethodHandle target =
> MethodHandles.lookup().findStatic(UpcallRunner.class, "target",
> descriptor.toMethodType());
Here I'm extracting the method handle lookup from the loop to make it more
likely that we hit the code path that we are interested in, and don't get
another VirtualMachineError due to this method handle lookup.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17056#discussion_r1422479110
