Copy ImmutableLinkedHashMap without rehashing or tuple allocation

[linyiru]

Pull Request Checklist

• Have you read through the contributor guidelines?
• Have you squashed your commits?
• Have you added copyright headers to new files? (no new files)
• Have you updated the documentation? (no API change)
• Have you added tests for any changed functionality?

Purpose

JsObject.+ / - (updated / removed on the backing ImmutableLinkedHashMap) copy the whole java.util.LinkedHashMap each time. That copy did two avoidable things:

• It was created with an initial capacity equal to the entry count, so at the 0.75 load factor it rehashed once while being filled.
• It was filled through the Scala tuple iterator (for ((k, v) <- this)), i.e. a Tuple2 plus withFilter/foreach closures per entry.

The copy is now sized with ceil(n / 0.75) for the final number of entries, and filled straight from entrySet(). The change is the same in the 2.12 and 2.13+ sources, and touches about 20 lines.

Background Context

Behaviour is unchanged: same entries, same order (overwriting a key keeps its position), same equals / hashCode.

Memory is unchanged too. The old code reached exactly the same final table size, just through an extra rehash. That's why updated checks containsKey: an overwrite has to be sized for n, not n + 1, or it would allocate a larger table than before. The existing JsonMemoryFootprintSpec exact-size assertions pass untouched. A new case checks that an object built with + takes the same memory as one built with JsObject(fields), for 1 to 20 fields.

I didn't use putAll: on JDK 17 HashMap.putMapEntries pre-sizes with s / 0.75 + 1 (fixed in 19 by JDK-8281631), which would make the footprint JDK-dependent.

++ is untouched. It doesn't go through this copy; the benchmark below confirms it's unchanged.

JMH, added to JsObjectBench: Scala 3.3.8, JDK 25, i7-1360P pinned to P-cores, -wi 8 -i 8 -f 2, ops/ms, 99.9% CI.

benchmark	size	before	after	speed-up
oPlusNewKey	1 / 4 / 15 / 30	21540 / 10549 / 3483 / 1945	31561 / 16808 / 6291 / 3271	1.47 / 1.59 / 1.81 / 1.68×
oPlusExistingKey	1 / 4 / 15 / 30	26031 / 12096 / 3494 / 1955	29615 / 16036 / 6059 / 3253	1.14 / 1.33 / 1.73 / 1.66×
oMinus	1 / 4 / 15 / 30	24088 / 11115 / 3563 / 1749	38032 / 18229 / 6484 / 3373	1.58 / 1.64 / 1.82 / 1.93×
oBuildByAdds (fold + from empty)	1 / 4 / 15 / 30	26473 / 4177 / 430 / 123	43174 / 6608 / 732 / 203	1.63 / 1.58 / 1.70 / 1.65×
oConcat	1 / 4 / 15 / 30	14079 / 9943 / 4337 / 2213	14099 / 9950 / 4440 / 2201	1.00 / 1.00 / 1.02 / 0.99×

All + / - / build intervals are disjoint; the oConcat ones overlap.

In an application: I found this profiling lichess. It builds JSON with chains of js + (k -> v), and the copy was 11–14% of the CPU of its tournament-list endpoints. With play-json 3.0.6 plus this patch (it applies cleanly to 3.0.x), an A/B of 4 interleaved rounds showed about 2% less CPU per request on those endpoints. That's within the run-to-run noise, so the clear win is at the JsObject level rather than end to end. The app's 586 tests and its JSON golden snapshots pass unchanged.

Checked locally: +play-jsonJVM/testFull on 2.12 / 2.13 / 3.3, +mimaReportBinaryIssues, validateCode, docs/testFull, and +play-jsonJS/Test/compile / +play-jsonNative/Test/compile. I didn't have Node or clang locally, so the JS and Native tests themselves are left to CI.

References

None. I didn't find an existing issue about this.

AI disclosure: I directed and verified this work; Claude Code (Claude Opus) assisted with the profiling, the patch, tests, benchmarks and this description.

[linyiru]

