On Wed, 6 Apr 2022 01:13:11 GMT, Stuart Marks <sma...@openjdk.org> wrote:
>> src/java.base/share/classes/java/util/LinkedHashMap.java line 804:
>>
>>> 802: * @since 19
>>> 803: */
>>> 804: public static <K, V> LinkedHashMap<K, V> 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<K,V>,
> Map.Entry<K,V>>` 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<K, V> 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
