RFR: 7903750: TimeBudget (-tb) does not fulfill its promisses

[Galder Zamarreño]

On Wed, 5 Mar 2025 15:52:59 GMT, Jiří Vaněk <jvanek at openjdk.org> wrote:

> Added warning, when time budget is to small, and final run will run at least two times longer.

Changes requested by galder (no project role).

jcstress-core/src/main/java/org/openjdk/jcstress/JCStress.java line 89:

> 87: 
> 88:     private TimeBudget getTimeBudget(ConfigsWithScheduler config) {
> 89:         if (config == null) {

By returning null you'll get an NPE when `printOn` is called in next line. If `config == null` you will get an NPE now. Seems like this change does not change anything. So, without that, I don't see a reason to wrap the TimeBudget instantiation in a private method.

jcstress-core/src/main/java/org/openjdk/jcstress/TimeBudget.java line 152:

> 150:         boolean print=false;
> 151:         if (expectedPerTest > budget.milliseconds() * 2l) {
> 152:             System.out.println(" + +++ FATAL - your tests will never finish as expected. They will run much longer ");

I don't think you should assume `System.out`. All these prints should be on a given `PrintStream out`, see `printOn()` method.

The formatting of the message does not seem to adhere to other messages. You should see how messages are printed elsewhere in the code and try to use same format? E.g.


out.println("FATAL: No matching tests.")


Also, about this particular message, if this is fatal jcstress should not run. I don't think this is really happening here. The `print=true` does not cause jcstress not to run.

jcstress-core/src/main/java/org/openjdk/jcstress/TimeBudget.java line 156:

> 154:         }
> 155:         if (expectedPerTest * 2 < budget.milliseconds() * 2l) {
> 156:             System.out.println(" + +++ WARNING - your time budget will not be used. Tests will end much sooner.");

See how other warning messages are printed. You should follow similar format e.g.


System.out.println("WARNING: Allocation profiling is not available: " + e.getMessage());

jcstress-core/src/main/java/org/openjdk/jcstress/TimeBudget.java line 160:

> 158:         }
> 159:         if (print) {
> 160:             System.out.println(" | For " + expectedTests + " with concurrency factor of " + getConcurentTestsFactor()

If it was me, I would switch these `println` calls for `printf` to make the messages more readable.

jcstress-core/src/main/java/org/openjdk/jcstress/infra/grading/ReportUtils.java line 308:

> 306:         long hours = TimeUnit.MILLISECONDS.toHours(ms);
> 307:         ms -= TimeUnit.HOURS.toMillis(hours);
> 308: 

Unnecessary

jcstress-core/src/main/java/org/openjdk/jcstress/infra/grading/ReportUtils.java line 310:

> 308:     }
> 309: 
> 310:     public static String getNiceMsTimeDate(long ms) {

I would not use the word `Nice`, what does it mean here? Something like `formatTimeDate` or `showTimeDate` would be fine for me.

jcstress-core/src/main/java/org/openjdk/jcstress/infra/grading/ReportUtils.java line 311:

> 309:         long minutes = TimeUnit.MILLISECONDS.toMinutes(ms);
> 310:         ms -= TimeUnit.MINUTES.toMillis(minutes);
> 311: 

Unnecessary

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

PR Review: https://git.openjdk.org/jcstress/pull/161#pullrequestreview-2705429785
PR Review Comment: https://git.openjdk.org/jcstress/pull/161#discussion_r2007280872
PR Review Comment: https://git.openjdk.org/jcstress/pull/161#discussion_r2007293040
PR Review Comment: https://git.openjdk.org/jcstress/pull/161#discussion_r2007295501
PR Review Comment: https://git.openjdk.org/jcstress/pull/161#discussion_r2007297303
PR Review Comment: https://git.openjdk.org/jcstress/pull/161#discussion_r2007297747
PR Review Comment: https://git.openjdk.org/jcstress/pull/161#discussion_r2007300211
PR Review Comment: https://git.openjdk.org/jcstress/pull/161#discussion_r2007297855