Re: RFR: 8186958: Need method to create pre-sized HashMap [v10]
On Wed, 6 Apr 2022 15:57:55 GMT, Alan Bateman wrote: > I suspect the core-libs label was added when you created it but you've > expanded it greatly since. Is there a way for making the bot re-calculate the labels? - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v10]
On Wed, 6 Apr 2022 02:38:17 GMT, Stuart Marks wrote:
>> XenoAmess has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> revert changes in jdk.compile
>
> src/java.base/share/classes/java/util/HashMap.java line 2556:
>
>> 2554: */
>> 2555: static int calculateHashMapCapacity(int numMappings) {
>> 2556: return (int) Math.ceil(numMappings / 0.75);
>
> Please use `(double) DEFAULT_LOAD_FACTOR` instead of `0.75`.
@stuart-marks done.
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v10]
On Wed, 6 Apr 2022 15:47:50 GMT, XenoAmess wrote: > I plan to include these changes to issue JDK-8186958, as I don't think it > better to flood about 50 P5 issues and do them seperately in every places. > > That would be a nightmare for reviewers. I didn't ask for 50 PRs, I just asked if you plan to separate the usages into a separate PR. As it stands, most of the areas that you've touched are not notified by this PR. I suspect the core-libs label was added when you created it but you've expanded it greatly since. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v10]
On Wed, 6 Apr 2022 00:54:41 GMT, Stuart Marks wrote: >> XenoAmess has updated the pull request incrementally with one additional >> commit since the last revision: >> >> revert changes in jdk.compile > > src/java.base/share/classes/java/util/LinkedHashMap.java line 792: > >> 790: >> 791: /** >> 792: * Creates a new, empty LinkedHashMap suitable for the expected >> number of mappings. > > Adjust wording here to read, > > Creates a new, empty, insertion-ordered LinkedHashMap suitable... > > I've used this wording in the CSR. @stuart-marks done. > I don't think we need this benchmark in this PR. @stuart-marks OK, that benchmark removed. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v10]
On Wed, 6 Apr 2022 14:27:45 GMT, Alan Bateman wrote: > The current patch touches usages all over the JDK. Is that for illustration > purposes or are you planning to include them with the methods? @AlanBateman I plan to include these changes to issue JDK-8186958, as I don't think it better to flood about 50 P5 issues and do them seperately in every places. That would be a nightmare for reviewers. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v10]
On Sat, 2 Apr 2022 22:46:19 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 jdk.compile The current patch touches usages all over the JDK. Is that for illustration purposes or are you planning to include them with the methods? - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v10]
On Sat, 2 Apr 2022 22:46:19 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 jdk.compile
src/java.base/share/classes/java/util/HashMap.java line 2556:
> 2554: */
> 2555: static int calculateHashMapCapacity(int numMappings) {
> 2556: return (int) Math.ceil(numMappings / 0.75);
Please use `(double) DEFAULT_LOAD_FACTOR` instead of `0.75`.
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v10]
On Wed, 6 Apr 2022 01:13:11 GMT, Stuart Marks wrote:
>> src/java.base/share/classes/java/util/LinkedHashMap.java line 804:
>>
>>> 802: * @since 19
>>> 803: */
>>> 804: public static LinkedHashMap newLinkedHashMap(int
>>> numMappings) {
>>
>> `LinkedHashMap` may be often extended for it has a `protected boolean
>> removeEldestEntry(Entry)`. Should we make a separate factory method for such
>> instances (with functional implementation) or just expose
>> `HashMap.calculateHashMapCapacity`?
>
> Good question. Having to subclass and override this method in order to
> provide a removal policy has always seemed rather clumsy. However, it's the
> supported approach, and it's done fairly frequently in the wild. A new
> subclass requires that the caller invoke `new` on that specific subclass,
> which in turn must choose which superclass constructor to call. This means
> that a static factory method can't be used. The alternatives would be to
> expose another constructor or to expose `calculateHashMapCapacity` as you
> suggest. A new constructor might also need to control the load factor and the
> ordering policy (insertion vs access order) so that's a fairly complicated
> overload to consider.
>
> Exposing the calculate method might help but that's mostly just a wrapper
> around a small computation. As we've seen it's easy to get this computation
> wrong, but exposing a method that _just_ does this computation seems like a
> really narrow case.
>
> (Still another alternative would be to pass a lambda expression that's called
> at the appropriate time. That would involve adding a `BiPredicate,
> Map.Entry>` to yet another constructor overload. This could work but it
> doesn't seem any simpler.)
>
> The need for subclassing LinkedHashMap and overriding this method might also
> be reduced by the addition of new APIs from the Sequenced Collections
> proposal (https://openjdk.java.net/jeps/8280836). One simply needs to call
> `pollFirstEntry` at the right time. That might remove the need to have some
> expiration policy plugged directly into the map itself.
>
> I'm not inclined to add more APIs to cover what seems to be a fairly narrow
> case, but we might keep this in mind to see if anything useful pops up.
True. My initial idea was to pass a lambda expression if we do make another
factory.
However, when I look at the constructor parameters, I realized that for those
evicting-queue-like use cases, it's more preferable for users to just call
new LinkedHashMap<>(cacheSize + 1, 1, true) {
protected boolean removeEldestEntry(Entry entry) {
return size() > cacheSize;
}
}
which preallocates the cache, avoids growth when the cache is full, and removes
by least-recently-used.
IMO this use case is better suited by the existing APIs (like the call above)
than a new factory method, especially that many libraries, like guava or
caffeine, offer more efficient caches.
So yes, this patch in the current state looks good. In the JDK, it seems there
are a few cases of linked hash map evicting by size that can have the map
initialized to be presized and avoid growth. But that's off-topic here.
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v10]
On Tue, 5 Apr 2022 23:16:57 GMT, liach wrote:
>> XenoAmess has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> revert changes in jdk.compile
>
> src/java.base/share/classes/java/util/LinkedHashMap.java line 804:
>
>> 802: * @since 19
>> 803: */
>> 804: public static LinkedHashMap newLinkedHashMap(int
>> numMappings) {
>
> `LinkedHashMap` may be often extended for it has a `protected boolean
> removeEldestEntry(Entry)`. Should we make a separate factory method for such
> instances (with functional implementation) or just expose
> `HashMap.calculateHashMapCapacity`?
Good question. Having to subclass and override this method in order to provide
a removal policy has always seemed rather clumsy. However, it's the supported
approach, and it's done fairly frequently in the wild. A new subclass requires
that the caller invoke `new` on that specific subclass, which in turn must
choose which superclass constructor to call. This means that a static factory
method can't be used. The alternatives would be to expose another constructor
or to expose `calculateHashMapCapacity` as you suggest. A new constructor might
also need to control the load factor and the ordering policy (insertion vs
access order) so that's a fairly complicated overload to consider.
Exposing the calculate method might help but that's mostly just a wrapper
around a small computation. As we've seen it's easy to get this computation
wrong, but exposing a method that _just_ does this computation seems like a
really narrow case.
(Still another alternative would be to pass a lambda expression that's called
at the appropriate time. That would involve adding a `BiPredicate,
Map.Entry>` to yet another constructor overload. This could work but it
doesn't seem any simpler.)
The need for subclassing LinkedHashMap and overriding this method might also be
reduced by the addition of new APIs from the Sequenced Collections proposal
(https://openjdk.java.net/jeps/8280836). One simply needs to call
`pollFirstEntry` at the right time. That might remove the need to have some
expiration policy plugged directly into the map itself.
I'm not inclined to add more APIs to cover what seems to be a fairly narrow
case, but we might keep this in mind to see if anything useful pops up.
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v10]
On Sat, 2 Apr 2022 22:46:19 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 jdk.compile I've drafted a CSR for this. Please review: https://bugs.openjdk.java.net/browse/JDK-8284377 I haven't yet reviewed all the call sites in detail, but I'll continue doing so. You're ambitious! But the number of call site errors that are fixed by this new API is quite high, so doing this is worthwhile. test/jdk/java/util/Collections/CalculateHashMapCapacityTestJMH.java line 1: > 1: /* I don't think we need this benchmark in this PR. Note, for future reference, benchmarks are located in the `test/micro` directory and not with the main regression suite in `test/jdk`. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v10]
On Sat, 2 Apr 2022 22:46:19 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 jdk.compile src/java.base/share/classes/java/util/LinkedHashMap.java line 792: > 790: > 791: /** > 792: * Creates a new, empty LinkedHashMap suitable for the expected > number of mappings. Adjust wording here to read, Creates a new, empty, insertion-ordered LinkedHashMap suitable... I've used this wording in the CSR. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v10]
On Sat, 2 Apr 2022 22:46:19 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 jdk.compile
src/java.base/share/classes/java/util/LinkedHashMap.java line 804:
> 802: * @since 19
> 803: */
> 804: public static LinkedHashMap newLinkedHashMap(int
> numMappings) {
`LinkedHashMap` may be often extended for it has a `protected boolean
removeEldestEntry(Entry)`. Should we make a separate factory method for such
instances (with functional implementation) or just expose
`HashMap.calculateHashMapCapacity`?
-
PR: https://git.openjdk.java.net/jdk/pull/7928
