Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v33]
On Fri, 11 Mar 2022 19:40:39 GMT, Stuart Marks wrote: >> XenoAmess has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - change KeyStructure to String >> - fix test > > Regarding the number of test cases for `tableSizeForCases` and > `populatedCapacityCases`... the issue is not necessarily with execution time > (although if the test is slow it is a problem). The issues are more around: > Is this testing anything useful, and does this make sense to the reader? > > I think we can rely on the monotonicity of these functions. If populating a > map both with 49 and with 96 mappings results in a table length of 128, we > don't need to test that all the intermediate inputs also result in a table > length of 128. Including all the intermediate inputs makes the source code > more bulky and requires future readers/maintainers to hunt around in the long > list of tests to figure out which ones are significant. Really, the only ones > that are significant are the boundary cases, so just keep those. Adding more > tests that aren't relevant actually does hurt, even if they execute quickly. > So: please cut out all the extra test cases that aren't near the boundary > cases. > > I'm not sure what's going on with the build. The builds are in GitHub Actions > and they aren't necessarily reliable, so I wouldn't worry about them too > much. I'll run the final version through our internal build/test system > before integration. (I've also done this previously, and the results were > fine.) @stuart-marks should it mean I shall first rebase all commits? or it would be automatically sqruashed? - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v33]
On Fri, 11 Mar 2022 19:40:39 GMT, Stuart Marks wrote: > I think we can rely on the monotonicity of these functions. If populating a > map both with 49 and with 96 mappings results in a table length of 128, we > don't need to test that all the intermediate inputs also result in a table > length of 128. Including all the intermediate inputs makes the source code > more bulky and requires future readers/maintainers to hunt around in the long > list of tests to figure out which ones are significant. Really, the only ones > that are significant are the boundary cases, so just keep those. Adding more > tests that aren't relevant actually does hurt, even if they execute quickly. > So: please cut out all the extra test cases that aren't near the boundary > cases. what I worried is, the boundary this is based on the current table size calculating mechanic in HashMap. If people change the mechanic in HashMap, then the boundary would change. But well, this is a white box text for HashMap (and HashMap-like) classes after all, so maybe I'm just over overthinking too much. - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v33]
On Fri, 11 Mar 2022 19:40:39 GMT, Stuart Marks wrote: >> XenoAmess has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - change KeyStructure to String >> - fix test > > Regarding the number of test cases for `tableSizeForCases` and > `populatedCapacityCases`... the issue is not necessarily with execution time > (although if the test is slow it is a problem). The issues are more around: > Is this testing anything useful, and does this make sense to the reader? > > I think we can rely on the monotonicity of these functions. If populating a > map both with 49 and with 96 mappings results in a table length of 128, we > don't need to test that all the intermediate inputs also result in a table > length of 128. Including all the intermediate inputs makes the source code > more bulky and requires future readers/maintainers to hunt around in the long > list of tests to figure out which ones are significant. Really, the only ones > that are significant are the boundary cases, so just keep those. Adding more > tests that aren't relevant actually does hurt, even if they execute quickly. > So: please cut out all the extra test cases that aren't near the boundary > cases. > > I'm not sure what's going on with the build. The builds are in GitHub Actions > and they aren't necessarily reliable, so I wouldn't worry about them too > much. I'll run the final version through our internal build/test system > before integration. (I've also done this previously, and the results were > fine.) @stuart-marks all the changes to the test you requested at last review comments are done. please have a look, thanks! - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v33]
On Fri, 11 Mar 2022 19:09:25 GMT, Stuart Marks wrote:
>> XenoAmess has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - change KeyStructure to String
>> - fix test
>
> test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 61:
>
>> 59: String String = Integer.toString(i);
>> 60: map.put(String, String);
>> 61: }
>
> Small details here... the `String` variable should be lower case. But really
> this method should be inlined into `putN`, since that's the method that needs
> to generate mappings. Then, `makeMap` should call `putN`.
@stuart-marks done.
> test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 107:
>
>> 105: for (int i = 0; i < size; ++i) {
>> 106: putMap(map, i);
>> 107: }
>
> Replace this loop with a call to `putN`.
@stuart-marks done.
> test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 131:
>
>> 129: void putN(Map map, int n) {
>> 130: for (int i = 0; i < n; i++) {
>> 131: putMap(map, i);
>
> Inline `putMap` here.
@stuart-marks done.
> test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 317:
>
>> 315: public void defaultCapacity(Supplier> s) {
>> 316: Map map = s.get();
>> 317: putMap(map, 0);
>
> `map.put("", "")`
@stuart-marks done.
> test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 343:
>
>> 341: public void requestedCapacity(String label, int cap,
>> Supplier> s) {
>> 342: Map map = s.get();
>> 343: putMap(map, 0);
>
> No need to generate a key/value pair here, just use string literals, e.g.
> `map.put("", "")`.
@stuart-marks done.
> test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 430:
>
>> 428: map.putAll(makeMap(size));
>> 429: })
>> 430: );
>
> Wait, did this get reindented? Adding line breaks totally destroys the
> tabular nature of test data. Please restore. Yes, the lines end up being
> longer than the usual limit, but the benefit of having things in a nice table
> outweighs to cost of having the lines be somewhat over-limit.
@stuart-marks done.
-
PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v33]
On Fri, 11 Mar 2022 15:56:26 GMT, XenoAmess wrote:
>> 8281631: HashMap copy constructor and putAll can over-allocate table
>
> XenoAmess has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - change KeyStructure to String
> - fix test
Regarding the number of test cases for `tableSizeForCases` and
`populatedCapacityCases`... the issue is not necessarily with execution time
(although if the test is slow it is a problem). The issues are more around: Is
this testing anything useful, and does this make sense to the reader?
I think we can rely on the monotonicity of these functions. If populating a map
both with 49 and with 96 mappings results in a table length of 128, we don't
need to test that all the intermediate inputs also result in a table length of
128. Including all the intermediate inputs makes the source code more bulky and
requires future readers/maintainers to hunt around in the long list of tests to
figure out which ones are significant. Really, the only ones that are
significant are the boundary cases, so just keep those. Adding more tests that
aren't relevant actually does hurt, even if they execute quickly. So: please
cut out all the extra test cases that aren't near the boundary cases.
I'm not sure what's going on with the build. The builds are in GitHub Actions
and they aren't necessarily reliable, so I wouldn't worry about them too much.
I'll run the final version through our internal build/test system before
integration. (I've also done this previously, and the results were fine.)
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 61:
> 59: String String = Integer.toString(i);
> 60: map.put(String, String);
> 61: }
Small details here... the `String` variable should be lower case. But really
this method should be inlined into `putN`, since that's the method that needs
to generate mappings. Then, `makeMap` should call `putN`.
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 107:
> 105: for (int i = 0; i < size; ++i) {
> 106: putMap(map, i);
> 107: }
Replace this loop with a call to `putN`.
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 131:
> 129: void putN(Map map, int n) {
> 130: for (int i = 0; i < n; i++) {
> 131: putMap(map, i);
Inline `putMap` here.
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 317:
> 315: public void defaultCapacity(Supplier> s) {
> 316: Map map = s.get();
> 317: putMap(map, 0);
`map.put("", "")`
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 343:
> 341: public void requestedCapacity(String label, int cap,
> Supplier> s) {
> 342: Map map = s.get();
> 343: putMap(map, 0);
No need to generate a key/value pair here, just use string literals, e.g.
`map.put("", "")`.
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 430:
> 428: map.putAll(makeMap(size));
> 429: })
> 430: );
Wait, did this get reindented? Adding line breaks totally destroys the tabular
nature of test data. Please restore. Yes, the lines end up being longer than
the usual limit, but the benefit of having things in a nice table outweighs to
cost of having the lines be somewhat over-limit.
-
PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v33]
On Fri, 11 Mar 2022 15:56:26 GMT, XenoAmess wrote: >> 8281631: HashMap copy constructor and putAll can over-allocate table > > XenoAmess has updated the pull request incrementally with two additional > commits since the last revision: > > - change KeyStructure to String > - fix test  seems ci fails for network failure. @stuart-marks I refine the tests using String to replace KeyStructure. Please have a look. thanks! - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v33]
> 8281631: HashMap copy constructor and putAll can over-allocate table XenoAmess has updated the pull request incrementally with two additional commits since the last revision: - change KeyStructure to String - fix test - Changes: - all: https://git.openjdk.java.net/jdk/pull/7431/files - new: https://git.openjdk.java.net/jdk/pull/7431/files/9b9156c1..51871859 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7431&range=32 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7431&range=31-32 Stats: 133 lines in 1 file changed: 0 ins; 51 del; 82 mod Patch: https://git.openjdk.java.net/jdk/pull/7431.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7431/head:pull/7431 PR: https://git.openjdk.java.net/jdk/pull/7431
