Skip to content

8277372: Add getters for BOT and card table members #6570

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

Closed
wants to merge 7 commits into from

Conversation

vish-chan
Copy link
Contributor

@vish-chan vish-chan commented Nov 26, 2021

Changed the visibility, added getters and refactored the following:

  1. Card Table Members
  2. BOT members
  3. ObjectStartArray block members

Progress

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

Issue

  • JDK-8277372: Add getters for BOT and card table members

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6570/head:pull/6570
$ git checkout pull/6570

Update a local copy of the PR:
$ git checkout pull/6570
$ git pull https://git.openjdk.java.net/jdk pull/6570/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6570

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6570.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 26, 2021

👋 Welcome back vish-chan! 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 added the rfr Pull request is ready for review label Nov 26, 2021
@openjdk
Copy link

openjdk bot commented Nov 26, 2021

@vish-chan 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 pull request command.

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Nov 26, 2021
@mlbridge
Copy link

mlbridge bot commented Nov 26, 2021

Webrevs

@tschatzl
Copy link
Contributor

@tstuefe : can you check whether the s390 and ppc changes still compile? The changes look straightforward enough, but...

Thanks,
Thomas

@tstuefe
Copy link
Member

tstuefe commented Nov 27, 2021

@tschatzl We have s390 + ppcle builds now in GHAs thanks to Alexey, and they do look fine (https://github.com/openjdk/jdk/pull/6570/checks?check_run_id=4341321057). Seeing how simple the platform changes are, I think this is okay.

Cheers, Thomas

Copy link
Contributor

@tschatzl tschatzl left a comment

Choose a reason for hiding this comment

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

I will push it through our testing, particular the changes for the SA agent (in vmstructs_gc.hpp) are always good to double-check.

Comment on lines 52 to 56
private:
static uint _LogN;
static uint _LogN_words;
static uint _N_bytes;
static uint _N_words;
Copy link
Contributor

@tschatzl tschatzl Nov 29, 2021

Choose a reason for hiding this comment

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

The private visibility modifier can be removed as this is default at the top of a class.
The static variables should start with a lower case letter after the underscore, something like _log_n.

My suggestion would also be to change N/n to something more understandable, like size, and add block, i.e. something like _log_block_size, _log_block_size_in_words similar to the corresponding CardTable members etc.

Edit: note that "block" isn't a good word to use here, so scratch that - "block" is any kind of area that is more generic than an object, but does not refer to the BOT entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I can understand, we need to replace "N" with something meaningful. Does something like "entry_size" or "bot_entry_size" would work?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would think that bot_entry_size is one byte. Probably "bot_card_size"?

Copy link
Contributor

@tschatzl tschatzl left a comment

Choose a reason for hiding this comment

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

Getting good :) Some minor comments.

@@ -287,12 +287,12 @@ void G1BlockOffsetTablePart::alloc_block_work(HeapWord** threshold_, HeapWord* b
for (size_t j = orig_index + 1; j <= end_index; j++) {
assert(_bot->offset_array(j) > 0 &&
_bot->offset_array(j) <=
(u_char) (BOTConstants::N_words+BOTConstants::N_powers-1),
(u_char) (BOTConstants::bot_card_size_words()+BOTConstants::N_powers-1),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
(u_char) (BOTConstants::bot_card_size_words()+BOTConstants::N_powers-1),
(u_char) (BOTConstants::bot_card_size_words() + BOTConstants::N_powers - 1),

Pre-existing: operator has no spaces around it

"offset array should have been set - "
"%u not > 0 OR %u not <= %u",
(uint) _bot->offset_array(j),
(uint) _bot->offset_array(j),
(uint) (BOTConstants::N_words+BOTConstants::N_powers-1));
(uint) (BOTConstants::bot_card_size_words()+BOTConstants::N_powers-1));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
(uint) (BOTConstants::bot_card_size_words()+BOTConstants::N_powers-1));
(uint) (BOTConstants::bot_card_size_words() + BOTConstants::N_powers - 1));

Pre-existing: spaces around operator

@@ -413,7 +413,7 @@ void CardTable::dirty_card_iterate(MemRegion mr, MemRegionClosure* cl) {
next_entry <= limit && *next_entry == dirty_card;
dirty_cards++, next_entry++);
MemRegion cur_cards(addr_for(cur_entry),
dirty_cards*card_size_in_words);
dirty_cards*_card_size_in_words);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
dirty_cards*_card_size_in_words);
dirty_cards * _card_size_in_words);

Pre-existing: spaces around operator

@@ -439,7 +439,7 @@ MemRegion CardTable::dirty_card_range_after_reset(MemRegion mr,
next_entry <= limit && *next_entry == dirty_card;
dirty_cards++, next_entry++);
MemRegion cur_cards(addr_for(cur_entry),
dirty_cards*card_size_in_words);
dirty_cards*_card_size_in_words);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
dirty_cards*_card_size_in_words);
dirty_cards * _card_size_in_words);

Pre-existing: spaces around operator

Comment on lines 49 to 52
static uint _block_shift;
static uint _block_size;
static uint _block_size_in_words;

Copy link
Contributor

Choose a reason for hiding this comment

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

Almost the same naming issue as in the BlockOffsetTable/SharedArray; I would prefer if these members (and getters) here were named similarly to the ones there.
It is true that ObjectStartArray and BlockOffsetTable are basically the same thing, but any eventual merge is another issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shall these be renamed to osa_card_shift? Something like this?

Copy link
Contributor

@tschatzl tschatzl Dec 1, 2021

Choose a reason for hiding this comment

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

Okay, _osa_card_shift and so on is also acceptable to me, but just _card_shift/_card_size would be fine too. We typically do not prefix members with abbreviations of class name they are in; I see now that we have that _bot prefix in the other class, for me the bot prefix not necessarily refers to the class but to the concept of a block offset table.

Maybe just reduce the names in both cases to _card_size/_card_shift/_log_card_<something>? What do others think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean for both blockOffsetTable and objectStartArray, all the relevant members be renamed to _card_size and so on?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this sounds like a good plan.

Copy link
Contributor

@kstefanj kstefanj left a comment

Choose a reason for hiding this comment

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

Nice to see this unified. Some comments below.

Comment on lines 52 to 55
static uint _log_bot_card_size;
static uint _log_bot_card_size_words;
static uint _bot_card_size_bytes;
static uint _bot_card_size_words;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change words to in_words to better match other places where we have sizes in words. And as Thomas suggested above, maybe also drop the bot in/pre-fix.

And also change the getters below.

Comment on lines 49 to 52
static uint _block_shift;
static uint _block_size;
static uint _block_size_in_words;

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this sounds like a good plan.

Copy link
Member

@albertnetymk albertnetymk left a comment

Choose a reason for hiding this comment

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

Just a very minor comment.

}

void ObjectStartArray::initialize(MemRegion reserved_region) {
// We're based on the assumption that we use the same
// size blocks as the card table.
assert((int)block_size == (int)CardTable::card_size, "Sanity");
assert(block_size <= MaxBlockSize, "block_size must be less than or equal to " UINT32_FORMAT, MaxBlockSize);
assert((int)_card_size == (int)(CardTable::card_size()), "Sanity");
Copy link
Member

Choose a reason for hiding this comment

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

I think the int cast can be dropped; both sides are uint.

@openjdk
Copy link

openjdk bot commented Dec 3, 2021

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

8277372: Add getters for BOT and card table members

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

  • 2b87c2b: 8277793: Support vector F2I and D2L cast operations for X86
  • e1cde19: 8278247: KeyStoreSpi::engineGetAttributes does not throws KeyStoreException
  • a729a70: 8225181: KeyStore should have a getAttributes method
  • 38f525e: 8275821: Optimize random number generators developed in JDK-8248862 using Math.unsignedMultiplyHigh()
  • 780b8b1: 8278179: Enable all doclint warnings for build of java.naming
  • 678ac58: 8278240: ProblemList containers/docker/TestJcmd.java on linux-aarch64
  • 01cb2b9: 8277529: SIGSEGV in C2 CompilerThread Node::rematerialize() compiling Packet::readUnsignedTrint
  • 660f21a: 8278119: ProblemList few headful test failing in macosx12-aarch64 system
  • 2e30fa9: 8278171: [vectorapi] Mask incorrectly computed for zero extending cast
  • fbf096e: 8251400: Fix incorrect addition of library to test in JDK-8237858
  • ... and 8 more: https://git.openjdk.java.net/jdk/compare/dda8f26ce0d2b54ef78b74b82e58255aca80576a...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 (@tschatzl, @kstefanj, @albertnetymk) 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).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Dec 3, 2021
Copy link
Contributor

@tschatzl tschatzl left a comment

Choose a reason for hiding this comment

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

Lgtm.

@vish-chan
Copy link
Contributor Author

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 3, 2021
@openjdk
Copy link

openjdk bot commented Dec 3, 2021

@vish-chan
Your change (at version 35112e6) is now ready to be sponsored by a Committer.

@vish-chan
Copy link
Contributor Author

@tschatzl is there anything else required from my end?

@tschatzl
Copy link
Contributor

tschatzl commented Dec 6, 2021

/sponsor
Thanks @vish-chan for fixing all this.

@openjdk
Copy link

openjdk bot commented Dec 6, 2021

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

  • 7c6f57f: 8275610: C2: Object field load floats above its null check resulting in a segfault
  • a885aab: 8276125: RunThese24H.java SIGSEGV in JfrThreadGroup::thread_group_id
  • 6994d80: 8278291: compiler/uncommontrap/TraceDeoptimizationNoRealloc.java fails with release VMs after JDK-8154011
  • 286a26c: 8278277: G1: Simplify implementation of G1GCPhaseTimes::record_or_add_time_secs
  • d14f06a: 8278031: MultiThreadedRefCounter should not use relaxed atomic decrement
  • 8d190dd: 8277496: Remove duplication in c1 Block successor lists
  • 194cdf4: 8277864: Compilation error thrown while doing a boxing conversion on selector expression
  • f39fe5b: 8154011: Make TraceDeoptimization a diagnostic flag
  • f180a45: 8278016: Add compiler tests to tier{2,3}
  • 104aa1f: 8268575: Annotations not visible on model elements before they are generated
  • ... and 23 more: https://git.openjdk.java.net/jdk/compare/dda8f26ce0d2b54ef78b74b82e58255aca80576a...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Dec 6, 2021
@openjdk openjdk bot added the integrated Pull request has been integrated label Dec 6, 2021
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Dec 6, 2021
@openjdk
Copy link

openjdk bot commented Dec 6, 2021

@tschatzl @vish-chan Pushed as commit adf3952.

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

@vish-chan vish-chan deleted the JDK-8277372-refactor branch January 13, 2022 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

5 participants