8318603: Parallelize sun/java2d/marlin/ClipShapeTest.java #17719

mrserb · 2024-02-06T03:10:36Z

The ClipShapeTest test is split into 4 different tests which could be executed in parallel.

The execution time is changed from:

==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR   
   jtreg:test/jdk/sun/java2d/marlin/ClipShapeTest.java
                                                         1     1     0     0   
==============================
TEST SUCCESS

Finished building target 'run-test' in configuration 'macosx-aarch64-server-release'

real	2m58.673s
user	3m8.847s
sys	0m11.845s

to:

==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR   
   jtreg:test/jdk/sun/java2d/marlin/ClipShapeTest.java
                                                         4     4     0     0   
==============================
TEST SUCCESS

Finished building target 'run-test' in configuration 'macosx-aarch64-server-release'

real	1m17.752s
user	3m25.308s
sys	0m12.987s

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-8318603: Parallelize sun/java2d/marlin/ClipShapeTest.java (Enhancement - P4)

Reviewers

Alexey Ivanov (@aivanov-jdk - Reviewer)
Aleksey Shipilev (@shipilev - Reviewer) ⚠️ Review applies to 67e3d2c5

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17719

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-02-06T03:11:04Z

👋 Welcome back serb! 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-06T03:12:34Z

@mrserb The following label will be automatically applied to this pull request:

client

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

mlbridge · 2024-02-06T08:11:26Z

Webrevs

bourgesl · 2024-02-06T10:48:29Z

LGTM, not an official reviewer.

I can check this test to select the minimal needed cases to detect regression (132 combinations for now) and reduce even more test wallclock time.

I suppose 3m25 user time is costly on ci infrastructure anyway if repeated too much.

shipilev · 2024-02-06T11:34:28Z

test/jdk/sun/java2d/marlin/ClipShapeTest.java

+ * @bug 8191814
+ * @summary Verifies that Marlin rendering generates the same
+ * images with and without clipping optimization with all possible
+ * stroke (cap/join) and/or dashes or fill modes (EO rules)
+ * for paths made of either 9 lines, 4 quads, 2 cubics (random)
+ * Note: Use the argument -slow to run more intensive tests (too much time)
+ *


I think our style is only put these in the first @run block, and the rest to only carry the essentials for the tests to run. Simplifies the patch as well.

Especially with such a long description that spans four lines.

aivanov-jdk

Looks good to me except for the minor comment.

Do you mind updating the copyright year?

aivanov-jdk · 2024-02-06T16:41:07Z

test/jdk/sun/java2d/marlin/ClipShapeTest.java

+ * @bug 8191814
+ * @summary Verifies that Marlin rendering generates the same
+ * images with and without clipping optimization with all possible
+ * stroke (cap/join) and/or dashes or fill modes (EO rules)
+ * for paths made of either 9 lines, 4 quads, 2 cubics (random)
+ * Note: Use the argument -slow to run more intensive tests (too much time)
+ *


Especially with such a long description that spans four lines.

openjdk · 2024-02-06T16:52:54Z

@mrserb 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:

8318603: Parallelize sun/java2d/marlin/ClipShapeTest.java

Reviewed-by: aivanov, shade

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 166 new commits pushed to the master branch:

52d4976: 8325437: Safepoint polling in monitor deflation can cause massive logs
8b70b8d: 8325440: Confusing error reported for octal literals with wrong digits
5daf622: 8325309: Amend "Listeners and Threads" in AWTThreadIssues.html
d165d12: 8325510: Serial: Remove redundant arg in non_clean_card_iterate
71b46c3: 8325471: CHeapBitMap(MEMFLAGS flags) constructor misleading use of super-constructor
e3dc6a7: 8314275: Incorrect stepping in switch
cc276ff: 8325516: Shenandoah: Move heap change tracking into ShenandoahHeap
8d9ad97: 8324641: [IR Framework] Add Setup method to provide custom arguments and set fields
b797652: 8322927: Unused code in LIR_Assembler::verify_oop_map
9936aee: 8324824: AArch64: Detect Ampere-1B core and update default options for Ampere CPUs
... and 156 more: https://git.openjdk.org/jdk/compare/c313d451a513eb08de0b295c1ce66d0d849d2374...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

