-
Notifications
You must be signed in to change notification settings - Fork 6k
8270333: -XX:+VerifyStringTableAtExit should not do linear search #4772
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
8270333: -XX:+VerifyStringTableAtExit should not do linear search #4772
Conversation
👋 Welcome back iklam! A progress list of the required criteria for merging this PR into |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes seem fine, but I don't like your use of templates here.
Thanks,
David
template<bool update> | ||
unsigned int java_lang_String::hash_code_impl(oop java_string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a really weird way to pass a parameter! Why do we want a template here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi David, thanks for the review. I replaced the template function with an inline function.
@iklam 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:
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 14 new commits pushed to the
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay ... I'm not sure why you want the code to be duplicated though?
I could add a new It probably doesn't matter, but I just don't want to deal with any potential performance issues when changing code used only for debugging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but one question.
public: | ||
size_t _errors; | ||
VerifyCompStrings(GrowableArray<oop>* oops) : _oops(oops), _errors(0) {} | ||
VerifyCompStrings() : _table(unsigned(_items_count + 8 / 8)), _errors(0) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you mean unsigned((_items_count + 8) / 8)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review. I changed it to _table(unsigned(_items_count / 8) + 1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Thanks @dholmes-ora and @yminqi for the review. |
Going to push as commit 1ebd946.
Your commit was automatically rebased without conflicts. |
The implementation of
-XX:+VerifyStringTableAtExit
used a linear search to check for duplicated strings in the interned string table. This would be excessively slow if there were more than a few thousand strings.This PR uses a hashtable so we can avoid doing a linear search. The table is sized such that each bucket has 8 items on average.
Since this code is executed only for verification, I added a new
java_lang_String::hash_code_noupdate(oop)
function, so we can avoid causing side effects.Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4772/head:pull/4772
$ git checkout pull/4772
Update a local copy of the PR:
$ git checkout pull/4772
$ git pull https://git.openjdk.java.net/jdk pull/4772/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 4772
View PR using the GUI difftool:
$ git pr show -t 4772
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4772.diff