Split JsonWriter into UTF-8 and UTF-16 specific types.#2292
Conversation
| { | ||
| _arrayFormatter.Clear(); | ||
| WriterSystemTextJsonBasic(Formatted, _arrayFormatter); | ||
| if (Target == EncoderTarget.InvariantUtf8) |
There was a problem hiding this comment.
@adamsitnik, please review the changes in JsonWriterPerf.cs.
Is there a way for me to avoid adding this if check? It doesn't have much impact on the end result, though.
There was a problem hiding this comment.
The extra branch should not impact this particular benchmark (it's not a nano-benchmark).
To avoid this check you would have to remove EncoderTarget param and write a benchmark method per case
adamsitnik
left a comment
There was a problem hiding this comment.
the benchmark changes look good to me! 👍
| { | ||
| _arrayFormatter.Clear(); | ||
| WriterSystemTextJsonBasic(Formatted, _arrayFormatter); | ||
| if (Target == EncoderTarget.InvariantUtf8) |
There was a problem hiding this comment.
The extra branch should not impact this particular benchmark (it's not a nano-benchmark).
To avoid this check you would have to remove EncoderTarget param and write a benchmark method per case
|
May I suggest looking into moving the pretty print part out of the writing methods?
|
How would that work for writing to a stream, etc.? We can't have a post-processor, if we are sending the bytes as we write them. Given the fact that the pretty print version has relatively low cost, it might not be worth it (looking at the delta between Formatted = false vs true). Still, I will file an issue because that is an interesting optimization we can investigate. If you are interested in giving it a shot, go for it :) We need to expand the Writer Perf tests to include more variety of json writing scenarios (similar to the JsonReader, for instance heavy nesting, where pretty print might end up costing a lot more). |
…itUtf8Utf16JsonWriter
[WIP] Changes are on top of #2291, and hence need to be re-based first, before review.This improves performance by 20-50% for some cases (small json payloads).
Focus only on the following commit, while reviewing: b220180