mrserb · 2024-02-08T06:35:59Z

duplicated comments are reworked

bourgesl · 2024-02-08T06:55:57Z

LGTM (comments fixed)

shipilev

All right, this looks good, thanks!

aivanov-jdk · 2024-02-08T11:10:30Z

test/jdk/sun/java2d/marlin/ClipShapeTest.java

+ * clipping optimization with all possible stroke (cap/join) and/or dashes or
+ * fill modes (EO rules) for paths made of either 9 lines, 4 quads, 2 cubics
+ * (random)
+ * Note: Use the argument -slow to run more intensive tests (too much time)


Suggested change

* Note: Use the argument -slow to run more intensive tests (too much time)

* <p>

* Note: Use the argument {@code -slow} to run more intensive tests (too much time)

Adding
will render the note in a new paragraph when viewed in IDE (or on class name hover).

aivanov-jdk · 2024-02-08T11:12:29Z

I ran the test in our CI and I didn't notice an improvement. On the other hand, having separate test ids is more flexible.

mrserb · 2024-02-09T02:12:57Z

I ran the test in our CI and I didn't notice an improvement. On the other hand, having separate test ids is more flexible.

I have retested by the complete jdk_desktop group of tests:
Base:

real    6m57.901s
user    123m45.204s
sys     9m16.735s

Fix:

real    4m23.600s
user    126m35.078s
sys     8m50.104s

So it might depend on the system and used concurrency.

prrace · 2024-02-09T02:17:01Z

I ran the test in our CI and I didn't notice an improvement.

Did you run jdk:tier4 like Sergey was doing ?

Our CI runs some number of (headless) tests in parallel (I am not sure but think it is by default 4) and this might change
the whole dynamics of "which test is run last", compared to other test frameworks.

I don't know if whatever Sergey is using does the same, but they could be quite different in the number of CPUs per VM.

If you ran just the one test and it was no faster (wall-clock time) that suggests the (sub-)tests were not being run in parallel which would be interesting .. and we might want to ask jtreg folks for input.

aivanov-jdk · 2024-02-09T13:22:44Z

I ran the test in our CI and I didn't notice an improvement.

Did you run jdk:tier4 like Sergey was doing ?

No, I ran just this single test. I don't know how Sergey is running the test.

Our CI runs some number of (headless) tests in parallel (I am not sure but think it is by default 4) and this might change the whole dynamics of "which test is run last", compared to other test frameworks.

Yep, I know that headless tests are run in parallel, which makes perfect sense.

I don't know if whatever Sergey is using does the same, but they could be quite different in the number of CPUs per VM.

If you ran just the one test and it was no faster (wall-clock time) that suggests the (sub-)tests were not being run in parallel which would be interesting .. and we might want to ask jtreg folks for input.

Yes, I submitted a job with one test only.

At the same time, I didn't look thoroughly into the logs.

This .java file contains four different tests which can be run in parallel, it could save the overall time; in the worst case, the same four tests are run consecutively. No negative impact, so I support the change.

mrserb · 2024-02-10T01:47:44Z

/integrate

openjdk · 2024-02-10T01:48:49Z

Going to push as commit 6c7029f.
Since your change was applied there have been 179 commits pushed to the master branch:

e33d8a2: 8311076: RedefineClasses doesn't check for ConstantPool overflow
6303c0e: 8325569: ProblemList gc/parallel/TestAlwaysPreTouchBehavior.java on linux
3ebe6c1: 8319578: Few java/lang/instrument ignore test.java.opts and accept test.vm.opts only
d39b7ba: 8316460: 4 javax/management tests ignore VM flags
ac4607e: 8226919: attach in linux hangs due to permission denied accessing /proc/pid/root
b42b888: 8325038: runtime/cds/appcds/ProhibitedPackage.java can fail with UseLargePages
6944537: 8325203: System.exit(0) kills the launched 3rd party application
4368437: 8325264: two compiler/intrinsics/float16 tests fail after JDK-8324724
4a3a38d: 8325517: Shenandoah: Reduce unnecessary includes from shenandoahControlThread.cpp
40708ba: 8325563: Remove unused Space::is_in
... and 169 more: https://git.openjdk.org/jdk/compare/c313d451a513eb08de0b295c1ce66d0d849d2374...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-02-10T01:48:55Z

