8273092: Sort classlist in JDK image #5288

iklam · 2021-08-27T23:12:52Z

When the classlist is generated using build.tools.classlist.HelloClasslist, its contents may be non-deterministic due to Java thread execution order.

We should sort the generated classlist to make the JDK image's contents more deterministic.

Tested with Mach5 tier1, tier2, builds-tier5

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8273092: Sort classlist in JDK image

Reviewers

Claes Redestad (@cl4es - Reviewer) ⚠️ Review applies to dc170ec
Magnus Ihse Bursie (@magicus - Reviewer) ⚠️ Review applies to dc170ec
Daniel Fuchs (@dfuch - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5288/head:pull/5288
$ git checkout pull/5288

Update a local copy of the PR:
$ git checkout pull/5288
$ git pull https://git.openjdk.java.net/jdk pull/5288/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5288

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5288.diff

bridgekeeper · 2021-08-27T23:13:29Z

👋 Welcome back iklam! 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 · 2021-08-27T23:15:17Z

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

build
core-libs

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 · 2021-08-28T03:05:47Z

Webrevs

01: Full - Incremental (ee710895)
00: Full (dc170ec0)

cl4es

Seems OK.

cl4es · 2021-08-28T13:16:02Z

make/jdk/src/classes/build/tools/classlist/SortClasslist.java

+ * common use.
+ *
+ * The classlist is produced by adding -XX:DumpLoadedClassList=classlist
+ */


Looks like a copy-paste error (also redundant)

openjdk · 2021-08-28T13:19:34Z

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

8273092: Sort classlist in JDK image

Reviewed-by: redestad, ihse, dfuchs

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

9732fbe: 8273153: Consolidate file_exists into os:file_exists
0609421: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null
7fc8540: 8260265: UTF-8 by Default
3204853: 8272343: Remove MetaspaceClosure::FLAG_MASK
fecefb8: 8271302: Regex Test Refresh
f18c0fa: 8271560: sun/security/ssl/DHKeyExchange/LegacyDHEKeyExchange.java still fails due to "An established connection was aborted by the software in your host machine"
5aaa20f: 8272861: Add a micro benchmark for vector api
7a01ba6: 8272093: Extract evacuation failure injection from G1CollectedHeap
98b9d98: 8272797: Mutex with rank safepoint_check_never imply allow_vm_block
f11e099: 8272651: G1 heap region info print order changed by JDK-8269914
... and 40 more: https://git.openjdk.java.net/jdk/compare/0f428ca533627e758cdca599d4589f39c21e27b6...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.

magicus

Looks great. Thank you for helping to make the build deterministic!

dfuch · 2021-08-30T12:51:43Z

make/jdk/src/classes/build/tools/classlist/SortClasslist.java

+        while (scanner.hasNextLine()) {
+            String line = scanner.nextLine();
+            Matcher m = p.matcher(line);
+            if (m.find()) {


Are we sure that a comment line will not match this regexp, or that if it matches, it is not a comment line?

Thanks for the comments. I've swapper the matching order to check for leading # and @ characters first.

dfuch

Thanks Ioi! These changes look good to me.

iklam · 2021-08-31T16:31:29Z

Thanks @cl4es @magicus @dfuch for the review!
/integrate

openjdk · 2021-08-31T16:33:15Z

Going to push as commit 1996f64.
Since your change was applied there have been 57 commits pushed to the master branch:

ba3587e: 8273144: Remove unused top level "Sample Collection Set Candidates" logging
75d987a: 8262095: NPE in Flow$FlowAnalyzer.visitApply: Cannot invoke getThrownTypes because tree.meth.type is null
e551852: 8271225: Add floorDivExact() method to java.lang.[Strict]Math
e671255: 8237567: Refactor G1-specific code in shared VM_CollectForMetadataAllocation
9bc7cc5: 8273033: SerialGC: remove obsolete comments
841e394: 8159979: During initial mark, preparing all regions for marking may take a significant amount of time
98fa533: 8273100: Improve AbstractStringBuilder.append(String) when using CompactStrings
9732fbe: 8273153: Consolidate file_exists into os:file_exists
0609421: 8272347: ObjectMethods::bootstrap should specify NPE if any argument except lookup is null
7fc8540: 8260265: UTF-8 by Default
... and 47 more: https://git.openjdk.java.net/jdk/compare/0f428ca533627e758cdca599d4589f39c21e27b6...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-08-31T16:33:26Z

@iklam Pushed as commit 1996f64.

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

8273092: Sort classlist in JDK image

dc170ec

openjdk bot added build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Aug 27, 2021

iklam marked this pull request as ready for review August 28, 2021 02:59

openjdk bot added the rfr Pull request is ready for review label Aug 28, 2021

cl4es approved these changes Aug 28, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 28, 2021

magicus approved these changes Aug 30, 2021

View reviewed changes

dfuch reviewed Aug 30, 2021

View reviewed changes

@dfuch comments

ee71089

dfuch approved these changes Aug 31, 2021

View reviewed changes

openjdk bot closed this Aug 31, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8273092: Sort classlist in JDK image #5288

8273092: Sort classlist in JDK image #5288

iklam commented Aug 27, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 27, 2021

openjdk bot commented Aug 27, 2021

mlbridge bot commented Aug 28, 2021 •

edited

Loading

cl4es left a comment

cl4es Aug 28, 2021

openjdk bot commented Aug 28, 2021 •

edited

Loading

magicus left a comment

dfuch Aug 30, 2021

iklam Aug 31, 2021

dfuch left a comment

iklam commented Aug 31, 2021

openjdk bot commented Aug 31, 2021

openjdk bot commented Aug 31, 2021

8273092: Sort classlist in JDK image #5288

8273092: Sort classlist in JDK image #5288

Conversation

iklam commented Aug 27, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Aug 27, 2021

openjdk bot commented Aug 27, 2021

mlbridge bot commented Aug 28, 2021 • edited Loading

Webrevs

cl4es left a comment

Choose a reason for hiding this comment

cl4es Aug 28, 2021

Choose a reason for hiding this comment

openjdk bot commented Aug 28, 2021 • edited Loading

magicus left a comment

Choose a reason for hiding this comment

dfuch Aug 30, 2021

Choose a reason for hiding this comment

iklam Aug 31, 2021

Choose a reason for hiding this comment

dfuch left a comment

Choose a reason for hiding this comment

iklam commented Aug 31, 2021

openjdk bot commented Aug 31, 2021

openjdk bot commented Aug 31, 2021

iklam commented Aug 27, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 28, 2021 •

edited

Loading

openjdk bot commented Aug 28, 2021 •

edited

Loading