On Wed, 1 Apr 2026 15:08:30 GMT, jengebr <[email protected]> wrote:
>> @stuart-marks I deliberately chose String to minimize the benchmark
>> overhead, but you are exactly right about both the blind spot and the impact
>> of copying the hashcode vs. recreating it. Good catches.
>
> A revised benchmark using a non-cached hashCode() shows even worse
> performance on the baseline cases, and has zero impact on the optimized case
> - thus highlighting the value of the change. I'll wait to update the PR and
> data until we have agreement on the details of the fastpath.
It doesn't add anything for the fast path method to be overloaded on its
argument type. Indeed, this is a potential footgun that I nearly fell into when
tinkering with this code. It's a bit too easy to accidentally call
`putMapEntries(Map)` recursively when one intends to call
`putMapEntries(HashMap)`. I'd suggest renaming this to `putHashMapEntries()`.
@liach raised a point about maintainability here and suggested using an
`entrySet` iteration instead. I think the performance disadvantage is clear.
Regarding maintainability, I observe that this is `HashMap` code depending only
on its own internals; it's not like it's depending on the internals of some
other class. There is also quite a bit of precedent for direct looping over the
internal data structures. See for example `containsValue`, `keysToArray`,
`replaceAll`, and the view collections' `forEach` methods and the spliterators'
`forEachRemaining` methods.
The approach for the direct loops in those methods looks something like this,
though there are variations:
Node<K,V>[] tab;
if (size > 0 && (tab = table) != null) {
for (Node<K,V> e : tab) {
for (; e != null; e = e.next) {
// ...
}
}
}
The assignment of the local variable `tab` is pretty idiosyncratic but is
typical for the code in this file, as is this style of for-loop used in the
inner loop. Also note the checks for both size being greater than zero and the
table being non-null. I guess this handles the case where the size is zero but
the table is non-null. Not sure how important this is, but this check is done
in all the other places. I'd suggest copying this loop form from the other
locations. What it's doing will be quite recognizable instead of being
gratuitously different. This in turn improves maintainability.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28243#discussion_r3033605185
