-
-
Notifications
You must be signed in to change notification settings - Fork 134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove JsObject lazy vals #701
Conversation
if (!found) o ++ JsObject(Seq(this.key -> transform(JsObject.empty))) | ||
else o | ||
val transformed = transform(obj.underlying.getOrElse(key, JsObject.empty)) | ||
obj + (key -> transformed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that our underlying LinkedHashMap
correctly preserves order on +
for existing keys.
scala> lhm
val res3: java.util.LinkedHashMap[String, Int] = {1=1, 2=2, 3=3}
scala> lhm.put("2", 2222222)
val res4: Int = 2
scala> lhm
val res5: java.util.LinkedHashMap[String, Int] = {1=1, 2=2222222, 3=3}
@gmethvin, WDYT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think given the optimizations you've done, it's probably not worth having these lazy vals anymore.
Purpose
Reduce memory footprint by removing the (now unnecessary) lazy vals from
JsObject
.Background Context
Many of our production OOME heap dumps show
JsValue
consuming the majority of memory. I've seen scenarios where there are either too many objects (batching, parallelism), unexpectedly large objects (services returning unwanted data), or both. Regardless of other possible mitigations, it would be nice to reduce the memory footprint ofJsValue
.In my previous PRs, I migrated most remaining internal usage off of the two lazy vals in JsObject.
lazy val value: Map[String, JsValue]
is now completely avoided, andJsObject
provides an immutable underlying Map by default.jsObject.value
toImmutableLinkedHashMap
to mitigate hash collisions #675lazy val fields: Seq[(String, JsValue)]
- is no longer used for hashCode/equals, and the last remaining uses don't seem to benefit from having an additional memoizedSeq
floating around.JsObject.equals()
/hashCode()
#692Caveats
The only drawbacks I can think of are in situations:
mutable.Map
intoJsObject
, in which case, they can restore the memoization ofvalue
(which isn't used internally anyway) by wrapping it in an immutable/unmodifiable wrapper.fields
(no longer used internally) more than once, but it's difficult to think of use cases for this. If desired,override val fields = super.fields
.While these (rare?) situations can be mitigated, the memory footprint of
JsObject
cannot be fixed in user space.Benefits
JsObject
: 32 bytes => 16 bytesvalue
is gone which usually results in a ~5% improvement on accessClassLayout
Before:
After: