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

8255234: ZGC: Bulk allocate forwarding data structures #804

Closed
wants to merge 4 commits into from

Conversation

pliden
Copy link
Contributor

@pliden pliden commented Oct 22, 2020

Allocation of forwarding data structures (ZForwarding/ZForwardingEntry instances) can generate a lot of calls to new/malloc if the number of pages in the relocation set is large. This can make the "Concurrent Select Relocation Set" phase long. By bulk allocating a single piece of memory and then placement new() all objects into that memory we can speed this phase up quite a bit.

During relocation set selection we keep track of the number of ZForwardingEntry instances we will need for the selected relocation set. The ZForwardingAllocator is then initialized to have enough room for all the relocation set array, all ZForwarding instances, and all ZForwardingEntry instances. Objects are then placement new()'d using into the memory allocated by the ZForwardingAllocator.

This patch was joint work with Albert (ayang).


Progress

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

Issue

  • JDK-8255234: ZGC: Bulk allocate forwarding data structures

Reviewers

Contributors

  • Albert Mingkun Yang <ayang@openjdk.org>
  • Per Liden <pliden@openjdk.org>

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 22, 2020

👋 Welcome back pliden! 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.

@pliden
Copy link
Contributor Author

@pliden pliden commented Oct 22, 2020

/contributor add ayang

@openjdk
Copy link

@openjdk openjdk bot commented Oct 22, 2020

@pliden
Contributor Albert Mingkun Yang <ayang@openjdk.org> successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 22, 2020

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

@pliden
Copy link
Contributor Author

@pliden pliden commented Oct 22, 2020

/contributor add pliden

@openjdk
Copy link

@openjdk openjdk bot commented Oct 22, 2020

@pliden
Contributor Per Liden <pliden@openjdk.org> successfully added.

@pliden pliden marked this pull request as ready for review Oct 22, 2020
@openjdk openjdk bot added the rfr label Oct 22, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 22, 2020

Webrevs

ZForwarding::destroy(_forwardings[i]);
_forwardings[i] = NULL;
}
_nforwardings = 0;
Copy link
Member

@albertnetymk albertnetymk Oct 22, 2020

Choose a reason for hiding this comment

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

This is not needed, right? _nforwardings will be properly reset in populate.

Copy link
Contributor Author

@pliden pliden Oct 23, 2020

Choose a reason for hiding this comment

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

Yes, this is strictly not needed. But even if the backing memory is still there, the relocation set is conceptually reset here. For example, RelocationSetIterator should not return any forwardings after this point.

Copy link
Member

@albertnetymk albertnetymk Oct 23, 2020

Choose a reason for hiding this comment

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

True; the set is conceptually empty after this.

if (new_top > _end) {
// Not enough space left
return NULL;
}
Copy link
Member

@albertnetymk albertnetymk Oct 22, 2020

Choose a reason for hiding this comment

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

This allocator should never return null; better use assert.

Copy link
Contributor Author

@pliden pliden Oct 23, 2020

Choose a reason for hiding this comment

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

Yep, good point. Will fix.

}

// Update statistics
ZStatRelocation::set_at_populate_relocation_set(_allocator.size());
Copy link
Member

@albertnetymk albertnetymk Oct 22, 2020

Choose a reason for hiding this comment

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

Instead of _allocator.size(), one could use the same arg of _allocator.reset(), which is already known in this scope.

Copy link
Contributor Author

@pliden pliden Oct 23, 2020

Choose a reason for hiding this comment

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

Sure, I can change that.

Copy link
Contributor Author

@pliden pliden Oct 23, 2020

Choose a reason for hiding this comment

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

Actually, the size() function will still be needed in the follow up patch #805, where the size isn't calculated in the same score. So in the end I think it's easier to just keep it here too.

Copy link
Member

@albertnetymk albertnetymk Oct 23, 2020

Choose a reason for hiding this comment

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

OK.