@mrserb Pushed as commit 6c7029f.

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

8318603: Parallelize sun/java2d/marlin/ClipShapeTest.java

333096a

openjdk bot added the client client-libs-dev@openjdk.org label Feb 6, 2024

mrserb marked this pull request as ready for review February 6, 2024 08:08

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

shipilev reviewed Feb 6, 2024

View reviewed changes

aivanov-jdk approved these changes Feb 6, 2024

View reviewed changes

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

Update ClipShapeTest.java

67e3d2c

shipilev approved these changes Feb 8, 2024

View reviewed changes

aivanov-jdk approved these changes Feb 8, 2024

View reviewed changes

Update ClipShapeTest.java

423a2e6

aivanov-jdk approved these changes Feb 9, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Feb 10, 2024

openjdk bot closed this Feb 10, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 10, 2024

mrserb deleted the JDK-8318603 branch February 10, 2024 01:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8318603: Parallelize sun/java2d/marlin/ClipShapeTest.java #17719

8318603: Parallelize sun/java2d/marlin/ClipShapeTest.java #17719

mrserb commented Feb 6, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 6, 2024

openjdk bot commented Feb 6, 2024

mlbridge bot commented Feb 6, 2024 •

edited

Loading

bourgesl commented Feb 6, 2024 •

edited

Loading

shipilev Feb 6, 2024

aivanov-jdk Feb 6, 2024

aivanov-jdk left a comment

aivanov-jdk Feb 6, 2024

openjdk bot commented Feb 6, 2024 •

edited

Loading

mrserb commented Feb 8, 2024

bourgesl commented Feb 8, 2024

shipilev left a comment

aivanov-jdk Feb 8, 2024

aivanov-jdk commented Feb 8, 2024

mrserb commented Feb 9, 2024

prrace commented Feb 9, 2024

aivanov-jdk commented Feb 9, 2024

mrserb commented Feb 10, 2024

openjdk bot commented Feb 10, 2024

openjdk bot commented Feb 10, 2024

	* Note: Use the argument -slow to run more intensive tests (too much time)
	* <p>
	* Note: Use the argument {@code -slow} to run more intensive tests (too much time)

8318603: Parallelize sun/java2d/marlin/ClipShapeTest.java #17719

8318603: Parallelize sun/java2d/marlin/ClipShapeTest.java #17719

Conversation

mrserb commented Feb 6, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Feb 6, 2024

openjdk bot commented Feb 6, 2024

mlbridge bot commented Feb 6, 2024 • edited Loading

Webrevs

bourgesl commented Feb 6, 2024 • edited Loading

shipilev Feb 6, 2024

Choose a reason for hiding this comment

aivanov-jdk Feb 6, 2024

Choose a reason for hiding this comment

aivanov-jdk left a comment

Choose a reason for hiding this comment

aivanov-jdk Feb 6, 2024

Choose a reason for hiding this comment

openjdk bot commented Feb 6, 2024 • edited Loading

mrserb commented Feb 8, 2024

bourgesl commented Feb 8, 2024

shipilev left a comment

Choose a reason for hiding this comment

aivanov-jdk Feb 8, 2024

Choose a reason for hiding this comment

aivanov-jdk commented Feb 8, 2024

mrserb commented Feb 9, 2024

prrace commented Feb 9, 2024

aivanov-jdk commented Feb 9, 2024

mrserb commented Feb 10, 2024

openjdk bot commented Feb 10, 2024

openjdk bot commented Feb 10, 2024

mrserb commented Feb 6, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 6, 2024 •

edited

Loading

bourgesl commented Feb 6, 2024 •

edited

Loading

openjdk bot commented Feb 6, 2024 •

edited

Loading