RFR: 8365919: Replace currentTimeMillis with nanoTime in Stresser.java [v2]

[Thomas Schatzl]

On Fri, 22 Aug 2025 18:37:47 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Mostly a mechanic change from `currentTimeMillis()` to `nanoTime()/1000000`. It also removes `finishTime` to avoid overflowing.
>> 
>> The change in `iteration()` is to ensure `currentTime` is properly initialized.
>> 
>> (Was investigating a timeout on Windows-x64, which led me to this code. I think this fix is good enough by its own.)
>>  
>> Test: tier1-5
>
> Albert Mingkun Yang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - review
>  - Merge branch 'master' into test-nano-time
>  - test-nano-time

Changes requested by tschatzl (Reviewer).

test/hotspot/jtreg/vmTestbase/nsk/share/test/Stresser.java line 184:

> 182:     public void printExecutionInfo(PrintStream out) {
> 183:         println(out, "Completed iterations: " + iterations);
> 184:         println(out, "Execution time: " + (currentTime - startTime)/1000.0 + " seconds");

There should be spaces around the operator. The change sometimes has them and sometimes not.
Suggestion:

        println(out, "Execution time: " + (currentTime - startTime) / 1000.0 + " seconds");

test/hotspot/jtreg/vmTestbase/nsk/share/test/Stresser.java line 214:

> 212:         iterations = 0;
> 213:         long stressTime = options.getTime();
> 214:         startTime = System.nanoTime()/1000000;

If the reason for the change is to use `System.nanoTime()` instead of `System.currentTimeMillis()` because of monoticity, wouldn't it be simpler to add a helper that returns a millisecond value based on `System.nanoTime()` and call that one instead of `System.currentTimeMillis()` and keep everything else the same?

Inlining the `System.nanoTime() / 1000000` statement a few times seems bad. Having a helper function for this might allow to include a comment why `System.currentTimeMillis()` is not used too.

test/hotspot/jtreg/vmTestbase/nsk/share/test/Stresser.java line 276:

> 274:                 && !finished
> 275:                 && (maxIterations == 0 || iterations < maxIterations)
> 276:                 && (options.getTime() == 0 || (currentTime - startTime) < options.getTime() * 1000);

I doubt that inlining `options.getTime() * 1000` literally 10 times instead of storing it in a local (`finishTime`) or having a getter makes the code easier to understand.

Maybe just rename `finishTime` to sth like `stressTime`?

test/hotspot/jtreg/vmTestbase/nsk/share/test/Stresser.java line 320:

> 318:      */
> 319:     public long getTimeLeft() {
> 320:         long elapsedTime = System.nanoTime()/1000000 - startTime;

This could maybe use `getExecutionTime()`? Also the repeated use of the term `System.nanoTime() / 1000000` five time definitely warrants a helper function imo, and allows documentation of the reason for its use.

-------------

PR Review: https://git.openjdk.org/jdk/pull/26879#pullrequestreview-3151131499
PR Review Comment: https://git.openjdk.org/jdk/pull/26879#discussion_r2297919503
PR Review Comment: https://git.openjdk.org/jdk/pull/26879#discussion_r2297945572
PR Review Comment: https://git.openjdk.org/jdk/pull/26879#discussion_r2297958826
PR Review Comment: https://git.openjdk.org/jdk/pull/26879#discussion_r2297928840