ZForwarding::destroy(_forwardings[i]);
_forwardings[i] = NULL;
}
_nforwardings = 0;
Copy link
Member

@albertnetymk albertnetymk Oct 23, 2020

Choose a reason for hiding this comment

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

True; the set is conceptually empty after this.

}

// Update statistics
ZStatRelocation::set_at_populate_relocation_set(_allocator.size());
Copy link
Member

@albertnetymk albertnetymk Oct 23, 2020

Choose a reason for hiding this comment

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

OK.

Copy link
Member

@stefank stefank left a comment

Looks good. Thanks for accepting the proposed changes. (Some old comments might have been sent by the mlbridge. I've delete those in the PR)

@openjdk
Copy link

@openjdk openjdk bot commented Oct 27, 2020

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

8255234: ZGC: Bulk allocate forwarding data structures

Co-authored-by: Albert Mingkun Yang <ayang@openjdk.org>
Co-authored-by: Per Liden <pliden@openjdk.org>
Reviewed-by: ayang, stefank

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

  • b7d483c: 8255245: C1: Fix output of -XX:+PrintCFGToFile to open it with visualizer
  • 5e0a8cd: 8255488: Re-enable some problem listed tests
  • 3f8bd92: 8233556: [TESTBUG] JPopupMenu tests fail on MacOS
  • 4acf634: 8171998: javax/swing/JMenu/4692443/bug4692443.java fails on Windows
  • a804c6a: 8254871: Remove unnecessary string copy in NetworkInterface.c
  • acd0e25: 8255254: Split os::reserve_memory and os::map_memory_to_file interfaces
  • dc85a3f: 8015602: [macosx] Test javax/swing/SpringLayout/4726194/bug4726194.java fails on MacOSX
  • 1d245c6: 8252117: com/sun/jdi/BadHandshakeTest.java failed with "ConnectException: Connection refused: connect"
  • 7a7ce02: 8007632: DES/3DES keys support in PKCS12 keystore
  • a023b93: 8255394: jdk/test/lib/hexdump/ASN1FormatterTest.java fails with ---illegal-access=deny
  • ... and 69 more: https://git.openjdk.java.net/jdk/compare/299e1154af86797b80c0c6176102d42d4f9ae1c6...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 Oct 27, 2020
@pliden
Copy link
Contributor Author

@pliden pliden commented Oct 28, 2020

/integrate

@openjdk openjdk bot closed this Oct 28, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 28, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 28, 2020

@pliden Since your change was applied there have been 79 commits pushed to the master branch:

  • b7d483c: 8255245: C1: Fix output of -XX:+PrintCFGToFile to open it with visualizer
  • 5e0a8cd: 8255488: Re-enable some problem listed tests
  • 3f8bd92: 8233556: [TESTBUG] JPopupMenu tests fail on MacOS
  • 4acf634: 8171998: javax/swing/JMenu/4692443/bug4692443.java fails on Windows
  • a804c6a: 8254871: Remove unnecessary string copy in NetworkInterface.c
  • acd0e25: 8255254: Split os::reserve_memory and os::map_memory_to_file interfaces
  • dc85a3f: 8015602: [macosx] Test javax/swing/SpringLayout/4726194/bug4726194.java fails on MacOSX
  • 1d245c6: 8252117: com/sun/jdi/BadHandshakeTest.java failed with "ConnectException: Connection refused: connect"
  • 7a7ce02: 8007632: DES/3DES keys support in PKCS12 keystore
  • a023b93: 8255394: jdk/test/lib/hexdump/ASN1FormatterTest.java fails with ---illegal-access=deny
  • ... and 69 more: https://git.openjdk.java.net/jdk/compare/299e1154af86797b80c0c6176102d42d4f9ae1c6...master

Your commit was automatically rebased without conflicts.

Pushed as commit 2c9dfc7.

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

@pliden pliden deleted the 8255234_bulk_alloc branch Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-gc integrated
3 participants