8284780: Need methods to create pre-sized HashSet and LinkedHashSet #8302

XenoAmess · 2022-04-19T18:00:19Z

as title.

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
Change requires a CSR request to be approved

Issues

JDK-8284780: Need methods to create pre-sized HashSet and LinkedHashSet
JDK-8287419: Need methods to create pre-sized HashSet and LinkedHashSet (CSR)

Reviewers

Naoto Sato (@naotoj - Reviewer) ⚠️ Review applies to 98bfb0e1
Brian Burkhalter (@bplb - Reviewer) ⚠️ Review applies to 98bfb0e1
Daniel Fuchs (@dfuch - Reviewer) ⚠️ Review applies to 98bfb0e1
Anthony Scarpino (@ascarpino - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8302

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

Using diff file

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

bridgekeeper · 2022-04-19T18:01:57Z

👋 Welcome back XenoAmess! 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 · 2022-04-19T18:04:07Z

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

core-libs

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 · 2022-04-19T18:10:49Z

Webrevs

liach · 2022-04-19T22:17:54Z

Need to add apiNote documentation section to capacity-based constructors like for maps.

dfuch · 2022-04-20T09:19:50Z

/csr needed

openjdk · 2022-04-20T09:20:54Z

@dfuch has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@XenoAmess please create a CSR request for issue JDK-8284780. This pull request cannot be integrated until the CSR request is approved.

XenoAmess · 2022-04-20T16:04:24Z

/csr needed

@stuart-marks csr please

XenoAmess · 2022-04-20T16:14:42Z

Need to add apiNote documentation section to capacity-based constructors like for maps.

@liach done.

bridgekeeper · 2022-05-18T23:10:15Z

@XenoAmess This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

stuart-marks · 2022-05-18T23:20:45Z

@XenoAmess oops, sorry for the delay. I think it would be good to get these into 19 as companions to HashMap.newHashMap et. al.

As before, I'd suggest reducing the number of changes to use sites in order to make review easier. I would suggest keeping the changes under src in java.base, java.net.http, java.rmi, and jdk.zipfs, and omitting all the other changes. Also keep the changes under test/jdk.

There needs to be a test for these new methods. I haven't thought much how to do that. My first attempt would be to modify our favorite WhiteBoxResizeTest and add a bit of machinery that asserts the table length of the HashMap contained within the created HashSet/LinkedHashSet. I haven't looked at it though, so it might not work out, in which case you should pursue an alternative approach.

I'll look at the specs and suggest updates as necessary and then handle filing of a CSR.

XenoAmess · 2022-05-24T20:48:55Z

@stuart-marks

suggest keeping the changes under src in java.base, java.net.http, java.rmi, and jdk.zipfs, and omitting all the other changes. Also keep the changes under test/jdk.

Done.

modify our favorite WhiteBoxResizeTest and add a bit of machinery that asserts the table length of the HashMap contained within the created HashSet/LinkedHashSet.

Test added...I don't really know if the test be done in the way you want, but I think it could work.

test/jdk/java/util/HashMap/WhiteBoxResizeTest.java

stuart-marks

I looked at all the use sites and they look fine. Some look like they could use additional cleanup, but that's probably beyond the scope of this change. (Also, I haven't seen StringTokenizer in a long time....) It's amazing how many bugs there are -- the majority look like they allocated the HashSet with the wrong capacity! Again, this proves the worth of these new APIs.

src/java.base/share/classes/java/util/HashSet.java

test/jdk/java/util/HashMap/WhiteBoxResizeTest.java

naotoj · 2022-05-25T21:23:59Z

