Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8278207: G1: Tighten verification in G1ResetSkipCompactingClosure #6698

Closed

Conversation

albertnetymk
Copy link
Member

@albertnetymk albertnetymk commented Dec 3, 2021

Simple change of expanding the verification logic to mirror its source.

Test: tier1-5


Progress

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

Issue

  • JDK-8278207: G1: Tighten verification in G1ResetSkipCompactingClosure

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6698

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 3, 2021

👋 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 openjdk bot added the rfr label Dec 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 3, 2021

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

@openjdk openjdk bot added the hotspot-gc label Dec 3, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 3, 2021

Webrevs

#ifdef ASSERT
if (r->is_humongous()) {
oop obj = cast_to_oop(r->humongous_start_region()->bottom());
assert(_collector->mark_bitmap()->is_marked(obj), "must be live");
} else if (r->is_open_archive()) {
bool is_empty = (_collector->live_words(r->hrm_index()) == 0);
assert(!is_empty, "should contain at least one live obj");
} else if (r->is_closed_archive()) {
// nothing to assert
} else {
assert(_collector->live_words(region_index) > _collector->scope()->region_compaction_threshold(),
"should be quite full");
}
#endif
Copy link
Contributor

@kstefanj kstefanj Dec 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about extracting this to a function. Something like:

void assert_should_reset(HeapRegion*) PRODUCT_RETURN;

But you might be able to come up with a better name :)

Copy link
Member Author

@albertnetymk albertnetymk Dec 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering this method is fairly short, I think having those asserts inlined doesn't hinder readability; instead, provides some documentation concerning in what context this method is invoked.

Copy link
Contributor

@kstefanj kstefanj Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the fact that the method is short a good reason to extract the asserts, now the bulk of the method is asserts. If we want more documentation I would rather see a comment explaining in what situations regions might be marked as "skip compacting". We do have some documentation on this in G1FullGCHeapRegionAttr.

I'm fine with leaving the asserts here if you and others prefer it. One other question, can we ever end up here with a closed archive region? Those are marked SkipMarking so I guess they should be filtered out above?

Copy link
Member Author

@albertnetymk albertnetymk Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want more documentation I would rather see a comment explaining in what situations regions might be marked as "skip compacting".

These asserts serve as "runnable" documentation -- less likely to deviate from the actual code.

One other question, can we ever end up here with a closed archive region? Those are marked SkipMarking so I guess they should be filtered out above?

Yes; you are right. I should put ShouldNotReachHere() for closed-archive regions.

Copy link
Contributor

@tschatzl tschatzl left a comment

I would probably also have preferred to move this lengthy assert into an extra method, but lgtm.

@openjdk
Copy link

@openjdk openjdk bot commented Dec 13, 2021

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

8278207: G1: Tighten verification in G1ResetSkipCompactingClosure

Reviewed-by: tschatzl, sjohanss

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

  • 2bbeae3: 8279668: x86: AVX2 versions of vpxor should be asserted
  • 3121898: 8279703: G1: Remove unused force_not_compacted local in G1CalculatePointersClosure::do_heap_region
  • bf7bcaa: 8277748: Obsolete the MinInliningThreshold flag in JDK 19
  • 126328c: 8279560: AArch64: generate_compare_long_string_same_encoding and LARGE_LOOP_PREFETCH alignment
  • ec5a455: 8279682: JFR: Remove dead code
  • 6504458: 8279647: JFR: Unclosed directory stream
  • 2f13872: 8279646: JFR: Remove recursive call in jdk.jfr.internal.Control
  • 2f7665b: 8279644: hsdis may not work when it was built with --with-binutils=system
  • 4471e95: 8279645: JFR: The cacheEventType in Dispatcher is never assigned
  • d7e6e9b: 8279643: JFR: Explain why path is sometimes missing from FileRead and FileWrite events
  • ... and 364 more: https://git.openjdk.java.net/jdk/compare/53df9ffbca841358eb8bb49837ec4bf43a1a743b...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 openjdk bot added the ready label Dec 13, 2021
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 10, 2022

@albertnetymk This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Copy link
Contributor

@kstefanj kstefanj left a comment

I'm ok with leaving this as is as well.

@albertnetymk
Copy link
Member Author

@albertnetymk albertnetymk commented Jan 11, 2022

Thanks for the review.

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jan 11, 2022

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

  • c08b2ac: 8225093: Special property jdk.boot.class.path.append should not default to empty string
  • 4c52eb3: 8279669: test/jdk/com/sun/jdi/TestScaffold.java uses wrong condition
  • d46410c: 8279785: JFR: 'jfr configure' should show default values
  • 2bbeae3: 8279668: x86: AVX2 versions of vpxor should be asserted
  • 3121898: 8279703: G1: Remove unused force_not_compacted local in G1CalculatePointersClosure::do_heap_region
  • bf7bcaa: 8277748: Obsolete the MinInliningThreshold flag in JDK 19
  • 126328c: 8279560: AArch64: generate_compare_long_string_same_encoding and LARGE_LOOP_PREFETCH alignment
  • ec5a455: 8279682: JFR: Remove dead code
  • 6504458: 8279647: JFR: Unclosed directory stream
  • 2f13872: 8279646: JFR: Remove recursive call in jdk.jfr.internal.Control
  • ... and 367 more: https://git.openjdk.java.net/jdk/compare/53df9ffbca841358eb8bb49837ec4bf43a1a743b...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label Jan 11, 2022
@openjdk openjdk bot closed this Jan 11, 2022
@openjdk openjdk bot removed ready rfr labels Jan 11, 2022
@openjdk
Copy link

@openjdk openjdk bot commented Jan 11, 2022

@albertnetymk Pushed as commit 08e14c6.

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

@albertnetymk albertnetymk deleted the full-gc-compact-assert branch Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-gc integrated
3 participants