8293210: G1: Remove redundant check in G1FreeHumongousRegionClosure #10119

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

Simple change of removing unnecessary code.

Test: hotspot_gc

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-8293210: G1: Remove redundant check in G1FreeHumongousRegionClosure

Reviewers

Thomas Schatzl (@tschatzl - Reviewer) ⚠️ Review applies to 01a7246d
Kim Barrett (@kimbarrett - Reviewer) ⚠️ Review applies to 01a7246d

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10119

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

Using diff file

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

bridgekeeper · 2022-09-01T11:17:50Z

👋 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-09-01T11:21:17Z

@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-09-01T11:25:49Z

Webrevs

openjdk · 2022-09-05T12:39:29Z

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

8293210: G1: Remove redundant check in G1FreeHumongousRegionClosure

Reviewed-by: tschatzl, kbarrett

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

8082c24: 8054572: [macosx] JComboBox paints the border incorrectly
b920d29: 8271328: User is able to choose the color after disabling the color chooser.
5725a93: 8293879: Remove unnecessary castings in jdk.hotspot.agent
ab7f58a: 6286501: JTabbedPane throws NPE from its stateChanged listener in particular case
d41f69f: 8293849: PrintIdealPhase in compiler directives file is ignored when used with other compile commands
471e2f1: 8292088: C2: assert(is_OuterStripMinedLoop()) failed: invalid node class: IfTrue

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

kimbarrett · 2022-09-09T09:37:15Z

src/hotspot/share/gc/g1/g1YoungGCPostEvacuateTasks.cpp

@@ -182,10 +182,6 @@ class G1FreeHumongousRegionClosure : public HeapRegionClosure {
  }

  virtual bool do_heap_region(HeapRegion* r) {
-    if (!r->is_starts_humongous()) {
-      return false;
-    }


I think this check is a fast-path to quickly filter out definitely uninteresting regions, which will be nearly all of them. I'm not convinced it should be removed.

It's the same argument as JDK-8292858; the attr-table can (or should) be used instead of the region-type.

Yeah, and I thought about objecting to that one on the same grounds. In retrospect I regret not doing so.

I see no issue with not using the attr-table. The attr-table is not the UR-source of the information. It is instead a cache of a non-trivial calculation. Avoiding it by directly using one of the inputs to that calculation because doing so gives us a fast-path for what is by far the most common case seems perfectly fine.

kimbarrett · 2022-09-09T09:39:25Z

src/hotspot/share/gc/g1/g1CollectedHeap.inline.hpp

@@ -233,7 +233,6 @@ inline bool G1CollectedHeap::is_obj_dead_full(const oop obj) const {
 }

 inline bool G1CollectedHeap::is_humongous_reclaim_candidate(uint region) {
-  assert(_hrm.at(region)->is_starts_humongous(), "Must start a humongous object");


I think I like the semantic requirement that this only be applied to starts-humongous regions. Unless there is some future change that needs this removal (and not just because of the change to G1FreeHumongousRegionClosure::do_heap_region that I don't like), then I'd prefer it stay.

There are no other uses in the near future. However, this method doesn't really require that as a pre-condition, even before this PR.

That it doesn't (currently) require that is arguably an implementation detail. I'm speaking of the intended semantics of the function. I think it's a query about the region that corresponds to a humongous object. It's placed near other predicates on objects. That its argument is a region rather than an oop is an inconsistency that arises from how it's being used. Maybe it's misplaced here?

albertnetymk · 2022-09-19T11:19:01Z

Added a micro benchmark and some results to the ticket. On my box, this PR reduces the relevant phase length by ~50%.

tschatzl

Still good.

kimbarrett

Looks good. One optional suggestion.

kimbarrett · 2022-09-19T11:36:59Z

src/hotspot/share/gc/g1/g1YoungGCPostEvacuateTasks.cpp

-    if (!is_reclaimable(region_idx)) {
-      return false;
-    }
+    G1CollectedHeap* g1h = G1CollectedHeap::heap();


Maybe look up the heap once at closure construction time, caching it in a data member?

albertnetymk · 2022-09-19T18:02:58Z

Thanks for the review.

/integrate

openjdk · 2022-09-19T18:05:37Z

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

8ff2c26: 8293942: [JVMCI] data section entries must be 4-byte aligned on AArch64
6e23b43: 8293502: (fc) FileChannel::transfer methods fail to copy /proc files on Linux
1f9ff41: 8292297: Fix up loading of override java.security properties file
64b96e5: 8293811: Provide a reason for PassFailJFrame.forceFail
f91762f: 8293965: Code signing warnings after JDK-8293550
8082c24: 8054572: [macosx] JComboBox paints the border incorrectly
b920d29: 8271328: User is able to choose the color after disabling the color chooser.
5725a93: 8293879: Remove unnecessary castings in jdk.hotspot.agent
ab7f58a: 6286501: JTabbedPane throws NPE from its stateChanged listener in particular case
d41f69f: 8293849: PrintIdealPhase in compiler directives file is ignored when used with other compile commands
... and 1 more: https://git.openjdk.org/jdk/compare/a93cf926356b974b8fc5a97d230a15bad066ac2a...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-09-19T18:05:54Z

@albertnetymk Pushed as commit 4020ed5.

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

g1-entail-2

2ba4b9e

openjdk bot changed the title ~~8293210~~ 8293210: G1: Remove redundant check in G1FreeHumongousRegionClosure Sep 1, 2022

openjdk bot added the rfr Pull request is ready for review label Sep 1, 2022

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Sep 1, 2022

tschatzl approved these changes Sep 5, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 5, 2022

kimbarrett reviewed Sep 9, 2022

View reviewed changes

albertnetymk added 2 commits September 19, 2022 12:12

Merge branch 'master' into g1-entail-2

49985a6

index-iteration

01a7246

tschatzl approved these changes Sep 19, 2022

View reviewed changes

kimbarrett approved these changes Sep 19, 2022

View reviewed changes

cache g1h

cc34238

openjdk bot added the integrated Pull request has been integrated label Sep 19, 2022

openjdk bot closed this Sep 19, 2022

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

albertnetymk deleted the g1-entail-2 branch September 19, 2022 18:06

8293210: G1: Remove redundant check in G1FreeHumongousRegionClosure #10119

8293210: G1: Remove redundant check in G1FreeHumongousRegionClosure #10119

Uh oh!

Conversation

albertnetymk commented Sep 1, 2022 • 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 Sep 1, 2022

Uh oh!

openjdk bot commented Sep 1, 2022

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

openjdk bot commented Sep 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kimbarrett Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

albertnetymk Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

kimbarrett Sep 10, 2022

Choose a reason for hiding this comment

Uh oh!

kimbarrett Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

albertnetymk Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

kimbarrett Sep 10, 2022

Choose a reason for hiding this comment

Uh oh!

albertnetymk commented Sep 19, 2022

Uh oh!

tschatzl left a comment

Choose a reason for hiding this comment

Uh oh!

kimbarrett left a comment

Choose a reason for hiding this comment

Uh oh!

kimbarrett Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

albertnetymk commented Sep 19, 2022

Uh oh!

openjdk bot commented Sep 19, 2022

Uh oh!

openjdk bot commented Sep 19, 2022

Uh oh!

Uh oh!

albertnetymk commented Sep 1, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 1, 2022 •

edited

Loading

openjdk bot commented Sep 5, 2022 •

edited

Loading