8258048: Placeholder hash code is the same as Dictionary hash code #1789

coleenp · 2020-12-15T21:41:40Z

This change is a set of cleanups to the placeholders hashtable. It moves the index calculation inside the hashtable. It avoids using the placeholder hashcode because it's the same hashcode as the dictionary hashcode. There are some asserts for this.

One of the find_class() functions is redundant to dictionary->find_class() so is removed.

Tested with tier1-6.

Progress

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

Issue

JDK-8258048: Placeholder hash code is the same as Dictionary hash code

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 0071785
Harold Seigel (@hseigel - Reviewer) ⚠️ Review applies to ddf96fa
Lois Foltan (@lfoltan - Reviewer)

Download

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

bridgekeeper · 2020-12-15T21:41:48Z

👋 Welcome back coleenp! 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 · 2020-12-15T21:42:58Z

@coleenp The following label will be automatically applied to this pull request:

hotspot-runtime

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 · 2020-12-15T21:46:03Z

Webrevs

dholmes-ora

Hi Coleen,
This all looks good to me.
Thanks,
David

openjdk · 2020-12-16T01:07:57Z

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

8258048: Placeholder hash code is the same as Dictionary hash code

Reviewed-by: dholmes, hseigel, lfoltan

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

ab5d581: 8258447: Move make/hotspot/hotspot.script to make/scripts
70183f4: 8251438: Issues with our POSIX set_signal_handler()
6eca296: 8258420: Move URL configuration from Docs.gmk to conf dir

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

coleenp · 2020-12-16T01:12:10Z

Thanks, David.

hseigel

Changes look good.
Thanks, Harold

lfoltan

Nice cleanup Coleen!
Lois

lfoltan · 2020-12-16T15:11:52Z

src/hotspot/share/classfile/placeholders.cpp

                                                 Symbol* name,
                                                 ClassLoaderData* loader_data,
                                                 classloadAction action,
                                                 Symbol* supername,
                                                 Thread* thread) {
-  PlaceholderEntry* probe = get_entry(index, hash, name, loader_data);
+  PlaceholderEntry* probe = get_entry(hash, name, loader_data);


Minor nit of a comment since you are in this file and specifically in this method can you consider changing the if statement conditional at line #143 to explicitly check if (probe != NULL)? I noticed line #167 has this type of conditional and it would be good to have consistency. I think there are some other cases in this file, like line #119, where it is inconsistent.

The if (probe) conditional on line #143 isn't necessary because we've just created a placeholder and code above would have null pointer references if it failed (in add_entry -> new_entry), so I removed it. I fixed another check against NULL just above this. I don't see any others here at least.
Thanks for reviewing!

coleenp · 2020-12-16T16:57:03Z

I took out some extra header file includes and fixed or removed some null checks. I'm rerunning sanity checks (tier1 on linux-x64, macosx-64, windows-x64 and linux-aarch64). Also cross compiled for ppc, arm, s390 and zero. Changing include files can be dangerous.
I also reran jck lang and vm tests.

lfoltan

Thank you for making suggested changes. Looks good.
Lois

coleenp · 2020-12-16T19:17:29Z

Thanks for the rereview, Lois. Sanity tests now passed.
/integrate

openjdk · 2020-12-16T19:18:21Z

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

ab5d581: 8258447: Move make/hotspot/hotspot.script to make/scripts
70183f4: 8251438: Issues with our POSIX set_signal_handler()
6eca296: 8258420: Move URL configuration from Docs.gmk to conf dir

Your commit was automatically rebased without conflicts.

Pushed as commit c463264.

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

8258048: Placeholder hash code is the same as Dictionary hash code

0071785

openjdk bot added the rfr Pull request is ready for review label Dec 15, 2020

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Dec 15, 2020

dholmes-ora approved these changes Dec 16, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 16, 2020

Merge branch 'master' into class-loading

ddf96fa

hseigel approved these changes Dec 16, 2020

View reviewed changes

lfoltan approved these changes Dec 16, 2020

View reviewed changes

Fix or remove extraneous null checks.

0ec6dc9

lfoltan approved these changes Dec 16, 2020

View reviewed changes

openjdk bot closed this Dec 16, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 16, 2020

coleenp deleted the class-loading branch December 16, 2020 19:19

8258048: Placeholder hash code is the same as Dictionary hash code #1789

8258048: Placeholder hash code is the same as Dictionary hash code #1789

coleenp commented Dec 15, 2020 •

edited by openjdk bot

bridgekeeper bot commented Dec 15, 2020

openjdk bot commented Dec 15, 2020

mlbridge bot commented Dec 15, 2020 •

edited

dholmes-ora left a comment

openjdk bot commented Dec 16, 2020 •

edited

coleenp commented Dec 16, 2020

hseigel left a comment

lfoltan left a comment

lfoltan Dec 16, 2020

coleenp Dec 16, 2020

coleenp commented Dec 16, 2020 •

edited

lfoltan left a comment

coleenp commented Dec 16, 2020

openjdk bot commented Dec 16, 2020

8258048: Placeholder hash code is the same as Dictionary hash code #1789

8258048: Placeholder hash code is the same as Dictionary hash code #1789

Conversation

coleenp commented Dec 15, 2020 • edited by openjdk bot

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Dec 15, 2020

openjdk bot commented Dec 15, 2020

mlbridge bot commented Dec 15, 2020 • edited

Webrevs

dholmes-ora left a comment

Choose a reason for hiding this comment

openjdk bot commented Dec 16, 2020 • edited

coleenp commented Dec 16, 2020

hseigel left a comment

Choose a reason for hiding this comment

lfoltan left a comment

Choose a reason for hiding this comment

lfoltan Dec 16, 2020

Choose a reason for hiding this comment

coleenp Dec 16, 2020

Choose a reason for hiding this comment

coleenp commented Dec 16, 2020 • edited

lfoltan left a comment

Choose a reason for hiding this comment

coleenp commented Dec 16, 2020

openjdk bot commented Dec 16, 2020

coleenp commented Dec 15, 2020 •

edited by openjdk bot

mlbridge bot commented Dec 15, 2020 •

edited

openjdk bot commented Dec 16, 2020 •

edited

coleenp commented Dec 16, 2020 •

edited