8364516: Serial: Move class unloading logic inside SerialFullGC::invoke_at_safepoint #26588

albertnetymk · 2025-08-01T08:47:57Z

Encapsulating classunloading logic inside invoke_at_safepoint and adding doc explaining why the classunloading logic can't be placed at the end of marking during full-gc, like other STW collectors.

Test: tier1-3

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-8364516: Serial: Move class unloading logic inside SerialFullGC::invoke_at_safepoint (Enhancement - P4)

Reviewers

Thomas Schatzl (@tschatzl - Reviewer)
Sangheon Kim (@sangheon - Reviewer) Review applies to f41fb5ef

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26588

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-08-01T08:48:35Z

👋 Welcome back ayang! 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 · 2025-08-01T08:49:10Z

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

8364516: Serial: Move class unloading logic inside SerialFullGC::invoke_at_safepoint

Reviewed-by: tschatzl, sangheki

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

68a4396: 8364316: JFR: Incorrect validation of mirror fields
da0d959: 8364190: JFR: RemoteRecordingStream withers don't work
b96b9c3: 8364461: JFR: Default constructor may not be first in setting control
... and 5 more: https://git.openjdk.org/jdk/compare/57553ca1dbc63e329116bc11764816a4c5ccb297...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 · 2025-08-01T08:49:48Z

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

hotspot-gc

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 · 2025-08-01T08:53:06Z

Webrevs

tschatzl

Minor suggestions to the comment.

tschatzl · 2025-08-01T09:21:51Z

src/hotspot/share/gc/serial/serialFullGC.cpp

