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

8252752: Clear card table for old regions during scan in G1 #343

Closed

Conversation

@tschatzl
Copy link
Contributor

@tschatzl tschatzl commented Sep 24, 2020

Hi all,

can I get reviews for this change that removes the need for explicit clearing of the card table for typically a large amount of regions in most cases?

Currently g1 unconditionally marks scanned dirty cards as "scanned" to remember that they had already been scanned in earlier evacuation passes. Then in the "clear card table phase" g1 clears all these marks including the card table of evacuated regions.

This change modifies g1 so that if possible, it clears dirty cards after scanning them instead of setting them to "scanned" if there is only one evacuation pass. This saves a lot of work later (e.g. the amount of regions to clear in the clear card table phase may be reduced by a magnitude or more, depending on the ration between evacuated and scanned regions).

The main change here is which type of regions (ones in the collection set, ones that are only scanned) are put into which list of regions g1 already manages: there is one "all" list containing all regions that need clearing, and a "next" list containing the regions to be scanned in the current evacuation pass. Previously g1 put regions into at least one list; now regions in the current collection set are always directly put into the "all" list (as they independently of other reasons require card table cleaning), while to-be-scanned regions are always only put into the "next" list.

Then, to achieve the desired effect that only regions that absolutely require clearing are in the "all" list, g1 does not merge the "next" list into the "all" list when it detects that there will only be one evacuation pass.

Performance impact:
Decreases clear card table time significantly if the number of regions in the collection set is much smaller than the number of scanned regions (as it decreases the number of regions to clear). Some cursory perf testing showed slight overall pause time improvements.

Testing:
hs-tier1-5 ("Initial import" change)
hs-tier1-2 (latest)


Progress

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

Issue

  • JDK-8252752: Clear card table for old regions during scan in G1

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 24, 2020

👋 Welcome back tschatzl! 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
Copy link

@openjdk openjdk bot commented Sep 24, 2020

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

@openjdk openjdk bot added the hotspot-gc label Sep 24, 2020
tschatzl added 2 commits Sep 24, 2020
@tschatzl tschatzl marked this pull request as ready for review Sep 25, 2020
@openjdk openjdk bot added the rfr label Sep 25, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 25, 2020

Webrevs

Copy link

@kimbarrett kimbarrett left a comment

I found confusing the name may_do_optional_evacuation, used in various
places for local variables, parameter names, and member variables.

"may do" to me suggests "permitted". I think better might be something like
"have_optional_evacuation_regions" or "have_optional_evacuation_work" or
something along those lines.

Other than that naming issue, this looks good.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 25, 2020

@tschatzl 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 more details.

After integration, the commit message for the final commit will be:

8252752: Clear card table for old regions during scan in G1

Reviewed-by: kbarrett, iwalulya, ayang

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

  • 276fcee: 8252835: Revert fix for JDK-8246051
  • ca1ed16: 8253639: Change os::attempt_reserve_memory_at parameter order
  • fed3636: 8252219: C2: Randomize IGVN worklist for stress testing
  • 625a935: 8253638: Cleanup os::reserve_memory and remove MAP_FIXED
  • f014854: 8253624: gtest fails when run from make with read-only source directory
  • 7817963: 8247691: [aarch64] Incorrect handling of VM exceptions in C1 deopt stub/traps
  • 79904c1: 8252730: jlink does not create reproducible builds on different servers
  • ea7c47c: 7179006: [macosx] Print-to-file doesn't work: printing to the default printer instead.
  • b66fa8f: 8253572: [windows] CDS archive may fail to open with long file names
  • 4167540: 8253647: Remove dead code in os::create_thread() on Linux/BSD
  • ... and 24 more: https://git.openjdk.java.net/jdk/compare/bf442c5b9e1eb013f562dd9495ed18a66b7fb648...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 Sep 25, 2020
Copy link
Member

@walulyai walulyai left a comment

minor comments

