8363932: G1: Better distribute KlassCleaningTask

[tschatzl]

Hi all,

please review this change to parallel klass cleaning to improve performance.

The current implementation only parallelizes cleaning of weak class links, while the main work, cleaning the object tree is single-threaded. Hence in practice, the current mechanism does not scale beyond 2-3 threads.

Cleaning an object graph in an application that loads some jars and instantiates central classes within them, with around 180k classes the current G1 Complete Cleaning task (which executes this code) can take 80ms (with 25 threads).

The suggested change is to walk the object graph by (live) ClassLoaderData klass by klass, fixing only the links of that particular klass.

E.g.

CLD1 has klasses A, B, C, CLD2 has klasses a, b, c and CLD3 has klasses 0, 1, 2, 4;
vertical links are subklass references, while horizontal links are sibling references.

j.l.O
  |
  A - B - c - 3
  |
  0 - 2 - C - 1

CLD 3 is dead. Thread 1 claims CLD 1, Thread 2 claims CLD 2 (and nobody claims CLD3 because it's dead).

So thread 1, when reaching A fixes its subklass link to C, and otherwise does nothing with A. When looking at C, it will remove the link to 1.
Thread 2 will only remove the link to 3 of c.

The result is

j.l.O
  |
  A - B - c
  |
  C

There should be no unnecessary object graph walking.

There is a slight change in printing during unlinking: previously the code, when cleaning subklasses it printed unlinking class (subclass)for every class that has been removed on the way to the next live one. In above case, it would print

unlinking class (subclass): 0
unlinking class (subclass): 2

With the change, to avoid following the subklasses of the graph twice, it prints

unlinking class (subclass): 0
unlinking class (sibling): 0

because the string in brackets is the actual link that is followed. I can revert that change.

With the change "Complete Cleaning" time for 200k classes takes 7.6ms (The test is a bit random on when it does the class unloading).

Testing: tier1-5

Thanks,
Thomas

---

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-8363932: G1: Better distribute KlassCleaningTask (Enhancement - P4)

Reviewers

• Coleen Phillimore (@coleenp - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27316

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back tschatzl! 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]

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

8363932: G1: Better distribute KlassCleaningTask

Reviewed-by: coleenp

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

• c3aaa87: 8361955: [GCC static analyzer] libjdwp/threadControl.c threadControl_setPendingInterrupt error: dereference of NULL 'node'
• 58270b7: 8346839: [TESTBUG] "java/awt/textfield/setechochartest4/setechochartest4.java" failed because the test frame disappears on clicking "Click Several Times" button
• ced3f13: 8367901: Calendar.roll(hour, 24) returns wrong result
• ... and 92 more: https://git.openjdk.org/jdk/compare/c2c44a061a6ba392b4e93eca2c85bd96ab7dcffe...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.

[openjdk]

@tschatzl this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout submit/8363932-klasstaskcleaning-distribution
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[openjdk]

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

• hotspot

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.

[tschatzl]

/label add hotspot-gc
/label add hotspot-runtime
/label remove hotspot

[openjdk]

@tschatzl
The hotspot-gc label was successfully added.

[openjdk]

@tschatzl
The hotspot-runtime label was successfully added.

[openjdk]

@tschatzl
The hotspot label was successfully removed.

[mlbridge]

Webrevs

• 01: Full - Incremental (4c0a9cbb)
• 00: Full (c6e3522d)

[albertnetymk]

Preexisting: These two methods, subklass and next_sibling, sounds like plain getters, but they actually query liveness and skip dead klasses. I wonder whether it's possible to prune dead klasses in one go at some place and turn these two methods into plain getters.

[albertnetymk]

I suggest adding _atomic suffix to the field name.

[openjdk]

@tschatzl hotspot has been added to this pull request based on files touched in new commit(s).

[tschatzl]

/label remove hotspot
/label add hotspot-gc
/label add hotspot-runtime

[openjdk]

@tschatzl
The hotspot label was successfully removed.

[openjdk]

@tschatzl
The hotspot-gc label was successfully added.

[openjdk]

@tschatzl
The hotspot-runtime label was successfully added.

[coleenp]

This looks good to me. I think your explanation makes sense and the example helps very much.