@@ -705,6 +705,16 @@ void SerialFullGC::invoke_at_safepoint(bool clear_all_softrefs) {

  allocate_stacks();

+  // Usually, all classunloading works occurs at the end of phase 1, but Serial


Suggested change

// Usually, all classunloading works occurs at the end of phase 1, but Serial

// Usually, all class unloading works occurs at the end of phase 1, but Serial

tschatzl · 2025-08-01T09:23:06Z

src/hotspot/share/gc/serial/serialFullGC.cpp

+  // Usually, all classunloading works occurs at the end of phase 1, but Serial
+  // full-gc accesses dead-objs' klass to find out the start of next live-obj
+  // during phase 2. This requires klasses of dead-objs to be kept loaded.
+  // Therefore, we declare classunloading context in the same level as various


Suggested change

// Therefore, we declare classunloading context in the same level as various

// Therefore, we declare the class unloading context at the same level as the other

or

Suggested change

// Therefore, we declare classunloading context in the same level as various

// Therefore, we declare the ClassUnloadingContext at the same level as the other

tschatzl · 2025-08-01T09:24:30Z

src/hotspot/share/gc/serial/serialFullGC.cpp

+  // phases, and purge dead classes (invoking ClassLoaderDataGraph::purge)
+  // after all phases of full-gc.


Suggested change

// phases, and purge dead classes (invoking ClassLoaderDataGraph::purge)

// after all phases of full-gc.

// phases, and purge dead classes (invoking ClassLoaderDataGraph::purge())

// after the other phases of full-gc.

Not sure why "full gc" has a hyphen in the middle but I guess that's done elsewhere too.

tschatzl · 2025-08-01T09:26:06Z

src/hotspot/share/gc/serial/serialFullGC.cpp

@@ -755,6 +765,13 @@ void SerialFullGC::invoke_at_safepoint(bool clear_all_softrefs) {
    compacter.phase4_compact();
  }

+  // Delete metaspaces for unloaded class loaders and clean up loader_data graph


Suggested change

// Delete metaspaces for unloaded class loaders and clean up loader_data graph

// Delete metaspaces for unloaded class loaders and clean up the CLDG.

or

Suggested change

// Delete metaspaces for unloaded class loaders and clean up loader_data graph

// Delete metaspaces for unloaded class loaders and clean up the class loader data graph.

albertnetymk · 2025-08-01T09:46:15Z

Thank you for the feedback; applied all except this one -- "the other" phases probably don't make much sense, because this comment precedes all full-gc phases; there is no "current" phase.

sangheon

Looks good as is.

There are not aligned lines at 714, 715.

tschatzl

Sorry, one more typo I apparently forgot last time.

tschatzl · 2025-08-04T08:04:55Z

src/hotspot/share/gc/serial/serialFullGC.cpp

@@ -705,6 +705,16 @@ void SerialFullGC::invoke_at_safepoint(bool clear_all_softrefs) {

  allocate_stacks();

+  // Usually, all class unloading works occurs at the end of phase 1, but Serial


Suggested change

// Usually, all class unloading works occurs at the end of phase 1, but Serial

// Usually, all class unloading work occurs at the end of phase 1, but Serial

tschatzl

Ship it!

albertnetymk · 2025-08-04T12:58:13Z

Thanks for review.

/integrate

openjdk · 2025-08-04T12:59:29Z

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

a9f3d3a: 8364177: JDK fails to build due to undefined symbol in libpng on LoongArch64
500462f: 8364580: Test compiler/vectorization/TestSubwordTruncation.java fails on platforms without RoundF/RoundD
68a4396: 8364316: JFR: Incorrect validation of mirror fields
... and 7 more: https://git.openjdk.org/jdk/compare/57553ca1dbc63e329116bc11764816a4c5ccb297...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-08-04T12:59:41Z

@albertnetymk Pushed as commit fc47555.

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

sgc-unloading-doc

f32132a

openjdk bot changed the title ~~8364516~~ 8364516: Serial: Move class unloading logic inside SerialFullGC::invoke_at_safepoint Aug 1, 2025

openjdk bot added the rfr Pull request is ready for review label Aug 1, 2025

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Aug 1, 2025

tschatzl suggested changes Aug 1, 2025

View reviewed changes

review

f41fb5e

sangheon approved these changes Aug 1, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 1, 2025

review

88fe17c

openjdk bot removed the ready Pull request is ready to be integrated label Aug 1, 2025

tschatzl suggested changes Aug 4, 2025

View reviewed changes

albertnetymk added 2 commits August 4, 2025 10:10

review

d0ed5e1

Merge branch 'master' into sgc-unloading-doc

a912850

tschatzl approved these changes Aug 4, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 4, 2025

openjdk bot added the integrated Pull request has been integrated label Aug 4, 2025

openjdk bot closed this Aug 4, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 4, 2025

albertnetymk deleted the sgc-unloading-doc branch August 4, 2025 13:00

		@@ -705,6 +705,16 @@ void SerialFullGC::invoke_at_safepoint(bool clear_all_softrefs) {

		allocate_stacks();

		// Usually, all classunloading works occurs at the end of phase 1, but Serial

	// Usually, all classunloading works occurs at the end of phase 1, but Serial
	// Usually, all class unloading works occurs at the end of phase 1, but Serial

	// Therefore, we declare classunloading context in the same level as various
	// Therefore, we declare the class unloading context at the same level as the other

	// Therefore, we declare classunloading context in the same level as various
	// Therefore, we declare the ClassUnloadingContext at the same level as the other

		// phases, and purge dead classes (invoking ClassLoaderDataGraph::purge)
		// after all phases of full-gc.

	// Delete metaspaces for unloaded class loaders and clean up loader_data graph
	// Delete metaspaces for unloaded class loaders and clean up the CLDG.

	// Usually, all class unloading works occurs at the end of phase 1, but Serial
	// Usually, all class unloading work occurs at the end of phase 1, but Serial

8364516: Serial: Move class unloading logic inside SerialFullGC::invoke_at_safepoint #26588

8364516: Serial: Move class unloading logic inside SerialFullGC::invoke_at_safepoint #26588

Uh oh!

Conversation

albertnetymk commented Aug 1, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Aug 1, 2025

Uh oh!

openjdk bot commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Aug 1, 2025

Uh oh!

mlbridge bot commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

tschatzl left a comment

Choose a reason for hiding this comment

Uh oh!

tschatzl Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

tschatzl Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

tschatzl Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

tschatzl Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

tschatzl Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

albertnetymk commented Aug 1, 2025

Uh oh!

sangheon left a comment

Choose a reason for hiding this comment

Uh oh!

tschatzl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tschatzl Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

tschatzl left a comment

Choose a reason for hiding this comment

Uh oh!

albertnetymk commented Aug 4, 2025

Uh oh!

openjdk bot commented Aug 4, 2025

Uh oh!

openjdk bot commented Aug 4, 2025

Uh oh!

Uh oh!

albertnetymk commented Aug 1, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Aug 1, 2025 •

edited

Loading

mlbridge bot commented Aug 1, 2025 •

edited

Loading

tschatzl left a comment •

edited

Loading