@@ -103,8 +104,8 @@ class G1CardTable : public CardTable {
// be inaccurate as it does not perform the dirtying atomically.
inline size_t mark_region_dirty(size_t start_card_index, size_t num_cards);

// Mark the given range of cards as Scanned. All of these cards must be Dirty.
inline void mark_as_scanned(size_t start_card_index, size_t num_cards);
// Mark the given range of cards to "which". All of these cards must be Dirty.

This comment has been minimized.

@walulyai

walulyai Sep 25, 2020
Member

Maybe change the comment to "Mark the given range of dirty cards as..." or the function name to "mark_dirty_to"

This comment has been minimized.

@tschatzl

tschatzl Sep 25, 2020
Author Contributor

I think I fixed that.

@@ -3940,6 +3952,8 @@ void G1CollectedHeap::evacuate_optional_collection_set(G1ParScanThreadStateSet*
evacuate_next_optional_regions(per_thread_states);
phase_times()->record_or_add_optional_evac_time((Ticks::now() - start).seconds() * 1000.0);
}

rem_set()->complete_evac_phase(true /* has_more_than_one_evacuation_phase */);

This comment has been minimized.

@walulyai

walulyai Sep 25, 2020
Member

I think it would be cleaner with a local variable:
bool has_more_than_one_evacuation_phase = true;
rem_set()->complete_evac_phase(has_more_than_one_evacuation_phase);

Instead of the comment, same with remember_already_scanned_cards comment above

This comment has been minimized.

@tschatzl

tschatzl Sep 25, 2020
Author Contributor

I kept it as is as this is the common hotspot style for "documenting" flag parameters, and this suggestion does not seem better to me.

void evacuate_initial_collection_set(G1ParScanThreadStateSet* per_thread_states);
// The may_do_optional_evacuation flag indicates whether some or more optional evacuation
// steps will follow.
void evacuate_initial_collection_set(G1ParScanThreadStateSet* per_thread_states,

This comment has been minimized.

@albertnetymk

albertnetymk Sep 25, 2020
Member

The PR description provides a very good motivation for this patch, and I think such motivation should be reprinted here to explain why we are interested in tracking if there is an optional evac.

This comment has been minimized.

@tschatzl

tschatzl Sep 25, 2020
Author Contributor

I added a little bit of motivation. I am planning to write up more about the card scanning and the various motivations in a better place. I hope the text that is now there is sufficient.

Thanks for your review.

This comment has been minimized.

@albertnetymk

albertnetymk Sep 25, 2020
Member

I hope the text that is now there is sufficient.

Sure; thank you.

@tschatzl
Copy link
Contributor Author

@tschatzl tschatzl commented Sep 25, 2020

@albertnetymk, @kimbarrett, @walulyai, thanks for your reviews.

Thomas

// The may_do_optional_evacuation flag indicates whether some or more optional evacuation
// steps will follow.
// The has_optional_evacuation_work flag for the initial collection set
// evacuation indicates whether some or more optional evacuation steps may

This comment has been minimized.

@tschatzl
Copy link
Contributor Author

@tschatzl tschatzl commented Sep 28, 2020

Thanks @kimbarrett @albertnetymk @walulyai for the reviews!

/integrate

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

@openjdk openjdk bot commented Sep 28, 2020

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

  • 276fcee: 8252835: Revert fix for JDK-8246051
  • ca1ed16: 8253639: Change os::attempt_reserve_memory_at parameter order
  • fed3636: 8252219: C2: Randomize IGVN worklist for stress testing
  • 625a935: 8253638: Cleanup os::reserve_memory and remove MAP_FIXED
  • f014854: 8253624: gtest fails when run from make with read-only source directory
  • 7817963: 8247691: [aarch64] Incorrect handling of VM exceptions in C1 deopt stub/traps
  • 79904c1: 8252730: jlink does not create reproducible builds on different servers
  • ea7c47c: 7179006: [macosx] Print-to-file doesn't work: printing to the default printer instead.
  • b66fa8f: 8253572: [windows] CDS archive may fail to open with long file names
  • 4167540: 8253647: Remove dead code in os::create_thread() on Linux/BSD
  • ... and 24 more: https://git.openjdk.java.net/jdk/compare/bf442c5b9e1eb013f562dd9495ed18a66b7fb648...master

Your commit was automatically rebased without conflicts.

Pushed as commit e9c1782.

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

@tschatzl tschatzl deleted the tschatzl:8252752-clear-card-table-opt branch Sep 28, 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.