Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Thu, 14 Apr 2022 18:32:03 GMT, Naoto Sato wrote: >>> Good point! Filed an issue: https://bugs.openjdk.java.net/browse/JDK-8284856 >> >> @stuart-marks @naotoj I can help solve JDK-8284856 after this pr. But >> usually we only solve 1 issue in 1 pr, so I think it's better to wait after >> this. > > Thanks. Will fix it myself, as I want to eliminate that immediate value in > the code. PR opened, based on this PR. https://github.com/openjdk/jdk/pull/8253 - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Thu, 14 Apr 2022 17:06:53 GMT, XenoAmess wrote: >> Good point! Filed an issue: https://bugs.openjdk.java.net/browse/JDK-8284856 > >> Good point! Filed an issue: https://bugs.openjdk.java.net/browse/JDK-8284856 > > @stuart-marks @naotoj I can help solve JDK-8284856 after this pr. But usually > we only solve 1 issue in 1 pr, so I think it's better to wait after this. Thanks. Will fix it myself, as I want to eliminate that immediate value in the code. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:53:15 GMT, Naoto Sato wrote: > Good point! Filed an issue: https://bugs.openjdk.java.net/browse/JDK-8284856 @stuart-marks @naotoj I can help solve JDK-8284856 after this pr. But usually we only solve 1 issue in 1 pr, so I think it's better to wait after this. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 23:25:47 GMT, Stuart Marks wrote:
>> XenoAmess has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update LastModified
>
> src/java.base/unix/classes/java/lang/ProcessEnvironment.java line 102:
>
>> 100: /* Only for use by Runtime.exec(...String[]envp...) */
>> 101: static Map emptyEnvironment(int capacity) {
>> 102: return new StringEnvironment(HashMap.newHashMap(capacity));
>
> This change is correct, since this is called with the length of an array
> that's used to populate the environment. It really should be named `size`
> instead of `capacity`. However the windows version of this code simply calls
> `super(capacity)` as it's a subclass of `HashMap`, which is wrong. Well,
> probably not worth changing this now. We may need to revisit this later to do
> some cleaning up. (And also the strange computation in the static initializer
> at line 71.)
@stuart-marks OK, changes on this class reverted.
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Thu, 14 Apr 2022 03:38:52 GMT, Joe Wang wrote: >>> I suspect the `size*2+1` was a failed attempt at allocating a HashMap of >>> the correct capacity for `size` mappings. >> >> I looked the codes and don't think so.. >> If I'm wrong, I'm glad to fix. > > Stuart's right, I looked at the code, it's as you said a failed attempt, > "size" would be good. So HashMap.newHashMap(size) would actually be a small > improvement. > > It's an interesting impl the way it used HashMap, but it's 20 year code. @JoeWang-Java @stuart-marks got it. done. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:20:14 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > update LastModified LGTM. - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Thu, 14 Apr 2022 01:13:18 GMT, XenoAmess wrote:
>> src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/xs/traversers/XSAttributeChecker.java
>> line 1819:
>>
>>> 1817: Map items;
>>> 1818: LargeContainer(int size) {
>>> 1819: items = HashMap.newHashMap(size*2+1);
>>
>> I'm wondering if we should change this to just `newHashMap(size)` since it
>> looks like this container is intended to hold up to `size` items.
>> @JoeWang-Java ? I suspect the `size*2+1` was a failed attempt at allocating
>> a HashMap of the correct capacity for `size` mappings.
>
>> I suspect the `size*2+1` was a failed attempt at allocating a HashMap of the
>> correct capacity for `size` mappings.
>
> I looked the codes and don't think so..
> If I'm wrong, I'm glad to fix.
Stuart's right, I looked at the code, it's as you said a failed attempt, "size"
would be good. So HashMap.newHashMap(size) would actually be a small
improvement.
It's an interesting impl the way it used HashMap, but it's 20 year code.
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Thu, 14 Apr 2022 01:15:05 GMT, XenoAmess wrote: >> src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/dom/DocumentCache.java >> line 171: >> >>> 169: _current = 0; >>> 170: _size = size; >>> 171: _references = HashMap.newHashMap(_size); >> >> Not `_size+2` ? > >> Not `_size+2` ? > > I don't have a idea here why he original use the + 2. > Is there any guy more familiar with this code tell me why? > Thanks! size, not size + 2, same situation as size*2+1 below. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:57:33 GMT, Stuart Marks wrote: > Not `_size+2` ? I don't have a idea here why he original use the + 2. Is there any guy more familiar with this code tell me why? Thanks! > I suspect the `size*2+1` was a failed attempt at allocating a HashMap of the > correct capacity for `size` mappings. I looked the codes and don't think so.. If I'm wrong, I'm glad to fix. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:20:14 GMT, XenoAmess wrote:
>> 8186958: Need method to create pre-sized HashMap
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update LastModified
src/java.base/unix/classes/java/lang/ProcessEnvironment.java line 102:
> 100: /* Only for use by Runtime.exec(...String[]envp...) */
> 101: static Map emptyEnvironment(int capacity) {
> 102: return new StringEnvironment(HashMap.newHashMap(capacity));
This change is correct, since this is called with the length of an array that's
used to populate the environment. It really should be named `size` instead of
`capacity`. However the windows version of this code simply calls
`super(capacity)` as it's a subclass of `HashMap`, which is wrong. Well,
probably not worth changing this now. We may need to revisit this later to do
some cleaning up. (And also the strange computation in the static initializer
at line 71.)
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:20:14 GMT, XenoAmess wrote:
>> 8186958: Need method to create pre-sized HashMap
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update LastModified
src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/xs/traversers/XSAttributeChecker.java
line 1819:
> 1817: Map items;
> 1818: LargeContainer(int size) {
> 1819: items = HashMap.newHashMap(size*2+1);
I'm wondering if we should change this to just `newHashMap(size)` since it
looks like this container is intended to hold up to `size` items. @JoeWang-Java
? I suspect the `size*2+1` was a failed attempt at allocating a HashMap of the
correct capacity for `size` mappings.
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:20:14 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > update LastModified src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/dom/DocumentCache.java line 171: > 169: _current = 0; > 170: _size = size; > 171: _references = HashMap.newHashMap(_size); Not `_size+2` ? - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:40:38 GMT, Stuart Marks wrote:
>> XenoAmess has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update LastModified
>
> src/java.base/share/classes/java/lang/Character.java line 8574:
>
>> 8572: private static final HashMap
>> aliases;
>> 8573: static {
>> 8574: aliases = HashMap.newHashMap(162);
>
> @naotoj Seems like this magic number is likely to go out of date. Should
> there be a test for it like the one you updated for NUM_ENTITIES?
> [JDK-8283465](https://bugs.openjdk.java.net/browse/JDK-8283465).
Good point! Filed an issue: https://bugs.openjdk.java.net/browse/JDK-8284856
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:20:14 GMT, XenoAmess wrote:
>> 8186958: Need method to create pre-sized HashMap
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update LastModified
src/java.base/share/classes/java/lang/Character.java line 8574:
> 8572: private static final HashMap
> aliases;
> 8573: static {
> 8574: aliases = HashMap.newHashMap(162);
@naotoj Seems like this magic number is likely to go out of date. Should there
be a test for it like the one you updated for NUM_ENTITIES?
[JDK-8283465](https://bugs.openjdk.java.net/browse/JDK-8283465).
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
> 8186958: Need method to create pre-sized HashMap XenoAmess has updated the pull request incrementally with one additional commit since the last revision: update LastModified - Changes: - all: https://git.openjdk.java.net/jdk/pull/7928/files - new: https://git.openjdk.java.net/jdk/pull/7928/files/4476c761..d110ecfd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7928=17 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7928=16-17 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7928.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7928/head:pull/7928 PR: https://git.openjdk.java.net/jdk/pull/7928
