8326718: Test java/util/Formatter/Padding.java should timeout on large inputs before fix in JDK-8299677 #18033

chadrako · 2024-02-27T17:24:03Z

JDK-8299677 fixes a bug with Formatter.format taking a long time when there is a lot of padding. This test runs Formatter.format with very large padding. Test fails before JDK-8299677 and passes after.

Timeout for the test was set to 10 seconds. Test passes locally with as low as 1 (after JDK-8299677) and fails as high as 120 (before JDK-8299677) so it should be consistent.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8326718: Test java/util/Formatter/Padding.java should timeout on large inputs before fix in JDK-8299677 (Enhancement - P4)

Reviewers

Raffaello Giulietti (@rgiulietti - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18033/head:pull/18033
$ git checkout pull/18033

Update a local copy of the PR:
$ git checkout pull/18033
$ git pull https://git.openjdk.org/jdk.git pull/18033/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18033

View PR using the GUI difftool:
$ git pr show -t 18033

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18033.diff

Webrev

Link to Webrev Comment

…rge inputs before JDK-8299677

bridgekeeper · 2024-02-27T17:24:52Z

👋 Welcome back chadrako! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-02-27T17:26:05Z

@chadrako The following labels will be automatically applied to this pull request:

core-libs
i18n

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-02-27T17:29:15Z

Webrevs

rgiulietti · 2024-02-27T19:04:27Z

test/jdk/java/util/Formatter/Padding.java

@@ -26,7 +26,7 @@
 * @bug 4906370


Suggested change

* @bug 4906370

* @bug 4906370 8299677 8326718

or maybe just

Suggested change

* @bug 4906370

* @bug 4906370 8326718

AlanBateman · 2024-02-27T19:13:10Z

test/jdk/java/util/Formatter/Padding.java

@@ -26,7 +26,7 @@
 * @bug 4906370
 * @summary Tests to excercise padding on int and double values,
 *      with various flag combinations.
- * @run junit Padding
+ * @run junit/timeout=10 Padding


/timeout=10 looks problematic, I don't think you want that.

What is the issue with this? Is there a different way to set a timeout? The test tests that format does not take a long time to run so I would like to have a timeout

What is the issue with this? Is there a different way to set a timeout? The test tests that format does not take a long time to run so I would like to have a timeout

Hard-coded timeout in a test are generally considered harmful as the test suite is run on a wide variety of systems, a single value could be too large for fast systems and too small for slow ones. The jtreg harness has an overall -timeout:N factor which can scale up or down all the timeouts of individual tests. I don't know offhand if there is an existing idiom to do this with junit tests from within jtreg.

Removed that timeout. Test fails before patch even when timeout factor is 10. Passes after patch within seconds. Default timeout should be good

rgiulietti · 2024-02-27T19:20:36Z

I guess the title should read
"8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs after JDK-8299677"

chadrako · 2024-02-27T19:25:19Z

I guess the title should read "8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs after JDK-8299677"

The test should timeout/fail before the fix in JDK-8299677 and pass after. I could update the title to be
"8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs before fix in JDK-8299677"
But I don't think after is correct

rgiulietti · 2024-02-27T19:32:44Z

"8326718: Test java/util/Formatter/Padding.java does timeout on large inputs before fix in JDK-8299677"
or
"8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs after JDK-8299677"

To me,
"8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs before fix in JDK-8299677"
as you propose, sounds like the fix JDK-8299677 didn't serve its intended purpose.

chadrako · 2024-02-27T19:46:27Z

The fix in JDK-8299677 serves it's intended purpose but the test added with it does not test that. The original test does not timeout before or after the fix which is the issue.

"8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs after JDK-8299677"
This is the expected case. Should the title be what the issue is or what the fix is? To me this sounds like the test should be timing out or was timing out after JDK-8299677

Maybe a better title is
"8326718: Test java/util/Formatter/Padding.java should timeout on large inputs before fix in JDK-8299677"

jddarcy · 2024-02-27T19:53:23Z

test/jdk/java/util/Formatter/Padding.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.


Copyright nit: per OpenJDK conventions, the new copyright for the updated file should be "2023, 2024," not just "2024".

Suggested change

* Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.

* Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights reserved.

with a , after the 2nd year as well. Otherwise a check will fail, as I learned the hard-way ;-)

rgiulietti · 2024-02-27T20:00:26Z

@chadrako The title of the issue should succinctly describe the problem at the time it is filed.

chadrako · 2024-02-27T20:37:03Z

@chadrako The title of the issue should succinctly describe the problem at the time it is filed.

Then I feel like the current title is correct. The issue at the time of filing is that Padding.java does not timeout on large inputs before the fix (which is should but doesn't) that was implemented in JDK-8299677

I'm open to other's opinions on this as well

rgiulietti · 2024-02-28T14:44:17Z

@chadrako OK, I now get what you mean with the description in the title.

Although it should be noted that the tests were introduced with JDK-8299677 and didn't exercise the large widths in the format specifications, so they could not timeout neither before nor after the fix as they were not tested.

So maybe yes, your second proposal

Maybe a better title is
"8326718: Test java/util/Formatter/Padding.java should timeout on large inputs before fix in JDK-8299677"

sounds better.

rgiulietti · 2024-03-01T16:44:40Z

test/jdk/java/util/Formatter/Padding.java

+    private static final String tenMillionZeros = "0".repeat(10000000);
+    private static final String tenMillionBlanks = " ".repeat(10000000);


Just a nit to help readability

Suggested change

private static final String tenMillionZeros = "0".repeat(10000000);

private static final String tenMillionBlanks = " ".repeat(10000000);

private static final String tenMillionZeros = "0".repeat(10_000_000);

private static final String tenMillionBlanks = " ".repeat(10_000_000);

openjdk · 2024-03-06T15:57:28Z

@chadrako This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8326718: Test java/util/Formatter/Padding.java should timeout on large inputs before fix in JDK-8299677

Reviewed-by: rgiulietti

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 120 new commits pushed to the master branch:

e92ecd9: 8326983: Unused operands reported after JDK-8326135
9f70940: 8327261: Parsing test for Double/Float succeeds w/o testing all bad cases
08b03a3: 8327376: Parallel: Remove unimplemented methods in psParallelCompact.hpp
ae5e3fd: 8327365: Inline and remove GCStats
326c91e: 8327288: Obsolete unused ProcessDistributionStride product option
a7461de: 8325255: jdk.internal.util.ReferencedKeySet::add using wrong test
2bdd387: 8327286: Obsolete unused NUMAPageScanRate product option
992104d: 8327239: Obsolete unused GCLockerEdenExpansionPercent product option
fbb422e: 8327201: C2: Uninitialized VLoop::_pre_loop_end after JDK-8324890
7d53559: 8319932: [JVMCI] class unloading related tests can fail on libgraal
... and 110 more: https://git.openjdk.org/jdk/compare/93feda3d9a1807422c7f47703358aabd2e8639b8...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@rgiulietti) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

chadrako · 2024-03-06T19:21:37Z

/integrate

openjdk · 2024-03-06T19:21:52Z

@chadrako
Your change (at version 2e24a45) is now ready to be sponsored by a Committer.

rgiulietti · 2024-03-06T19:54:52Z

/sponsor

openjdk · 2024-03-06T19:55:30Z

Going to push as commit 4f33608.
Since your change was applied there have been 120 commits pushed to the master branch:

e92ecd9: 8326983: Unused operands reported after JDK-8326135
9f70940: 8327261: Parsing test for Double/Float succeeds w/o testing all bad cases
08b03a3: 8327376: Parallel: Remove unimplemented methods in psParallelCompact.hpp
ae5e3fd: 8327365: Inline and remove GCStats
326c91e: 8327288: Obsolete unused ProcessDistributionStride product option
a7461de: 8325255: jdk.internal.util.ReferencedKeySet::add using wrong test
2bdd387: 8327286: Obsolete unused NUMAPageScanRate product option
992104d: 8327239: Obsolete unused GCLockerEdenExpansionPercent product option
fbb422e: 8327201: C2: Uninitialized VLoop::_pre_loop_end after JDK-8324890
7d53559: 8319932: [JVMCI] class unloading related tests can fail on libgraal
... and 110 more: https://git.openjdk.org/jdk/compare/93feda3d9a1807422c7f47703358aabd2e8639b8...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-03-06T19:55:37Z

@rgiulietti @chadrako Pushed as commit 4f33608.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8326718: Test java/util/Formatter/Padding.java does not timeout on la…

b3050e1

…rge inputs before JDK-8299677

openjdk bot added the rfr Pull request is ready for review label Feb 27, 2024

openjdk bot added core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org labels Feb 27, 2024

rgiulietti reviewed Feb 27, 2024

View reviewed changes

AlanBateman reviewed Feb 27, 2024

View reviewed changes

jddarcy reviewed Feb 27, 2024

View reviewed changes

Test updates

45d0acd

chadrako changed the title ~~8326718: Test java/util/Formatter/Padding.java does not timeout on large inputs before JDK-8299677~~ 8326718: Test java/util/Formatter/Padding.java should timeout on large inputs before fix in JDK-8299677 Feb 28, 2024

rgiulietti reviewed Mar 2, 2024

View reviewed changes

Add underscores to large number

2e24a45

rgiulietti approved these changes Mar 6, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 6, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 6, 2024

openjdk bot added the integrated Pull request has been integrated label Mar 6, 2024

openjdk bot closed this Mar 6, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Mar 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8326718: Test java/util/Formatter/Padding.java should timeout on large inputs before fix in JDK-8299677 #18033

8326718: Test java/util/Formatter/Padding.java should timeout on large inputs before fix in JDK-8299677 #18033

chadrako commented Feb 27, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 27, 2024

openjdk bot commented Feb 27, 2024

mlbridge bot commented Feb 27, 2024 •

edited

Loading

rgiulietti Feb 27, 2024

AlanBateman Feb 27, 2024

chadrako Feb 27, 2024

jddarcy Feb 27, 2024

chadrako Feb 27, 2024

rgiulietti commented Feb 27, 2024

chadrako commented Feb 27, 2024 •

edited

Loading

rgiulietti commented Feb 27, 2024

chadrako commented Feb 27, 2024

jddarcy Feb 27, 2024 •

edited

Loading

rgiulietti Feb 27, 2024

rgiulietti commented Feb 27, 2024

chadrako commented Feb 27, 2024

rgiulietti commented Feb 28, 2024

rgiulietti Mar 1, 2024

openjdk bot commented Mar 6, 2024 •

edited

Loading

chadrako commented Mar 6, 2024

openjdk bot commented Mar 6, 2024

rgiulietti commented Mar 6, 2024

openjdk bot commented Mar 6, 2024

openjdk bot commented Mar 6, 2024

	* Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
	* Copyright (c) 2023, 2024, Oracle and/or its affiliates. All rights reserved.

		private static final String tenMillionZeros = "0".repeat(10000000);
		private static final String tenMillionBlanks = " ".repeat(10000000);

8326718: Test java/util/Formatter/Padding.java should timeout on large inputs before fix in JDK-8299677 #18033

8326718: Test java/util/Formatter/Padding.java should timeout on large inputs before fix in JDK-8299677 #18033

Conversation

chadrako commented Feb 27, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Feb 27, 2024

openjdk bot commented Feb 27, 2024

mlbridge bot commented Feb 27, 2024 • edited Loading

Webrevs

rgiulietti Feb 27, 2024

Choose a reason for hiding this comment

AlanBateman Feb 27, 2024

Choose a reason for hiding this comment

chadrako Feb 27, 2024

Choose a reason for hiding this comment

jddarcy Feb 27, 2024

Choose a reason for hiding this comment

chadrako Feb 27, 2024

Choose a reason for hiding this comment

rgiulietti commented Feb 27, 2024

chadrako commented Feb 27, 2024 • edited Loading

rgiulietti commented Feb 27, 2024

chadrako commented Feb 27, 2024

jddarcy Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

rgiulietti Feb 27, 2024

Choose a reason for hiding this comment

rgiulietti commented Feb 27, 2024

chadrako commented Feb 27, 2024

rgiulietti commented Feb 28, 2024

rgiulietti Mar 1, 2024

Choose a reason for hiding this comment

openjdk bot commented Mar 6, 2024 • edited Loading

chadrako commented Mar 6, 2024

openjdk bot commented Mar 6, 2024

rgiulietti commented Mar 6, 2024

openjdk bot commented Mar 6, 2024

openjdk bot commented Mar 6, 2024

chadrako commented Feb 27, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 27, 2024 •

edited

Loading

chadrako commented Feb 27, 2024 •

edited

Loading

jddarcy Feb 27, 2024 •

edited

Loading

openjdk bot commented Mar 6, 2024 •

edited

Loading