Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v28]
On Thu, 10 Mar 2022 22:37:58 GMT, Roger Riggs wrote: > Heads up here! java.lang.Integer is specified as a value based class and > should not be used where identity is needed. > https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/doc-files/ValueBased.html > > I don't have a ready suggestion, but be forewarned that this use is not > considered valid. @RogerRiggs Sounds reasonable. Could you please have a look at the latest commit? I tried to make a class to use as a key, instead of using integer. - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v28]
On Thu, 10 Mar 2022 22:21:28 GMT, Stuart Marks wrote: > The difficulty with having a loop instead of constants is that the expected > value now needs to be computed. We could probably use `tableSizeFor` to do > this. But this is starting to get uncomfortably close to a testing > antipattern which is to use the code under test as part of the test. If a bug > is introduced into `tableSizeFor`, it could introduce the error into both the > actual value and the expected value, covering up the bug. (This is related to > one of the flaws with the Enum/ConstantDirectoryOptimalCapacity test.) Now we > _hope_ that `tableSizeFor` is correct and tested, but that verges on having > tests depend on each other in a subtle and uncomfortable way. you are correct about this. - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v28]
On Thu, 10 Mar 2022 22:01:49 GMT, Stuart Marks wrote:
>> @stuart-marks please have a look in changes in the latest commit, I think
>> we'd better to manually create references for keys like that.
>
> Good point about WeakHashMap! I don't think we need a separate table. Since
> the value is held by a strong reference, using the same reference as the key
> will prevent the key's weak reference from being cleared. I'd suggest this:
>
> for (int i = 0; i < n; i++) {
> Integer obj = i; // prevent WeakHashMap from clearing keys
> map.put(obj, obj);
> }
Heads up here!
java.lang.Integer is specified as a value based class and should not be used
where identity is needed.
https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/doc-files/ValueBased.html
I don't have a ready suggestion, but be forewarned that this use is not
considered valid.
-
PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v28]
On Thu, 10 Mar 2022 16:10:31 GMT, XenoAmess wrote: >> XenoAmess has updated the pull request incrementally with five additional >> commits since the last revision: >> >> - clean out tests >> - Remove 'randomness' keyword. >> - Cleanup and commenting. >> - initial rewrite of WhiteBoxResizeTest >> - Restore WhiteBoxResizeTest.java > > test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 290: > >> 288: cases.addAll(genPopulatedCapacityCases(12, 16)); >> 289: cases.addAll(genPopulatedCapacityCases(13, 32)); >> 290: cases.addAll(genPopulatedCapacityCases(64, 128)); > > @stuart-marks should there better be a loop from 1 to 64? The difficulty with having a loop instead of constants is that the expected value now needs to be computed. We could probably use `tableSizeFor` to do this. But this is starting to get uncomfortably close to a testing antipattern which is to use the code under test as part of the test. If a bug is introduced into `tableSizeFor`, it could introduce the error into both the actual value and the expected value, covering up the bug. (This is related to one of the flaws with the Enum/ConstantDirectoryOptimalCapacity test.) Now we _hope_ that `tableSizeFor` is correct and tested, but that verges on having tests depend on each other in a subtle and uncomfortable way. Recall that one of the original cases was that the table size computation (int) (numMappings / 0.75f) + 1 gives a value one too large: for 12 mappings it results in 17, giving a table size of 32. The (12, 16) case is sufficient to verify that this is fixed. Then the adjacent cases are there to make sure nothing weird is going on. (You could argue that (11, 16) isn't necessary.) I threw in the (64, 128) case because of the loop to 64, but it's not necessarily testing anything useful. If you think we need more cases we can add more at the boundaries, such as (24, 32) and (25, 64). - PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v28]
On Thu, 10 Mar 2022 16:22:29 GMT, XenoAmess wrote:
>> test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 116:
>>
>>> 114: }
>>> 115:
>>> 116: void putN(Map map, int n) {
>>
>> @stuart-marks well we know this is correct for WeakHashMap when n <
>> IntegerCache.high because we have Integer constant pool, but I think it be
>> better to manlly make it sure it is referenced, as IntegerCache.high is
>> configurable.
>
> @stuart-marks please have a look in changes in the latest commit, I think
> we'd better to manually create references for keys like that.
Good point about WeakHashMap! I don't think we need a separate table. Since the
value is held by a strong reference, using the same reference as the key will
prevent the key's weak reference from being cleared. I'd suggest this:
for (int i = 0; i < n; i++) {
Integer obj = i; // prevent WeakHashMap from clearing keys
map.put(obj, obj);
}
-
PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v28]
On Thu, 10 Mar 2022 16:15:03 GMT, XenoAmess wrote:
>> XenoAmess has updated the pull request incrementally with five additional
>> commits since the last revision:
>>
>> - clean out tests
>> - Remove 'randomness' keyword.
>> - Cleanup and commenting.
>> - initial rewrite of WhiteBoxResizeTest
>> - Restore WhiteBoxResizeTest.java
>
> test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 116:
>
>> 114: }
>> 115:
>> 116: void putN(Map map, int n) {
>
> @stuart-marks well we know this is correct for WeakHashMap when n <
> IntegerCache.high because we have Integer constant pool, but I think it be
> better to manlly make it sure it is referenced, as IntegerCache.high is
> configurable.
@stuart-marks please have a look in changes in the latest commit, I think we'd
better to manually create references for keys like that.
-
PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v28]
On Thu, 10 Mar 2022 16:13:42 GMT, XenoAmess wrote:
>> 8281631: HashMap copy constructor and putAll can over-allocate table
>
> XenoAmess has updated the pull request incrementally with five additional
> commits since the last revision:
>
> - clean out tests
> - Remove 'randomness' keyword.
> - Cleanup and commenting.
> - initial rewrite of WhiteBoxResizeTest
> - Restore WhiteBoxResizeTest.java
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 116:
> 114: }
> 115:
> 116: void putN(Map map, int n) {
@stuart-marks well we know this is correct for WeakHashMap when n <
IntegerCache.high because we have Integer constant pool, but I think it be
better to manlly make it sure it is referenced, as IntegerCache.high is
configurable.
test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 290:
> 288: cases.addAll(genPopulatedCapacityCases(12, 16));
> 289: cases.addAll(genPopulatedCapacityCases(13, 32));
> 290: cases.addAll(genPopulatedCapacityCases(64, 128));
@stuart-marks should there better be a loop from 1 to 64?
-
PR: https://git.openjdk.java.net/jdk/pull/7431
Re: RFR: 8281631: HashMap copy constructor and putAll can over-allocate table [v28]
> 8281631: HashMap copy constructor and putAll can over-allocate table XenoAmess has updated the pull request incrementally with five additional commits since the last revision: - clean out tests - Remove 'randomness' keyword. - Cleanup and commenting. - initial rewrite of WhiteBoxResizeTest - Restore WhiteBoxResizeTest.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/7431/files - new: https://git.openjdk.java.net/jdk/pull/7431/files/49aeef20..11a57a2d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7431&range=27 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7431&range=26-27 Stats: 845 lines in 6 files changed: 311 ins; 534 del; 0 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
