Improve jsoniter encoding performance #2181
Conversation
| @@ -5,6 +5,7 @@ import caliban._ | |||
| import caliban.parsing.adt.LocationInfo | |||
| import com.github.plokhotnyuk.jsoniter_scala.core._ | |||
|
|
|||
| import scala.annotation.nowarn | |||
There was a problem hiding this comment.
Oops, dev leftover
| case v: FloatValue.FloatNumber => out.writeVal(v.value) | ||
| case v: FloatValue.DoubleNumber => out.writeVal(v.value) | ||
| case v: FloatValue.BigDecimalNumber => out.writeVal(v.value) | ||
| case v => out.writeVal(v.toString) |
There was a problem hiding this comment.
This is going to bite us some day 🥲
Couldn't we group the remaining IntValue (and FloatValue) into one case and do a second pattern matching?
There was a problem hiding this comment.
I was thinking the same thing, but then I thought it's very unlikely we'll add more ResponseValue types. But the more I think of it, I think you're right, better be safe than sorry.
There was a problem hiding this comment.
I tried a couple of different ways in order to fully preserve type-safety, and the only feasible way to do it was to have a "full encoder" to fallback to. Encoding non-common types won't be too great performance-wise, but at least we get really good performance when for common types, and we get to preserve type-safety
There was a problem hiding this comment.
What's the issue with
case v: IntValue => some method that only pattern match on IntValue
case v: FloatValue => some method that only pattern match on FloatValueat the end and no case _ =>?
We can still keep IntNumber at the top.
There was a problem hiding this comment.
So the issue with IntValue is that both IntNumber and LongNumber are commonly used (LongNumber is used a fair bit in some wrappers like ApolloTracing), so handling it generically doesn't make sense cause the only case left is BigIntNumber.
I ended up handling only FloatValue generically, while leaving DoubleNumber in the main pattern match cause I think that's something that's somewhat commonly used. Running the benchmarks shows that we're still OK with the number of cases that we have this way
| case v: IntValue.LongNumber => out.writeVal(v.value) | ||
| case v: IntValue.BigIntNumber => out.writeVal(v.value) | ||
| case v: FloatValue.FloatNumber => out.writeVal(v.value) | ||
| case v: FloatValue.DoubleNumber => out.writeVal(v.value) |
There was a problem hiding this comment.
Shouldn’t we put the cases that are not included in the fast encoder first? Or does it not make a difference to the compiler?
There was a problem hiding this comment.
I ended up only handling FloatValue in a separate method, seems that the number of cases is still low enough to get the ~20% improvement
TIL that the number of cases in when pattern-matching affects performance in an interesting way:
I'm not sure exactly why this happens, but my guess is that the JVM manages to JIT/inline the pattern matching when the number of cases is relatively low.
Unfortunately, the only way to keep the number of cases below this threshold (12 type-checked cases in our case) was to remove the type-checking on StreamValue and use the
.toStringmethod. This should work OK as long as we don't need to extend ResponseValue with another kind of value, and forget to add it to the pattern matching. Given that the chances of adding another kind of ResponseValue type are very very small, I think this approach should be okayWith these changes, we see ~20% improvement in throughput when encoding
ResponseValues 🎉 🚀PS: I also noticed that we were parsing floats into a
BigDecimalNumberfor all floats, so I changed it to first try and parse them into aDoublewhich is more memory efficient, and fallback toBigDecimalNumberonly in case of an error