Skip to content
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

Optimize JsObject.equals()/hashCode() #692

Merged
merged 1 commit into from
Jan 19, 2022

Conversation

htmldoug
Copy link
Contributor

@htmldoug htmldoug commented Jan 9, 2022

Summary

For JsObject.equals()/hashCode():

  • improves runtime by avoiding making two copies of the underlying data
  • improves memory footprint by avoiding holding lazy val fields: collection.Seq[(String, JsValue)] after the computation is performed.
  • Benchmark: jmh results

@htmldoug htmldoug changed the title Optimize JsObject.equals()/hashCode() Optimize JsObject.equals()/hashCode() Jan 9, 2022
case _ => false
override def equals(other: Any): Boolean = {
other match {
case o: AnyRef if this.eq(o) =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How could this happen?

Copy link
Contributor Author

@htmldoug htmldoug Jan 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! As an illustrative example, consider a json text that looks like [[{"k":0}], [{"k":0}],[{"k":0}],[{"k":0}],...] whose length approaches the jvm's total heap size. Json.parse(inputStream) would throw OutOfMemoryError, but is it possible to represent such a text as a JsValue?

Currently, jackson-core will return the same "k" String instance for all keys in the example, but that's only a small part of the eventual JsValue memory footprint. It's possible to apply the same technique to the other duplicated values as well.

In weepickle terms, a minimal implementation could look like:

val jsValue = FromJson(inputStream).transform(new DdVisitor(PlayJson))

class DdVisitor[T, J](underlying: Visitor[T, J])
 (implicit cache: mutable.Map[Any, Any] = mutable.Map.empty)
 extends Visitor.Delegate[T, J](underlying)
   with RecursiveDelegate[T, J] {

 override def asRecursiveDelegate[A, B](visitor: Visitor[A, B]) =
   new DdVisitor(
     visitor.map { jsValue =>
       cache
         .getOrElseUpdate(jsValue, jsValue)
         .asInstanceOf[B]
     }
   )
}

As we walk the input depth-first, every JsValue node is run through a Map[JsValue, JsValue] to return a canonical instance. Each {"k":0} would result in a cache hit and return a single canonical instance.

Back to your question, this line would be hit when we attempt to deduplicate [{"k":0}]. In order to check if two JsArray are identical, it needs to test that all elements pass equals(). Since the {"k":0} JsObject that's present in both arrays would have already been canonicalized to the same instance, this eq other would be true for that comparison.

I wish this were a purely hypothetical scenario. In practice, we talk to some APIs that return duplication-heavy JSON texts that have OOME'd our apps before. My favorite example was a 5.4 GB JsObject in heap whose UTF-8 input compressed down to 1.7 MB. Those units are not typos. That's ~3,000x memory amplification.

One of the things we like about play-json is that it gives us the flexibility to mitigate these problems by extending the JsValue classes. Applying deduplication brought the 5.4 GB JsValue down to a 205 MB JsValue. Bringing our own Map/Seq and doing some unboxing got the same input down to a 23 MB JsValue. Throughput was also faster due to eliminated GC pressure. Win-win. (We also logged an issue to please stop sending us info we don't need, but I was on-call and wanted to stop getting paged about this.)

Some APIs are highly normalized and duplicate-free, but in practice, I suspect many of them contain significant duplication. Using the few thousand test json files in our test double service as a proxy for the real-world data we deal with, I found that 71% of the nodes were present somewhere else in the same json text. Duplication wasn't present only in primitives either. I found significant duplication up to height=5 of the trees. I'm very curious if we just deal with bad REST APIs or if this is more widespread.

Anyway, over typical usage this line won't hit, although it's quite cheap, especially compared to the previous implementation that constructed two copies of underlying--one of them a HashSet! If needed, we could remove it and get by with the underlying Map performing the eq check, but while I was benchmarking, I saw the opportunity to short-circuit and took it.

Thanks for the early feedback on this draft! Code is actually ready for review, but I didn't want to send reviewer notifications until I'd written the PR description. Now that I've typed all this out, maybe I'll just skip it. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

K, would be interested to add a benchmark after.

Copy link
Contributor Author

@htmldoug htmldoug Jan 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before:

Benchmark                    (size)   Mode  Cnt    Score    Error   Units
JsObjectBench.oEqualsCopy        15  thrpt    5  290.695 ±  4.548  ops/ms
JsObjectBench.oEqualsEq          15  thrpt    5  300.135 ± 10.104  ops/ms
JsObjectBench.oEqualsNe          15  thrpt    5  289.910 ± 17.405  ops/ms
JsObjectBench.oHashCodeWarm      15  thrpt    5  521.727 ± 40.900  ops/ms

After:

Benchmark                    (size)   Mode  Cnt       Score       Error   Units
JsObjectBench.oEqualsCopy        15  thrpt    5    4386.757 ±   124.675  ops/ms
JsObjectBench.oEqualsEq          15  thrpt    5  430769.750 ± 43891.912  ops/ms
JsObjectBench.oEqualsNe          15  thrpt    5  321636.207 ± 23899.623  ops/ms
JsObjectBench.oHashCodeWarm      15  thrpt    5    4024.849 ±   276.276  ops/ms

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@htmldoug htmldoug marked this pull request as ready for review January 9, 2022 19:53
Copy link
Member

@gmethvin gmethvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to me, and a nice performance improvement!

Copy link
Member

@cchantep cchantep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge ready?

@htmldoug
Copy link
Contributor Author

htmldoug commented Jan 12, 2022

I was thinking #675 first if you want to see the memory footprint drop. Otherwise, ready.

@cchantep
Copy link
Member

See throughput bench, if mem footprint bench is a quickwin ...

@htmldoug
Copy link
Contributor Author

Sorry, I don't follow. Is there an existing test you want me to look at?

The memory footprint tests I'm referring to are here. The benefits are less about throughput and more about preventing OOME conditions over large or many inputs that have to be in memory at once.

@cchantep
Copy link
Member

Fine for me

@mergify mergify bot merged commit 4f7724e into playframework:main Jan 19, 2022
@htmldoug htmldoug deleted the fast-JsObject branch January 19, 2022 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants