8253420: Refactor HeapRegionManager::find_highest_free #2193

albertnetymk · 2021-01-22T08:59:51Z

Using for-loop to make the number of iterations more explicit. Direct backward iteration, for (uint curr = reserved_length() - 1; curr >= 0; curr--) doesn't work due to underflow of uint type. Therefore, I went for current approach.

Test: hotspot_gc

Progress

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

Issue

JDK-8253420: Refactor HeapRegionManager::find_highest_free

Reviewers

Stefan Johansson (@kstefanj - Reviewer)
Kim Barrett (@kimbarrett - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2193/head:pull/2193
$ git checkout pull/2193

bridgekeeper · 2021-01-22T08:59:58Z

👋 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 · 2021-01-22T09:02:10Z

@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 · 2021-01-22T09:06:05Z

Webrevs

01: Full - Incremental (67ce558)
00: Full (c734b63)

kimbarrett · 2021-01-22T20:46:46Z

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

-  uint curr = reserved_length() - 1;
-  while (true) {
+  for (uint i = 0; i < reserved_length(); ++i) {
+    uint curr = reserved_length() - 1 - i;


Maybe this instead?

for (uint curr = reserved_length(); curr-- > 0; ) {

I would prefer not having side effect in the condition. At first glance, it's not obvious how many iteration the loop entails, length or length - 1?

Avoiding side effects is normally good, but in this case I think it actually make the whole intent of the code clearer. We could add to the comment that we loop backwards through all reserved regions to make it clear what the bound is.

Updated as suggested, since you both think it's better.

openjdk · 2021-01-26T09:27:12Z

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

8253420: Refactor HeapRegionManager::find_highest_free

Reviewed-by: sjohanss, 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 78 new commits pushed to the master branch:

1c77046: 8260404: jvm_io.h include missing in a number of files
bd2744d: 8260106: Shenandoah: refactor reference updating closures and related code
c836da3: 8252412: [macos11] system dynamic libraries removed from filesystem
e1411fd: 6606673: Path2D.Double, Path2D.Float and GeneralPath ctors throw exception when initialCapacity is negative
6f2be9c: 8246112: Remove build-time and run-time checks for clock_gettime and CLOCK_MONOTONIC
19b6f61: 8260334: Remove deprecated sv_for_node_id() from Compile
1bebd41: 8260421: Shenandoah: Fix conc_mark_roots timing name and indentations
42cef27: 8260343: Delete obsolete classes in the Windows L&F
9f0a043: 8260286: Manual Test "ws/open/test/jdk/sun/security/tools/jarsigner/compatibility/Compatibility.java" fails
fd00ed7: 8256298: Shenandoah: Enable concurrent stack processing
... and 68 more: https://git.openjdk.java.net/jdk/compare/c3c6662528311d3fd3f29bb0131fc5cfd832183c...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kstefanj, @kimbarrett) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

albertnetymk · 2021-01-27T07:36:04Z

Thank you for the reviews.

/integrate

openjdk · 2021-01-27T07:36:35Z

@albertnetymk
Your change (at version 67ce558) is now ready to be sponsored by a Committer.

kstefanj · 2021-01-27T09:54:20Z

/integrate

openjdk · 2021-01-27T09:54:34Z

@kstefanj Only the author (@albertnetymk) is allowed to issue the integrate command. As this PR is ready to be sponsored, and you are an eligible sponsor, did you mean to issue the /sponsor command?

kstefanj · 2021-01-27T09:56:35Z

/sponsor

openjdk · 2021-01-27T09:57:39Z

@kstefanj @albertnetymk Since your change was applied there have been 80 commits pushed to the master branch:

4d004c9: 8260449: Remove stale declaration of SATBMarkQueue::apply_closure_and_empty()
fd2641e: 8260236: better init AnnotationCollector _contended_group
1c77046: 8260404: jvm_io.h include missing in a number of files
bd2744d: 8260106: Shenandoah: refactor reference updating closures and related code
c836da3: 8252412: [macos11] system dynamic libraries removed from filesystem
e1411fd: 6606673: Path2D.Double, Path2D.Float and GeneralPath ctors throw exception when initialCapacity is negative
6f2be9c: 8246112: Remove build-time and run-time checks for clock_gettime and CLOCK_MONOTONIC
19b6f61: 8260334: Remove deprecated sv_for_node_id() from Compile
1bebd41: 8260421: Shenandoah: Fix conc_mark_roots timing name and indentations
42cef27: 8260343: Delete obsolete classes in the Windows L&F
... and 70 more: https://git.openjdk.java.net/jdk/compare/c3c6662528311d3fd3f29bb0131fc5cfd832183c...master

Your commit was automatically rebased without conflicts.

Pushed as commit fa40a96.

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

for

c734b63

openjdk bot added the rfr Pull request is ready for review label Jan 22, 2021

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Jan 22, 2021

kimbarrett reviewed Jan 22, 2021

View reviewed changes

review

67ce558

kstefanj approved these changes Jan 26, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 26, 2021

kimbarrett approved these changes Jan 27, 2021

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Jan 27, 2021

openjdk bot closed this Jan 27, 2021

openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 27, 2021

albertnetymk deleted the for branch January 27, 2021 10:02

8253420: Refactor HeapRegionManager::find_highest_free #2193

8253420: Refactor HeapRegionManager::find_highest_free #2193

Uh oh!

Conversation

albertnetymk commented Jan 22, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Download

Uh oh!

bridgekeeper bot commented Jan 22, 2021

Uh oh!

openjdk bot commented Jan 22, 2021

Uh oh!

mlbridge bot commented Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

kimbarrett Jan 22, 2021

Choose a reason for hiding this comment

Uh oh!

albertnetymk Jan 22, 2021

Choose a reason for hiding this comment

Uh oh!

kstefanj Jan 23, 2021

Choose a reason for hiding this comment

Uh oh!

albertnetymk Jan 26, 2021

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

albertnetymk commented Jan 27, 2021

Uh oh!

openjdk bot commented Jan 27, 2021

Uh oh!

kstefanj commented Jan 27, 2021

Uh oh!

openjdk bot commented Jan 27, 2021

Uh oh!

kstefanj commented Jan 27, 2021

Uh oh!

openjdk bot commented Jan 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

albertnetymk commented Jan 22, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 22, 2021 •

edited

Loading

openjdk bot commented Jan 26, 2021 •

edited

Loading