On Thu, 24 Apr 2025 17:15:22 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Less unnecessary logging of expected exceptions
>
> test/jdk/sun/jvmstat/monitor/MonitoredVm/MonitorVmStartTerminate.java line
> 254:
>
>> 252: String timeoutFactorText =
>> System.getProperty("test.timeout.factor", "1.0");
>> 253: double timeoutFactor =
>> Double.parseDouble(timeoutFactorText);
>> 254: long timeoutNanos = 1000_000_000L*(long)(REMOVE_TIMEOUT *
>> timeoutFactor);
>
> I don't understand the need for this already large timeout, and making it
> larger (now 2000 seconds instead of 1000). Is it because failures in
> hasMainArgs() takes time to timeout? If so, I thought attach failures only
> took 10 seconds to time out, and we are at most retrying 5 times.
Yes two problems with that timeout change, so I'll update it again:
(1)
I think I misread the 1000 as milliseconds. It just can't be intentionally a
1000 second timeout, 17 minutes, and it does get multiplied by timeout factor
(which can be 10 in some runs).
I expect it was meant to be 1 second many years ago, as it was multiplied by
timeout factor.
Even my local run with -Xcomp -ea -esa -XX:CompileThreshold=100
-XX:+UnlockExperimentalVMOptions -server -XX:-TieredCompilation
..this part usually takes about 1.5 seconds and the test works reliably due to
timeout factor getting set to 10.
This may also explain some very very long delays when it has failed 8-)
Updating! 300 seconds * timeout factor would be very cautious.
I think this and the main args fetching timeout work together to make the test
fail/timeout should something clearly not be working, and the Xcomp and other
slow compiler stress test settings test these choices.
(2)
This particular timeout which was 1000 seconds x timeout.factor was a big part
of why the test filled and truncated its logs when timing out.
With this change we don't log about our wait every iteration, just when done.
As we aren't logging, we shouldn't fill and truncate the log so it doesn't
matter very much how long we wait. But with this hint that it was actually
1000 seconds, it should go down, not up!
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24843#discussion_r2060144129
