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
Add Json.Folder #656
Add Json.Folder #656
Conversation
Codecov Report
@@ Coverage Diff @@
## master #656 +/- ##
==========================================
+ Coverage 83.68% 84.27% +0.58%
==========================================
Files 70 70
Lines 2090 2111 +21
Branches 150 146 -4
==========================================
+ Hits 1749 1779 +30
+ Misses 341 332 -9
Continue to review full report at Codecov.
|
|
||
@Benchmark | ||
def withFoldWith: Int = doc.foldWith( | ||
new Json.Folder[Int] { |
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.
One cool trick that I learned from Flavio Brasi is embedding this val
ed functions into their parent scope/class. Somewhat - merging interfaces.
new Json.Folder[Int] with ((Int, Json) => Int) {
def apply(i: Int, j: Json): Int = i + j.foldWith(this)
...
}
This will allocate less.
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.
👍, yeah, this is neat. It only has a tiny effect on allocations in this benchmark, but I think it looks a little nicer anyway, so let's consider it idiomatic.
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.
Has printing benchmark numbers changed as well?
@vkostyukov Yep—the change is pretty small with this PR, but I've got a follow-up coming with some bigger improvements (In particular I'm thinking of adding an optional setting for printers that lets them reuse string builders via thread locals, which has a pretty big impact). |
Nice @travisbrown! That sounds pretty cool - I'd probably be down to have that setting on by default in finch-circe. |
@vkostyukov Yeah, I think for most users it would make sense, but I'm wary of cases where it could cause problems in application servers, so it'll probably be behind a |
This change introduces a new way to fold
Json
values that's a little more verbose thanJson#fold
but is faster and involves fewer allocations. It also seems to perform better than pattern matching on the (package-private)Json
constructors in the benchmark included here:The allocation rates also look pretty good:
The benchmark is intended to test idiomatic use of the three styles.
The primary motivation for this change is #654—I'm adding a couple new
Json
constructors and would prefer to avoid using pattern matching even internally, but I don't want to pay the extra cost offold
. I believe it's a reasonable addition even apart from #654, though.I've also added
asNull
andwithNull
methods for the sake of consistency, since I've found myself wanting these a couple of times recently.After this addition the recommendation would continue to be something like "start with
fold
or theasX
orwithX
methods" but with a new "unless you know the performance cost is unacceptable, in which case there'sfoldWith
".