Add JsonBufferBuilder. #51
Conversation
| @@ -0,0 +1,353 @@ | |||
| // Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file | |||
There was a problem hiding this comment.
I ran out of steam a bit by the time I got to this one admittedly, but I can take a closer look later 🤣
There was a problem hiding this comment.
That's a happy coincidence as I need to take another pass at this one :) will do so next, so please expect another commit with additions/tweaks here after these comments.
|
Definitely liking the general direction here - lots of areas to squeeze out more performance later on too if desired. I think my main concern overall is we are doing so much work lazily we are hiding a lot of costs and moving them to places that aren't necessarily expected, in the name of "free" deserialization. We should probably be thinking about benchmarks that actually use these deserialized objects in "reasonable" ways, including reading the same fields multiple times, etc. |
There was a problem hiding this comment.
Thanks :)
All replied, and pushed an additional commit instead of breaking everything this time.
You commented that there aren't enough comments on TypedMap and I agree, I didn't get around to taking another look at TypedMap either, I will do that next :)
Feel free to wait for next commit before taking another look, or to look at the comments+updates I'm sending now, as you like.
| /// Set to an instance of [Explanations] to turn on logging, reset to null to | ||
| /// turn off logging. If set, [JsonBufferBuilder#toString] will print | ||
| /// additional information for each byte. | ||
| Explanations? explanations; |
There was a problem hiding this comment.
We could do it with an environment variable so it is const - and then if tests fail you could just set the environment variable to get better output (and maybe we would suggest doing this)?
I don't mind a global if it isn't mutable :)
Made any classes and methods that can be private, private.
davidmorgan
force-pushed
the
json-buffer-builder
branch
from
6b0584d to
c2c2b2f
Compare
| /// Set to an instance of [Explanations] to turn on logging, reset to null to | ||
| /// turn off logging. If set, [JsonBufferBuilder#toString] will print | ||
| /// additional information for each byte. | ||
| Explanations? explanations; |
There was a problem hiding this comment.
Environment const is a good idea, thanks, done; moved it to a member of JsonBufferBuilder and made everything private.
I couldn't get the bool to work right with default "dart test", only "dart test -c source", so I put that in the instructions.
| /// | ||
| /// Then, [JsonBufferBuilder#toString] prints additional information for | ||
| /// each byte. | ||
| final _Explanations? _explanations = |
There was a problem hiding this comment.
This should be static yeah?
There was a problem hiding this comment.
Class local is a bit neater for the tests, and it doesn't seem to make a difference in the benchmarks, i.e. performance seems to be the same.
| /// Then, [JsonBufferBuilder#toString] prints additional information for | ||
| /// each byte. | ||
| final _Explanations? _explanations = | ||
| const bool.fromEnvironment('debug_json_buffer') ? _Explanations() : null; |
There was a problem hiding this comment.
I was originally thinking system environment variables not -D ones, but I see now that the former can't be const.
We could file an issue on package:test to support these, fwiw. Not sure we would actually implement it, there could be some pushback, but it might be worth thinking about.
There was a problem hiding this comment.
Thanks, filed dart-lang/test#2272
New alternative implementation for
JsonBuffer, still WIP, will send an email about status.