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

Trim identity-hashcode to 25bit #3

Closed
wants to merge 3 commits into from
Closed

Conversation

rkennke
Copy link
Collaborator

@rkennke rkennke commented May 10, 2021

I'd like to trim the identity hashcode to 25 bits to make room in the upper 32bits for the compressed class-pointer.

This could in theory regress performance on workloads that make heavy use of i-hashes, but I could not show this in practice (by specjvm workloads like compiler which uses lots of ihashes).

Next step would be to move the Klass* into the upper 32bit part. (If you look closely, this would be the exact header layout of 32bit JVMs.) And then get rid of the dedicated Klass* field. :-D Eventually I'd make ihash-bits and klass* bits configurable to trade one for the other.

Testing:

  • some manual benchmarks
  • tier1
  • tier2

Progress

  • Change must not contain extraneous whitespace
  • Change must be properly reviewed

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 10, 2021

👋 Welcome back rkennke! 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 label May 10, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 10, 2021

Webrevs

// unused:25 hash:31 -->| unused_gap:1 age:4 biased_lock:1 lock:2 (normal object)
// JavaThread*:54 epoch:2 unused_gap:1 age:4 biased_lock:1 lock:2 (biased object)
// unused:32 hash:25 -->| age:4 biased_lock:1 lock:2 (normal object)
// JavaThread*:54 epoch:2 age:4 biased_lock:1 lock:2 (biased object)
Copy link
Collaborator

@shipilev shipilev May 10, 2021

Choose a reason for hiding this comment

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

This does not compute. Where did the unused_gap go in the second case? Is epoch now larger? Or does JavaThread* now takes 55 bits?

Copy link
Collaborator Author

@rkennke rkennke May 10, 2021

Choose a reason for hiding this comment

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

Epoch and JavaThread in the biased case are already overlaid with hashcode. Hashcode and biased state doesn't happen at the same time. 32+25+4+1+2 == 64.

Copy link
Collaborator Author

@rkennke rkennke May 10, 2021

Choose a reason for hiding this comment

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

Oh right, unused_gap is now used, epoch is moved 1 bit to the right and JavaThread is now 55 bits (actually, I guess it is still 54 bits and the uppermost bit is unused). I will correct this.

Copy link
Collaborator

@shipilev shipilev left a comment

Ok, good, here is a provisional approval, while you correct the comments.

@openjdk
Copy link

@openjdk openjdk bot commented May 10, 2021

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

Trim identity-hashcode to 25bit

Reviewed-by: shade

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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 May 10, 2021
@rkennke
Copy link
Collaborator Author

@rkennke rkennke commented May 11, 2021

/integrate

@openjdk openjdk bot closed this May 11, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels May 11, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 11, 2021

@rkennke Pushed as commit 08987ab.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated
2 participants