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

8253237: [REDO] Improve large object handling during evacuation #266

Closed
wants to merge 1 commit into from

Conversation

@kimbarrett
Copy link

@kimbarrett kimbarrett commented Sep 19, 2020

Please review this change to improve the handling of arrays by G1's
copy_to_survivor_space.

This is essentially the same change that was pushed 9/15:
https://bugs.openjdk.java.net/browse/JDK-8158045
#90

and then backed out due to tripping over a compiler bug:
https://bugs.openjdk.java.net/browse/JDK-8253169
[BACKOUT] Improve large object handling during evacuation
#181

That compiler bug has been worked around:
https://bugs.openjdk.java.net/browse/JDK-8253270
Limit fastdebug inlining in G1 evacuation
#220

so now redoing the original change to array handling. The original
JDK-8158045 change was cherry-picked into this change. There were a couple of
minor merge conflicts with JDK-8253270.

The actual change (substantially cribbed from the old PR):

Change type dispatching and handling in G1's evacuation copying, in order to
improve the hot paths and improve array handling. This change addresses
several closely co-located enhancement requests; it seemed difficult to split
them up in a sensible way.

do_copy_to_survivor_space now gets the klass of the object being copied once,
up front, for use in multiple places. This avoids fetching (including
re-decoding when compressed) the klass several times. This addresses part of
JDK-8027545.

Moved check for and handling of string deduplication later, only applying it
after the special array cases have been dealt with, since strings are not
arrays. (They are header objects pointing to an array of character values.)

Special case typeArray, doing nothing other than the copy, since they contain
no oops that need to be processed. This addresses JDK-8027761.

Changed handling of objArray, possibly enqueuing multiple partial array tasks
for an array that is large enough. Initially no more than one partial array
task will be enqueued. As each such tasks is processed, an increasing number
of additional tasks may be enqueued, up to a limit based on the number of
worker threads. This gives other threads an opportunity to steal such tasks,
while keeping queue growth under control. Most of the calculation for this is
handled by a new helper class, so this can later be shared with ParallelGC.
This addresses JDK-8158045.

The dispatch on array klass type has also been changed. It will now break (via
an assert) Project Valhalla, rather than quietly doing something that probably
isn't what is actually wanted. I've discussed this with them so there is a
plan for dealing with it when they take this update.

Deleted a lingering reference to G1ParScanPartialArrayClosure that was deleted
long ago (JDK-8035330, JDK 9).

Ran tier1-6 in mach5 and some local stress tests.

I've not redone performance testing. I don't think there should be any change
from the previous go-round, which was pretty much performance neutral.

/issue 8253237, 8253238, 8253236
/label hotspot-gc
/summary Generate multiple partial array tasks for large objArrays.


Progress

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

Issues

  • JDK-8253237: [REDO] Improve large object handling during evacuation
  • JDK-8253238: [REDO] Improve object array chunking test in G1's copy_to_survivor_space
  • JDK-8253236: [REDO] Investigate fast-path for scanning only objects with references during gc

Reviewers

Download

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

8027761: Investigate fast-path for scanning only objects with references during gc
8027545: Improve object array chunking test in G1's copy_to_survivor_space

Generate multiple partial array tasks for large objArrays.

Reviewed-by: sjohanss, ayang, tschatzl
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 19, 2020

👋 Welcome back kbarrett! 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 changed the title 8253237: [REDO] Improve large object handling during evacuation 8253237: [REDO] Improve large object handling during evacuation Sep 19, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 19, 2020

@kimbarrett This issue is referenced in the PR title - it will now be updated.

Adding additional issue to issue list: 8253238: [REDO] Improve object array chunking test in G1's copy_to_survivor_space.

Adding additional issue to issue list: 8253236: [REDO] Investigate fast-path for scanning only objects with references during gc.

@openjdk openjdk bot added the hotspot-gc label Sep 19, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 19, 2020

@kimbarrett
The hotspot-gc label was successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 19, 2020

@kimbarrett Setting summary to:

Generate multiple partial array tasks for large objArrays.
<!-- Anything below this marker will be automatically updated, please do not edit manually! -->
---------
### Progress
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
- [ ] Change must be properly reviewed

### Issues
* [JDK-8253237](https://bugs.openjdk.java.net/browse/JDK-8253237): [REDO] Improve large object handling during evacuation
* [JDK-8253238](https://bugs.openjdk.java.net/browse/JDK-8253238): [REDO] Improve object array chunking test in G1's copy_to_survivor_space
* [JDK-8253236](https://bugs.openjdk.java.net/browse/JDK-8253236): [REDO] Investigate fast-path for scanning only objects with references during gc


### Download
`$ git fetch https://git.openjdk.java.net/jdk pull/266/head:pull/266`
`$ git checkout pull/266`
@openjdk
Copy link

@openjdk openjdk bot commented Sep 19, 2020

@kimbarrett The following label will be automatically applied to this pull request: hotspot.

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 (add|remove) "label" command.

@openjdk openjdk bot added the hotspot label Sep 19, 2020
@kimbarrett
Copy link
Author

@kimbarrett kimbarrett commented Sep 19, 2020

/summary Generate multiple partial array tasks for large objArrays.

@kimbarrett
Copy link
Author

@kimbarrett kimbarrett commented Sep 19, 2020

/label remove hotspot

@openjdk
Copy link

@openjdk openjdk bot commented Sep 19, 2020

@kimbarrett Updating existing summary to Generate multiple partial array tasks for large objArrays.

@openjdk openjdk bot removed the hotspot label Sep 19, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 19, 2020

@kimbarrett
The hotspot label was successfully removed.

@kimbarrett kimbarrett marked this pull request as ready for review Sep 19, 2020
@openjdk openjdk bot added the rfr label Sep 19, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 19, 2020

Webrevs

Copy link
Contributor

@kstefanj kstefanj left a comment

Looked through the changes again and they still look good.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 21, 2020

@kimbarrett This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8253237: [REDO] Improve large object handling during evacuation
8253238: [REDO] Improve object array chunking test in G1's copy_to_survivor_space
8253236: [REDO] Investigate fast-path for scanning only objects with references during gc

Generate multiple partial array tasks for large objArrays.

Reviewed-by: sjohanss, ayang, tschatzl
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 21 commits pushed to 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 automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate d1f9b8a8b54843f06a93078c4a058af86fcc2aac.

➡️ 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 Sep 21, 2020
@kimbarrett
Copy link
Author

@kimbarrett kimbarrett commented Sep 22, 2020

Thanks for reviews, Stefan, Albert, and Thomas.

@kimbarrett
Copy link
Author

@kimbarrett kimbarrett commented Sep 22, 2020

/integrate

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

@openjdk openjdk bot commented Sep 22, 2020

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

Your commit was automatically rebased without conflicts.

Pushed as commit 0e98fc1.

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

@kimbarrett kimbarrett deleted the kimbarrett:partial_arrays branch Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.