8318647: Serial: Refactor BlockOffsetTable #16304

albertnetymk · 2023-10-23T08:14:46Z

The diff is too large; maybe it's better to read the new impl directly, which I believe is much easier to follow.

There is some duplication with G1's implementation, which should probably be dealt with in its own PR.

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-8318647: Serial: Refactor BlockOffsetTable (Enhancement - P4)

Reviewers

Thomas Schatzl (@tschatzl - Reviewer)
Ivan Walulya (@walulyai - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16304

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-10-23T08:16:04Z

👋 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 · 2023-10-23T08:18:28Z

@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 · 2023-10-23T08:22:01Z

Webrevs

tschatzl

Initial batch of comments.

tschatzl · 2023-10-25T10:41:36Z

src/hotspot/share/gc/serial/serialBlockOffsetTable.hpp

-//////////////////////////////////////////////////////////////////////////
-// BlockOffsetSharedArray
-//////////////////////////////////////////////////////////////////////////
 class BlockOffsetSharedArray: public CHeapObj<mtGC> {


BlockOffsetSharedArray could probably be moved to the .cpp file.

resize is called by TenuredGeneration for heap-resizing.

tschatzl · 2023-10-25T10:43:56Z

src/hotspot/share/gc/serial/serialBlockOffsetTable.hpp

 // The CollectedHeap type requires subtypes to implement a method
 // "block_start".  For some subtypes, notably generational
 // systems using card-table-based write barriers, the efficiency of this
-// operation may be important.  Implementations of the "BlockOffsetArray"
+// operation may be important.  Implementations of the BlockOffsetTable
 // class may be useful in providing such efficient implementations.


The comment is completely outdated, talking about subtypes where there are none any more of this class.

I think the subtypes refer to heaps from different collectors. Not sth around BlockOffsetArray.

Still it isn't worth (and somewhat confusing) imo to talk about subtypes when there are none, particularly because the change deleted the comment about subtypes below.

OK, removed.

tschatzl · 2023-10-25T10:45:11Z

src/hotspot/share/gc/serial/serialBlockOffsetTable.hpp

-//////////////////////////////////////////////////////////////////////////
-// BlockOffsetSharedArray
-//////////////////////////////////////////////////////////////////////////
 class BlockOffsetSharedArray: public CHeapObj<mtGC> {


Should be prefixed by Serial, same as BlockOffsetTable (since this is a complete reimplementation anyway).

Maybe this renaming can be done in its own (trivial) PR for easier reviewing.

tschatzl · 2023-10-25T10:48:50Z

src/hotspot/share/gc/serial/serialBlockOffsetTable.hpp

+  static bool is_crossing_card_boundary(HeapWord* const obj_start,
+                                        HeapWord* const obj_end) {
+    HeapWord* cur_card_boundary = align_up_by_card_size(obj_start);
+    // strictly greater-than


This comment should not only mention what the code below does but also why.

tschatzl · 2023-10-25T10:49:24Z

src/hotspot/share/gc/serial/serialBlockOffsetTable.hpp

-  virtual size_t last_active_index() const;
 };

+


extra newline

tschatzl · 2023-10-25T10:54:09Z

src/hotspot/share/gc/serial/serialBlockOffsetTable.cpp

+void BlockOffsetTable::update_for_block_work(HeapWord* blk_start,
+                                             HeapWord* blk_end) {
+  HeapWord* const cur_card_boundary = align_up_by_card_size(blk_start);
+  size_t const offset_card =  _array->index_for(cur_card_boundary);


Suggested change

size_t const offset_card = _array->index_for(cur_card_boundary);

size_t const offset_card = _array->index_for(cur_card_boundary);

tschatzl · 2023-10-25T10:54:25Z

src/hotspot/share/gc/serial/serialBlockOffsetTable.cpp

-    offset = BOTConstants::card_size_in_words() + i;
-    if (reach >= end_card) {
-      _array->set_offset_array(start_card_for_region, end_card, offset, reducing);
+// Write the backskip value for each region.


Suggested change

// Write the backskip value for each region.

// Write the backskip value for the given region.

Note that region isn't defined very exactly. Here it is (apparently) the memory from blk_start to blk_end, but later it is also used as "area containing the same backskip value". Maybe there is some better wording to be found.

"region" in this file always means "logarithmic region". If we refer to [blk_start, blk_end], "block" should be used.

I noticed just now. It would be nice to define that somewhere. 👍

tschatzl · 2023-10-25T10:55:39Z

src/hotspot/share/gc/serial/serialBlockOffsetTable.cpp

+    for (uint i = 0; i < BOTConstants::N_powers; i++) {
+      // -1 so that the reach ends in this region and not at the start
+      // of the next.
+      size_t reach = offset_card + BOTConstants::power_to_cards_back(i+1) - 1;


Suggested change

size_t reach = offset_card + BOTConstants::power_to_cards_back(i+1) - 1;

size_t reach = offset_card + BOTConstants::power_to_cards_back(i + 1) - 1;

tschatzl · 2023-10-25T10:57:52Z

src/hotspot/share/gc/serial/serialBlockOffsetTable.cpp

+
+  if (offset_card != end_card) {
+    // Handling remaining cards.
+    size_t start_card_for_region = offset_card + 1;


Maybe start_card_for/to_update is a better name?

Obsolete given a definition of a "region".

tschatzl · 2023-10-25T10:58:40Z

src/hotspot/share/gc/serial/serialBlockOffsetTable.hpp

  // Array for keeping offsets for retrieving object start fast given an
  // address.
  VirtualSpace _vs;
  u_char* _offset_array;          // byte array keeping backwards offsets


I always wondered whether u_char is what we want here, maybe uint8_t is more appropriate?

Can probably be done in its own PR.

tschatzl · 2023-10-25T11:51:20Z

src/hotspot/share/gc/serial/serialBlockOffsetTable.cpp

+      if (reach >= end_card) {
+        _array->set_offset_array(start_card_for_region, end_card, value);
+        start_card_for_region = reach + 1;
+        break;
+      }
+      _array->set_offset_array(start_card_for_region, reach, value);
      start_card_for_region = reach + 1;


Suggested change

if (reach >= end_card) {

_array->set_offset_array(start_card_for_region, end_card, value);

start_card_for_region = reach + 1;

break;

}

_array->set_offset_array(start_card_for_region, reach, value);

start_card_for_region = reach + 1;

_array->set_offset_array(start_card_for_region, MIN2(end_card, reach), value);

start_card_for_region = reach + 1;

if (reach >= end_card) {

// Set all the cards covered by this region.

break;

}

I would probably try to merge these two paths as much as possible as suggested above (untested).

tschatzl

I would have renamed the classes as suggested as well when renaming the files; there does not seem to be much meaning to just renaming the files to me.

However assuming you will finish the renaming in an upcoming change anyway, I'll approve this.

tschatzl · 2023-10-30T09:02:58Z

src/hotspot/share/gc/serial/serialBlockOffsetTable.hpp

+// "N" = 2^"LogN".  An array with an entry for each such subregion indicates
+// how far back one must go to find the start of the chunk that includes the
+// first word of the subregion.
+class BlockOffsetTable {


The file has been renamed which is great, but the class is still called BlockOffsetTable without the prefix.

openjdk · 2023-10-30T09:07:15Z

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

8318647: Serial: Refactor BlockOffsetTable

Reviewed-by: tschatzl, iwalulya

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

b4f5379: 8304939: os::win32::exit_process_or_thread should be marked noreturn
0461d9a: 8318016: Per-compilation memory ceiling
2a76ad9: 8318683: compiler/c2/irTests/TestPhiDuplicatedConversion.java "Failed IR Rules (2) of Methods (2)"
b3fec6b: 8306980: Generated docs should contain correct Legal Documents
1139482: 8316132: CDSProtectionDomain::get_shared_protection_domain should check for exception
2182c93: 8313643: Update HarfBuzz to 8.2.2
613a3cc: 8301846: Invalid TargetDataLine after screen lock when using JFileChooser or COM library
613d32c: 8169475: WheelModifier.java fails by timeout
f1e8787: 8317609: Classfile API fails to verify /jdk.jcmd/sun/tools/jstat/Alignment.class
47624f6: 8299058: AssertionError in sun.net.httpserver.ServerImpl when connection is idle
... and 23 more: https://git.openjdk.org/jdk/compare/3934127b087ade1c1286008df3497ca6d84778a5...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.

albertnetymk · 2023-10-30T10:54:00Z

Renamed class and u_char. Split into their own commits for easier reviewing.

tschatzl

Thank you.

albertnetymk · 2023-11-01T11:46:55Z

Thanks for the review.

/integrate

openjdk · 2023-11-01T11:50:54Z

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

b4f5379: 8304939: os::win32::exit_process_or_thread should be marked noreturn
0461d9a: 8318016: Per-compilation memory ceiling
2a76ad9: 8318683: compiler/c2/irTests/TestPhiDuplicatedConversion.java "Failed IR Rules (2) of Methods (2)"
b3fec6b: 8306980: Generated docs should contain correct Legal Documents
1139482: 8316132: CDSProtectionDomain::get_shared_protection_domain should check for exception
2182c93: 8313643: Update HarfBuzz to 8.2.2
613a3cc: 8301846: Invalid TargetDataLine after screen lock when using JFileChooser or COM library
613d32c: 8169475: WheelModifier.java fails by timeout
f1e8787: 8317609: Classfile API fails to verify /jdk.jcmd/sun/tools/jstat/Alignment.class
47624f6: 8299058: AssertionError in sun.net.httpserver.ServerImpl when connection is idle
... and 23 more: https://git.openjdk.org/jdk/compare/3934127b087ade1c1286008df3497ca6d84778a5...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-11-01T11:51:05Z

@albertnetymk Pushed as commit ab19348.

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

s1-bot

40513f3

openjdk bot changed the title ~~8318647~~ 8318647: Serial: Refactor BlockOffsetTable Oct 23, 2023

openjdk bot added the rfr Pull request is ready for review label Oct 23, 2023

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Oct 23, 2023

tschatzl suggested changes Oct 25, 2023

View reviewed changes

review

2d12fb0

tschatzl suggested changes Oct 25, 2023

View reviewed changes

review

a4f6ea8

tschatzl approved these changes Oct 30, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 30, 2023

albertnetymk added 4 commits October 30, 2023 11:19

Merge branch 'master' into s1-bot

1968165

s1-rename

89d0b66

s1-rename

fe195c9

uint8

eabe68d

tschatzl approved these changes Oct 30, 2023

View reviewed changes

walulyai approved these changes Nov 1, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Nov 1, 2023

openjdk bot closed this Nov 1, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 1, 2023

albertnetymk deleted the s1-bot branch November 1, 2023 11:52

	size_t const offset_card = _array->index_for(cur_card_boundary);
	size_t const offset_card = _array->index_for(cur_card_boundary);

	// Write the backskip value for each region.
	// Write the backskip value for the given region.

	size_t reach = offset_card + BOTConstants::power_to_cards_back(i+1) - 1;
	size_t reach = offset_card + BOTConstants::power_to_cards_back(i + 1) - 1;

8318647: Serial: Refactor BlockOffsetTable #16304

8318647: Serial: Refactor BlockOffsetTable #16304

Uh oh!

Conversation

albertnetymk commented Oct 23, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Oct 23, 2023

Uh oh!

openjdk bot commented Oct 23, 2023

Uh oh!

mlbridge bot commented Oct 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

tschatzl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tschatzl Oct 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tschatzl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

albertnetymk commented Oct 30, 2023

Uh oh!

tschatzl left a comment

Choose a reason for hiding this comment

Uh oh!

albertnetymk commented Oct 23, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 23, 2023 •

edited

Loading

tschatzl Oct 25, 2023 •

edited

Loading

openjdk bot commented Oct 30, 2023 •

edited

Loading