Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v10]
On Wed, 25 May 2022 05:22:44 GMT, Stuart Marks wrote: >> test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 360: >> >>> 358: throw new RuntimeException(e); >>> 359: } >>> 360: }) >> >> These probably need a `mapField.setAccessible(true)` call, or a `VarHandle` >> for the `HashSet.map` field. > > Yes, this test fails with IllegalAccessException. Probably it's easiest to > use a VarHandle to get private fields, similar to other usage already in this > test. > > This test case is a bit odd though in that it's supposed to test HashSet and > LinkedHashSet but it mostly actually tests HashMap. It creates the Set > instance and immediately extracts the HashMap, which is then passed to the > actual test, which operates directly on the HashMap. It would be preferable > to create a Set; add an element (so that it's properly allocated); and then > make assertions over the Set (which involve extracting the HashMap, etc.) It > seems like there should be factoring that allows this sort of arrangement to > be retrofitted without adding too much complication. > > Finally, please add "8284780" to the `@bug` line at the top of this test. @stuart-marks I refactored the tests. please have a look. @stuart-marks 8284780 added. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v10]
On Wed, 25 May 2022 04:50:33 GMT, liach wrote:
>> XenoAmess has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> add test for newHashSet and newLinkedHashSet
>
> test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 354:
>
>> 352: rsc("rsls", size, cap, () -> {
>> 353: try {
>> 354: Field mapField =
>> HashSet.class.getDeclaredField("map");
>
> For clarity, consider using var handle/method handle and keep the reflection
> code in the `WhiteBoxResizeTest` constructor.
@liach I refactored the tests. please have a look.
-
PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v10]
On Wed, 25 May 2022 05:07:12 GMT, Stuart Marks wrote:
>> XenoAmess has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> add test for newHashSet and newLinkedHashSet
>
> src/java.base/share/classes/java/util/HashSet.java line 398:
>
>> 396: public static HashSet newHashSet(int numItems) {
>> 397: return new
>> HashSet<>(HashMap.calculateHashMapCapacity(numItems));
>> 398: }
>
> Please use "elements" instead of "items" throughout the specifications for
> the objects that are members of the HashSet. This includes the API notes
> above as well as the specs and API notes in LinkedHashSet.
@stuart-marks done.
-
PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v10]
On Wed, 25 May 2022 05:22:44 GMT, Stuart Marks wrote: > Yes, this test fails with IllegalAccessException. Probably it's easiest to > use a VarHandle to get private fields, similar to other usage already in this > test. > > This test case is a bit odd though in that it's supposed to test HashSet and > LinkedHashSet but it mostly actually tests HashMap. It creates the Set > instance and immediately extracts the HashMap, which is then passed to the > actual test, which operates directly on the HashMap. Considering about it being a whitebox test, it is not very weird, but still weird enough. A better way might be wrap it into another layer. Will try it when I have time. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v10]
On Wed, 25 May 2022 05:42:22 GMT, Stuart Marks wrote: > (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. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v10]
On Wed, 25 May 2022 03:02:45 GMT, ExE Boss wrote: >> XenoAmess has updated the pull request incrementally with one additional >> commit since the last revision: >> >> add test for newHashSet and newLinkedHashSet > > test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 360: > >> 358: throw new RuntimeException(e); >> 359: } >> 360: }) > > These probably need a `mapField.setAccessible(true)` call, or a `VarHandle` > for the `HashSet.map` field. Yes, this test fails with IllegalAccessException. Probably it's easiest to use a VarHandle to get private fields, similar to other usage already in this test. This test case is a bit odd though in that it's supposed to test HashSet and LinkedHashSet but it mostly actually tests HashMap. It creates the Set instance and immediately extracts the HashMap, which is then passed to the actual test, which operates directly on the HashMap. It would be preferable to create a Set; add an element (so that it's properly allocated); and then make assertions over the Set (which involve extracting the HashMap, etc.) It seems like there should be factoring that allows this sort of arrangement to be retrofitted without adding too much complication. Finally, please add "8284780" to the `@bug` line at the top of this test. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v10]
On Tue, 24 May 2022 21:37:52 GMT, XenoAmess wrote:
>> as title.
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> add test for newHashSet and newLinkedHashSet
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 line 398:
> 396: public static HashSet newHashSet(int numItems) {
> 397: return new HashSet<>(HashMap.calculateHashMapCapacity(numItems));
> 398: }
Please use "elements" instead of "items" throughout the specifications for the
objects that are members of the HashSet. This includes the API notes above as
well as the specs and API notes in LinkedHashSet.
-
PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v10]
On Tue, 24 May 2022 21:37:52 GMT, XenoAmess wrote:
>> as title.
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> add test for newHashSet and newLinkedHashSet
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 354:
> 352: rsc("rsls", size, cap, () -> {
> 353: try {
> 354: Field mapField =
> HashSet.class.getDeclaredField("map");
For clarity, consider using var handle/method handle and keep the reflection
code in the `WhiteBoxResizeTest` constructor.
-
PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v10]
On Tue, 24 May 2022 21:37:52 GMT, XenoAmess wrote: >> as title. > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > add test for newHashSet and newLinkedHashSet test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 360: > 358: throw new RuntimeException(e); > 359: } > 360: }) These probably need a `mapField.setAccessible(true)` call, or a `VarHandle` for the `HashSet.map` field. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v10]
> as title. XenoAmess has updated the pull request incrementally with one additional commit since the last revision: add test for newHashSet and newLinkedHashSet - Changes: - all: https://git.openjdk.java.net/jdk/pull/8302/files - new: https://git.openjdk.java.net/jdk/pull/8302/files/07e9b8b0..56d029f4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8302=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8302=08-09 Stats: 22 lines in 1 file changed: 21 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8302.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8302/head:pull/8302 PR: https://git.openjdk.java.net/jdk/pull/8302
