8279856: Parallel: Use PreservedMarks to record promotion-failed objects #7028

albertnetymk · 2022-01-11T14:02:20Z

Simple change of using PreservedMarks to skip young-gen walking in finding promotion-failed objs.

Test: tier1-6

Progress

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

Issue

JDK-8279856: Parallel: Use PreservedMarks to record promotion-failed objects

Reviewers

Stefan Johansson (@kstefanj - Reviewer)
Thomas Schatzl (@tschatzl - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7028

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

Using diff file

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

bridgekeeper · 2022-01-11T14:02:26Z

👋 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 · 2022-01-11T14:04:35Z

@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 · 2022-01-11T14:07:48Z

Webrevs

01: Full - Incremental (aee7f1b7)
00: Full (37979a3b)

kstefanj

Looks good.

One think that would be interesting would be to do some measurements on the additional memory usage for the PreservedMarks stack with this change. To better understand what kind of memory trade-off we do for the quicker processing.

kstefanj · 2022-01-17T09:36:13Z

src/hotspot/share/gc/parallel/psPromotionManager.cpp

@@ -360,7 +360,7 @@ oop PSPromotionManager::oop_promotion_failed(oop obj, markWord obj_mark) {

    push_contents(obj);

-    _preserved_marks->push_if_necessary(obj, obj_mark);
+    _preserved_marks->push_always(obj, obj_mark);


I think a comment here explaining that we use the preserved marks no only to preserved "special" marks but also to restore and clear the forwardning pointer in "normal" marks.

kstefanj · 2022-01-17T09:37:37Z

src/hotspot/share/gc/shared/preservedMarks.hpp

@@ -57,6 +57,7 @@ class PreservedMarks {
 public:
  size_t size() const { return _stack.size(); }
  inline void push_if_necessary(oop obj, markWord m);
+  inline void push_always(oop obj, markWord m);


Any reason not to just call this push() like we had in the past and maybe also re-use it in push_if_necessary()?

I was thinking push_always makes a clearer contrast with its neighbor: the two are parallel and there's no interconnection/dependency btw them.

Sure, but they do push elements the same way so it could be shared to avoid having to change two places if anything needs updating in the future. That said, the code is small and live close together so should be fine if you prefer it this way.

I think it's better not to mix the two APIs. The duplication is very small after all.

kstefanj · 2022-01-17T11:18:54Z

src/hotspot/share/gc/parallel/psScavenge.cpp

-  RemoveForwardedPointerClosure remove_fwd_ptr_closure;
-  young_gen->object_iterate(&remove_fwd_ptr_closure);
-


Since these are removed, there is no longer any need to get heap and young_gen above either. I think the cleanest would be do directly use ParallelScavengeHeap::heap() in:

NOT_PRODUCT(heap->reset_promotion_should_fail();)

tschatzl · 2022-01-17T11:27:09Z

One think that would be interesting would be to do some measurements on the additional memory usage for the PreservedMarks stack with this change. To better understand what kind of memory trade-off we do for the quicker processing.

It might actually be nice to have some log message for that memory usage somewhere; while I know there is no "framework" for that in Parallel/Serial, it would still be nice to have.

openjdk · 2022-01-31T16:06:18Z

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

8279856: Parallel: Use PreservedMarks to record promotion-failed objects

Reviewed-by: sjohanss, tschatzl

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

993a248: 8280450: Add task queue printing to STW Full GCs
dcc666d: 8280139: Report more detailed statistics about task stealing in task queue stats
bdda43e: 8280705: Parallel: Full gc mark stack draining should prefer to make work available to other threads
091aff9: 8278908: [macOS] Unexpected text normalization on pasting from clipboard
61794c5: 8280817: Clean up and unify empty VM operations
c6ed204: 8278263: Remove redundant synchronized from URLStreamHandler.openConnection methods
251351f: 8280889: java/lang/instrument/GetObjectSizeIntrinsicsTest.java fails with -XX:-UseCompressedOops
be9f984: 8280553: resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java can fail if GC occurs
268880b: 8277412: Use String.isBlank to simplify code in sun.net.www.protocol.mailto.Handler
d366d15: 8280903: javadoc build fails after JDK-4774868
... and 279 more: https://git.openjdk.java.net/jdk/compare/3121898c33fa3cc5a049977f8677105a84c3e50c...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.

tschatzl

Lgtm.

albertnetymk · 2022-02-01T08:45:32Z

Thanks for the review.

/integrate

openjdk · 2022-02-01T08:47:17Z

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

de3113b: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
0e70d45: 8280950: RandomGenerator:NextDouble() default behavior non conformant after JDK-8280550 fix
1ea0146: 8281007: Test jdk/javadoc/doclet/checkStylesheetClasses/CheckStylesheetClasses.java fails after JDK-8280738
9c0104b: 8221642: AccessibleObject::setAccessible throws NPE when invoked by JNI code with no java frame on stack
4dbebb6: 8280534: Enable compile-time doclint reference checking
4191b2b: 8275337: C1: assert(false) failed: live_in set of first block must be empty
96d0df7: 8272984: javadoc support for reproducible builds
ee3be0b: 8280488: doclint reference checks withstand warning suppression
74921e8: 8280738: Minor cleanup for HtmlStyle
3916561: 8280543: Update the "java" and "jcmd" tool specification for CDS
... and 291 more: https://git.openjdk.java.net/jdk/compare/3121898c33fa3cc5a049977f8677105a84c3e50c...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-02-01T08:47:28Z

@albertnetymk Pushed as commit 16ec47d.

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

parallel-promotion-failure

37979a3

openjdk bot added the rfr Pull request is ready for review label Jan 11, 2022

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Jan 11, 2022

kstefanj suggested changes Jan 17, 2022

View reviewed changes

kstefanj reviewed Jan 17, 2022

View reviewed changes

albertnetymk added 2 commits January 25, 2022 16:03

review

07aa8c7

review

aee7f1b

kstefanj approved these changes Jan 31, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 31, 2022

tschatzl approved these changes Jan 31, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Feb 1, 2022

openjdk bot closed this Feb 1, 2022

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

albertnetymk deleted the parallel-promotion-failure branch February 1, 2022 08:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8279856: Parallel: Use PreservedMarks to record promotion-failed objects #7028

8279856: Parallel: Use PreservedMarks to record promotion-failed objects #7028

albertnetymk commented Jan 11, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 11, 2022

openjdk bot commented Jan 11, 2022

mlbridge bot commented Jan 11, 2022 •

edited

Loading

kstefanj left a comment

kstefanj Jan 17, 2022

kstefanj Jan 17, 2022

albertnetymk Jan 25, 2022

kstefanj Jan 26, 2022

albertnetymk Jan 26, 2022

kstefanj Jan 17, 2022

tschatzl commented Jan 17, 2022

openjdk bot commented Jan 31, 2022 •

edited

Loading

tschatzl left a comment

albertnetymk commented Feb 1, 2022

openjdk bot commented Feb 1, 2022

openjdk bot commented Feb 1, 2022

		RemoveForwardedPointerClosure remove_fwd_ptr_closure;
		young_gen->object_iterate(&remove_fwd_ptr_closure);

8279856: Parallel: Use PreservedMarks to record promotion-failed objects #7028

8279856: Parallel: Use PreservedMarks to record promotion-failed objects #7028

Conversation

albertnetymk commented Jan 11, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 11, 2022

openjdk bot commented Jan 11, 2022

mlbridge bot commented Jan 11, 2022 • edited Loading

Webrevs

kstefanj left a comment

Choose a reason for hiding this comment

kstefanj Jan 17, 2022

Choose a reason for hiding this comment

kstefanj Jan 17, 2022

Choose a reason for hiding this comment

albertnetymk Jan 25, 2022

Choose a reason for hiding this comment

kstefanj Jan 26, 2022

Choose a reason for hiding this comment

albertnetymk Jan 26, 2022

Choose a reason for hiding this comment

kstefanj Jan 17, 2022

Choose a reason for hiding this comment

tschatzl commented Jan 17, 2022

openjdk bot commented Jan 31, 2022 • edited Loading

tschatzl left a comment

Choose a reason for hiding this comment

albertnetymk commented Feb 1, 2022

openjdk bot commented Feb 1, 2022

openjdk bot commented Feb 1, 2022

albertnetymk commented Jan 11, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 11, 2022 •

edited

Loading

openjdk bot commented Jan 31, 2022 •

edited

Loading