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 [v16]
On Wed, 13 Apr 2022 23:48:06 GMT, Stuart Marks wrote: > but I suspect the cleanup may simply be removing them entirely. +1 for removing it. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 20:06:34 GMT, Naoto Sato wrote: >> XenoAmess has updated the pull request incrementally with one additional >> commit since the last revision: >> >> revert changes in: >> src/java.desktop >> src/java.management >> src/jdk.internal.vm.ci >> src/jdk.jfr >> src/jdk.management.jfr >> src/jdk.management >> src/utils/IdealGraphVisualizer > > Looks good for changes in i18n related call sites, assuming that the > copyright years will be updated. > > Should we need some additions/modifications to the hashmap optimal capacity > utility `test/lib/jdk/test/lib/util/OptimalCapacity.java`? @naotoj wrote: > Should we need some additions/modifications to the hashmap optimal capacity > utility `test/lib/jdk/test/lib/util/OptimalCapacity.java`? The only thing that uses this utility now is `test/jdk/java/lang/Enum/ConstantDirectoryOptimalCapacity.java`, which is on the problem list. The cleanup is covered by [JDK-8282120](https://bugs.openjdk.java.net/browse/JDK-8282120). After this PR gets integrated, the Enum ConstantDirectory will be initialized with `HashMap.newHashMap(universe.length)` so the OptimalCapacity test won't really be testing anything. I'll take another look at it and the library utility, but I suspect the cleanup may simply be removing them entirely. - 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
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 20:06:34 GMT, Naoto Sato wrote: >> XenoAmess has updated the pull request incrementally with one additional >> commit since the last revision: >> >> revert changes in: >> src/java.desktop >> src/java.management >> src/jdk.internal.vm.ci >> src/jdk.jfr >> src/jdk.management.jfr >> src/jdk.management >> src/utils/IdealGraphVisualizer > > Looks good for changes in i18n related call sites, assuming that the > copyright years will be updated. > > Should we need some additions/modifications to the hashmap optimal capacity > utility `test/lib/jdk/test/lib/util/OptimalCapacity.java`? @naotoj @JoeWang-Java copyright updated to 2022. LastModified updated to 2022. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v17]
> 8186958: Need method to create pre-sized HashMap XenoAmess has updated the pull request incrementally with one additional commit since the last revision: Copyright latest year to 2022 - Changes: - all: https://git.openjdk.java.net/jdk/pull/7928/files - new: https://git.openjdk.java.net/jdk/pull/7928/files/2f5617bb..4476c761 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7928=16 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7928=15-16 Stats: 24 lines in 24 files changed: 0 ins; 0 del; 24 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
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 16:29:11 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: > > revert changes in: > src/java.desktop > src/java.management > src/jdk.internal.vm.ci > src/jdk.jfr > src/jdk.management.jfr > src/jdk.management > src/utils/IdealGraphVisualizer The ZipFS and Jar changes seem OK - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 16:29:11 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: > > revert changes in: > src/java.desktop > src/java.management > src/jdk.internal.vm.ci > src/jdk.jfr > src/jdk.management.jfr > src/jdk.management > src/utils/IdealGraphVisualizer Looks good for the changes in java.xml, like Naoto, assuming copyright years and the LastModified tag are updated. I generally prefer for the source level to stay at 8 as the upstream source is maintained at an older JDK level, but these changes seem to be small enough to tolerate. - Marked as reviewed by joehw (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 16:29:11 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: > > revert changes in: > src/java.desktop > src/java.management > src/jdk.internal.vm.ci > src/jdk.jfr > src/jdk.management.jfr > src/jdk.management > src/utils/IdealGraphVisualizer Looks good for changes in i18n related call sites, assuming that the copyright years will be updated. Should we need some additions/modifications to the hashmap optimal capacity utility `test/lib/jdk/test/lib/util/OptimalCapacity.java`? - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 16:29:11 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: > > revert changes in: > src/java.desktop > src/java.management > src/jdk.internal.vm.ci > src/jdk.jfr > src/jdk.management.jfr > src/jdk.management > src/utils/IdealGraphVisualizer Reviewers for i18n, net, nio, and security, please review call site changes in your areas. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/7928
