On Mon, 30 Mar 2026 20:18:42 GMT, Stuart Marks <[email protected]> wrote:
>> src/java.base/share/classes/java/util/HashMap.java line 540:
>>
>>> 538: putMapEntries(hashMap, evict);
>>> 539: return;
>>> 540: }
>>
>> Same comment, add comments to indicate the two branches have same
>> code/bytecode, they differ in the method profiling data which makes a huge
>> difference.
>
> @jengebr I agree that some comments are indeed necessary. I'm tinkering with
> a way to simplify the special case logic, so don't worry about adding them
> just yet.
There are two special cases being handled separately here: adding from a
HashMap, and adding from an unmodifiable map that wraps a HashMap. The wrapped
case is handled above and separately from this code; they should be unified. In
addition, the above code modifies a method parameter, which is a bit of a code
smell, in that the meaning of the `m` parameter contains the superposition of
states in the intervening code. The good news is that it's not difficult to
unify the checks.
After trying several variations, I came up with the following:
Map<? extends K, ? extends V> hm;
if ((hm = m).getClass() == HashMap.class
|| m instanceof Collections.UnmodifiableMap<? extends K, ? extends
V> umap
&& (hm = umap.m).getClass() == HashMap.class) {
putMapEntries((HashMap<? extends K, ? extends V>) hm, evict);
return;
}
One might consider this dense, and indeed it is. But it does have a few
advantages: one boolean condition for handling the special case; a single
occurrence of the `putMapEntries` invocation and early return; and lack of
special-case state held in local variables across large portions of the method.
This uses the assign-to-local-within-a-conditional construct that's
unconventional but which is in common use in the code in this file. It takes
some getting used to but it makes sense, and it's hard to rewrite without
duplicating some code somewhere. This also uses `instanceof` instead of a
direct class check for the UnmodifiableMap class. This is a bit of a cheat to
declare and bind the `umap` local variable in a concise fashion. I happen to
know that any unmodifiable subclasses (sequenced, sorted, navigable) cannot
contain a HashMap... but this is a small coupling with the unmodifiable
hierarchy over in the Collections class. This check could be changed to a
direct `HashMap.class` check, at the cost of adding a cast in the next line. I
could go either way.
Finally, a brief comment here is warranted about the reason for special-casing
when the source is a HashMap. A mention of the bug report would be good too.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28243#discussion_r3033647895