Note on the force-pushes: the first run had a formatting miss in the Scala 3 test (fixed). The second run failed one existing test on JDK 21 / Scala 3.3: JsonMemoryFootprintSpec › malicious › obj0, 7536 vs 7432 bytes. That test is parsing only, so it doesn't reach the changed copy path. It passed in the other Scala 3 jobs of the same run. I couldn't reproduce it locally with the same JDK (21.0.12.1, play-jsonJVM/testFull on main, impl-only and this branch). So I re-pushed the identical tree to re-run CI, and it's green now. It looks like that test can be flaky.

[cchantep]

It builds JSON with chains of js + (k -> v)

The current optimization makes each JsObject.+ cheaper, but a chain like:

js + (k1 -> v1) + (k2 -> v2) + ...

still copies the backing map for every append.

A JsObject builder could accumulate the fields in a mutable structure and materialize the immutable object once:

JsObject.builder()
  .add("a", 1)
  .add("b", 2)
  .add("c", 3)
  .result()

That would still require one copy. The important point is avoiding the repeated copies for such use case.

[linyiru]

A JsObject builder could accumulate the fields in a mutable structure and materialize the immutable object once

Agreed. A chain of + still copies once per append. This PR makes each copy cheaper, but it doesn't remove any of them. I think the two changes complement each other:

• A builder only helps code that is rewritten to use it. Code that already uses + / - gets faster with no changes. That includes conditional fields, like lila's add(key, Option) helpers that wrap +, and objects that come from parsing or from other libraries.
• play-json already has the builder: Json.newBuilder, from Add Json.newBuilder utility #876 (since 2.10.0). It materializes once.

While I was checking Json.newBuilder for this reply, I found a bug in it. It accumulates into Map.newBuilder[String, JsValue], so above 4 fields the result is backed by a Scala HashMap and the field order is lost. On the current main (Scala 3.3.8):

val b = Json.newBuilder
(1 to 8).foreach(i => b += (s"f$i" -> i))
Json.stringify(b.result())
// {"f7":7,"f6":6,"f1":1,"f8":8,"f5":5,"f3":3,"f4":4,"f2":2}   (underlying: immutable.HashMap)

Json.stringify(JsObject((1 to 8).map(i => s"f$i" -> JsNumber(i))))
// {"f1":1,"f2":2,"f3":3,"f4":4,"f5":5,"f6":6,"f7":7,"f8":8}

The existing test only uses 2 fields, and JsObject equality ignores order, so nothing caught it. Backing the builder with ImmutableLinkedHashMap.newBuilder would fix the order and drop the second copy (Map → JsObject). It needs some care, though: clear() and reuse after result() must not mutate an object that was already returned.

I'd like to keep this PR focused on the copy, and open that fix as a separate PR. It changes the observable behaviour of a public API, so it probably deserves its own review and a 3.0.x backport. Would that work for you, or would you rather have it here?

[linyiru]

Two updates:

• The Json.newBuilder order fix is now Keep the field order in Json.newBuilder #1482, so this PR stays scoped to the copy.
• be99adb fixes a bug in my own test here. The new JsonMemoryFootprintSpec case took its value by value. So it diffed a layout against itself and always compared 0 with 0. It now takes it by name, like assertSize, and requires a non-zero size. I checked it the other way too: with the copy deliberately sized 4× larger, the old version still passed, and the fixed one fails (1 fields: 184 did not equal 160). With the real code it passes for 1 to 20 fields, so the memory claim in the description still holds.

[cchantep]

LGTM

[mergify]

Queued — the merge queue status continues in this comment ↓.

[mergify]

Merge Queue Status

• ✅ Entered queue — 2026-10-07 13:47 UTC · Rule: default · triggered by @cchantep with the merge queue checkbox
• ✅ Checks skipped · PR is already up-to-date
• ✅ Merged — 2026-10-07 13:47 UTC · at 6801babadef6acfe9c2e50ba5e39e23ea1691fcc · merge

This pull request spent 20 seconds in the queue, including 2 seconds running CI.

Required conditions to merge

• github-review-approved [🛡 GitHub branch protection]
  • Copy ImmutableLinkedHashMap without rehashing or tuple allocation #1479
• any of [🛡 GitHub branch protection]:
  • check-success = Finish / Ready To Merge
  • check-neutral = Finish / Ready To Merge
  • check-skipped = Finish / Ready To Merge