(Also, I haven't seen StringTokenizer in a long time....)

That's some old code lingering in locale-related stuff. Will fix them after this PR gets integrated.

stuart-marks · 2022-06-01T17:40:47Z

Reviewers for i18n, net, nio, and security, please review call site changes in your areas. Thanks.

naotoj

Reviewed i18n-related changes and they look good. One minor suggestion in Calendar, but that can be applied later.

src/java.base/share/classes/java/util/Calendar.java

openjdk · 2022-06-01T18:30:09Z

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

8284780: Need methods to create pre-sized HashSet and LinkedHashSet

Reviewed-by: naoto, bpb, dfuchs, ascarpino

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

130ce7c: 8288052: Small logging clarification during failed heap shrinkage
b623398: 8287901: Loom: Failures with -XX:+VerifyStack
04f02ac: 8214976: Warn about uses of functions replaced for portability
024a240: 8287333: Clean up ParamTaglet and ThrowsTaglet
c8cff1b: 8202449: overflow handling in Random.doubles
c15e10f: 8233760: Result of BigDecimal.toString throws overflow exception on new BigDecimal(str)
b92ce26: 8281001: Class::forName(String) defaults to system class loader if the caller is null
c68419f: 8286990: Add compiler name to warning messages in Compiler Directive
6fb84e2: 8287541: Files.writeString fails to throw IOException for charset "windows-1252"
a9b9831: 8286663: Resolve IDE warnings in WTrayIconPeer and SystemTray
... and 192 more: https://git.openjdk.java.net/jdk/compare/7cb368b34df97614f0d208b8b0b93aa714a49282...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 (@stuart-marks, @naotoj, @bplb, @dfuch, @ascarpino) 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).

bplb

java.io and java.nio look all right.

dfuch

Changes to net and http look good.

openjdk-notifier · 2022-06-08T17:37:41Z

@XenoAmess Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

XenoAmess · 2022-06-08T17:42:16Z

/integrate

openjdk · 2022-06-08T17:44:50Z

@XenoAmess
Your change (at version 95d59b9) is now ready to be sponsored by a Committer.

stuart-marks · 2022-06-08T20:13:21Z

Running tests and awaiting review from security team. Our internal test system is backlogged and tests might not complete in time to get into JDK 19.

ascarpino

I gave a quick look at the security files touched and seems straightforward. I didn't see any problems

stuart-marks · 2022-06-09T01:48:50Z

/sponsor

openjdk · 2022-06-09T01:51:01Z

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

a941bc2: 8288082: Build failure due to clang_major is not defined after JDK-8214976
65f0829: 8288068: Javadoc contains spurious reference to CLinker
130ce7c: 8288052: Small logging clarification during failed heap shrinkage
b623398: 8287901: Loom: Failures with -XX:+VerifyStack
04f02ac: 8214976: Warn about uses of functions replaced for portability
024a240: 8287333: Clean up ParamTaglet and ThrowsTaglet
c8cff1b: 8202449: overflow handling in Random.doubles
c15e10f: 8233760: Result of BigDecimal.toString throws overflow exception on new BigDecimal(str)
b92ce26: 8281001: Class::forName(String) defaults to system class loader if the caller is null
c68419f: 8286990: Add compiler name to warning messages in Compiler Directive
... and 194 more: https://git.openjdk.java.net/jdk/compare/7cb368b34df97614f0d208b8b0b93aa714a49282...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-06-09T01:51:20Z

@stuart-marks @XenoAmess Pushed as commit e01cd7c.

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

The HashSet(int) constructor accepts the capacity of the set rather than its size. Unfortunately, HashSet requires N/0.75 capacity to hold N elements. So, if we call new HashSet(n) to hold N elements, the map will be too small and need to be resized/rehashed midway. We add a new methods Sets.newHashSetWithExpectedSize and Sets.newLinkedHashSetWithExpectedSize that calculate the correct capacity for the specified expected size. See * https://bugs.openjdk.org/browse/JDK-8284975 * https://bugs.openjdk.org/browse/JDK-8287419 * openjdk/jdk#8302

The HashSet(int) constructor accepts the capacity of the set rather than its size. Unfortunately, HashSet requires N/0.75 capacity to hold N elements. So, if we call new HashSet(n) to hold N elements, the map will be too small and need to be resized/rehashed midway. We add a new methods Sets.newHashSetWithExpectedSize and Sets.newLinkedHashSetWithExpectedSize that calculate the correct capacity for the specified expected size. See * https://bugs.openjdk.org/browse/JDK-8284975 * https://bugs.openjdk.org/browse/JDK-8287419 * openjdk/jdk#8302 (cherry picked from commit 26c1d33)

stuart-marks · 2022-07-07T00:29:37Z

@liach

'the new' fix should be applied to newHashMap etc. too.

I've filed JDK-8289872 for this. It's a small thing but best to fix it before we forget about it.

XenoAmess · 2022-07-08T13:53:42Z

@liach

'the new' fix should be applied to newHashMap etc. too.

I've filed JDK-8289872 for this. It's a small thing but best to fix it before we forget about it.

Sorry for I already forgotten it.

8284780: Need methods to create pre-sized HashSet and LinkedHashSet

436273e

openjdk bot added core-libs core-libs-dev@openjdk.org rfr Pull request is ready for review labels Apr 19, 2022

XenoAmess added 3 commits April 20, 2022 02:17

fix LinkedHashSet constructor

b139366

migrate LinkedHashSet usage

a64ccc1

migrate HashSet usages

f051c1f

XenoAmess force-pushed the fix_8284780 branch from e052c34 to f051c1f Compare April 19, 2022 20:10

openjdk bot added the csr Pull request needs approved CSR before integration label Apr 20, 2022

add apiNote s

26bb579

XenoAmess added 4 commits April 21, 2022 02:59

add more replaces

931e504

add more replaces

53fdb7c

add more replaces

704e895

add more replaces

363fcc5

revert much too changes for newHashSet

07e9b8b

add test for newHashSet and newLinkedHashSet

56d029f

ExE-Boss reviewed May 25, 2022

View reviewed changes

test/jdk/java/util/HashMap/WhiteBoxResizeTest.java Outdated Show resolved Hide resolved

liach reviewed May 25, 2022

View reviewed changes

test/jdk/java/util/HashMap/WhiteBoxResizeTest.java Outdated Show resolved Hide resolved

stuart-marks reviewed May 25, 2022

View reviewed changes

src/java.base/share/classes/java/util/HashSet.java Show resolved Hide resolved

test/jdk/java/util/HashMap/WhiteBoxResizeTest.java Outdated Show resolved Hide resolved

openjdk bot removed the csr Pull request needs approved CSR before integration label Jun 1, 2022

naotoj approved these changes Jun 1, 2022

View reviewed changes

src/java.base/share/classes/java/util/Calendar.java Outdated Show resolved Hide resolved

openjdk bot added the ready Pull request is ready to be integrated label Jun 1, 2022

bplb approved these changes Jun 1, 2022

View reviewed changes

dfuch approved these changes Jun 2, 2022

View reviewed changes

remove null check for Capacitiable in WhiteBoxResizeTest

bacc9ca

clean up Calendar

95d59b9

openjdk bot added the sponsor Pull request is ready to be sponsored label Jun 8, 2022

ascarpino approved these changes Jun 8, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jun 9, 2022

openjdk bot closed this Jun 9, 2022

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 Jun 9, 2022

arteam mentioned this pull request Jun 16, 2022

Correctly pre-size HashSets elastic/elasticsearch#87700

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8284780: Need methods to create pre-sized HashSet and LinkedHashSet #8302

8284780: Need methods to create pre-sized HashSet and LinkedHashSet #8302

XenoAmess commented Apr 19, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 19, 2022

openjdk bot commented Apr 19, 2022

mlbridge bot commented Apr 19, 2022 •

edited

Loading

liach commented Apr 19, 2022

dfuch commented Apr 20, 2022

openjdk bot commented Apr 20, 2022

XenoAmess commented Apr 20, 2022

XenoAmess commented Apr 20, 2022

bridgekeeper bot commented May 18, 2022

stuart-marks commented May 18, 2022

XenoAmess commented May 24, 2022 •

edited

Loading

stuart-marks left a comment

naotoj commented May 25, 2022

stuart-marks commented Jun 1, 2022

naotoj left a comment

openjdk bot commented Jun 1, 2022 •

edited

Loading

bplb left a comment

dfuch left a comment

openjdk-notifier bot commented Jun 8, 2022

XenoAmess commented Jun 8, 2022

openjdk bot commented Jun 8, 2022

stuart-marks commented Jun 8, 2022

ascarpino left a comment

stuart-marks commented Jun 9, 2022

openjdk bot commented Jun 9, 2022

openjdk bot commented Jun 9, 2022

stuart-marks commented Jul 7, 2022

XenoAmess commented Jul 8, 2022

8284780: Need methods to create pre-sized HashSet and LinkedHashSet #8302

8284780: Need methods to create pre-sized HashSet and LinkedHashSet #8302

Conversation

XenoAmess commented Apr 19, 2022 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Apr 19, 2022

openjdk bot commented Apr 19, 2022

mlbridge bot commented Apr 19, 2022 • edited Loading

Webrevs

liach commented Apr 19, 2022

dfuch commented Apr 20, 2022

openjdk bot commented Apr 20, 2022

XenoAmess commented Apr 20, 2022

XenoAmess commented Apr 20, 2022

bridgekeeper bot commented May 18, 2022

stuart-marks commented May 18, 2022

XenoAmess commented May 24, 2022 • edited Loading

stuart-marks left a comment

Choose a reason for hiding this comment

naotoj commented May 25, 2022

stuart-marks commented Jun 1, 2022

naotoj left a comment

Choose a reason for hiding this comment

openjdk bot commented Jun 1, 2022 • edited Loading

bplb left a comment

Choose a reason for hiding this comment

dfuch left a comment

Choose a reason for hiding this comment

openjdk-notifier bot commented Jun 8, 2022

XenoAmess commented Jun 8, 2022

openjdk bot commented Jun 8, 2022

stuart-marks commented Jun 8, 2022

ascarpino left a comment

Choose a reason for hiding this comment

stuart-marks commented Jun 9, 2022

openjdk bot commented Jun 9, 2022

openjdk bot commented Jun 9, 2022

stuart-marks commented Jul 7, 2022

XenoAmess commented Jul 8, 2022

XenoAmess commented Apr 19, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 19, 2022 •

edited

Loading

XenoAmess commented May 24, 2022 •

edited

Loading

openjdk bot commented Jun 1, 2022 •

edited

Loading