-
Notifications
You must be signed in to change notification settings - Fork 3
reboot - provide an unsafe faster implementation of json4s-ast? #9
Comments
I have pushed a test for the The runtime overhead appears to be around 30-40% in my trivial test, so there is definitely an overhead, which means there may be a stronger case for an unsafe ast |
I think the jawn-ast approach is mostly sound, with a couple caveats;
Argonaut's |
At first sight I don't really like having a safe and an unsafe AST version side by side. |
There are 3 ways to achieve this that I have in mind
My personal preference is 2, I have already create an very initial rudimentary implementation here (https://gist.github.com/mdedetrich/874dbac57bff9a93fa8c). Obviously this approach is going from one extreme to another, we have a very unsafe, but fast AST, and then we have not so fast (more accurately, fast unless you need to extract/compose |
This has been implemented in the latest pull request. Talked with @sirthias at length today on gitter, and long story short, I didn't see a way of implementing a People have 2 very distinct concepts in mind when they think about a |
After speaking with @eed3si9n, there may be impetus for an
unsafe
json4s-ast
which doesn't provide the same correctness guarantees as the currentjson4s-ast
, however will have much better performance.I personally don't have much issue with this, I suppose the only real contention is going from
json4s-ast
unsafe tojson4s-ast
current will have 2 methods, one that is safe (and will return anOption[JValue]
), and another which is unsafe, that will return aJValue
One example that @eed3si9n came up with, is storing the
JNumber
as a string internally (which is whatjawn
apprently does). This improves runtime performance, since it doesn't need to convert to aBigDecimal
first (which can be costly).I also have a suspicion that it may be the same reason why
JLong
/JInt
etc etc existed injson4s
, however we then get into the issues of side effects (due to possible failure)We could also just add
JLong
,JInt
etc etc types to our current implementation, but than that brings back problems that we may had earlier which we were trying to remove.@eed3si9n currently uses https://github.com/non/jawn for this reason (performance), we may look at their AST for inspiration, or maybe even just copy it 😛.
The text was updated successfully, but these errors were encountered: