Skip to content
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

(*Stream).WriteMore: do not Flush #441

Merged
merged 3 commits into from
Apr 29, 2020
Merged

(*Stream).WriteMore: do not Flush #441

merged 3 commits into from
Apr 29, 2020

Conversation

robfig
Copy link
Contributor

@robfig robfig commented Jan 18, 2020

I believe that WriteMore should not call Flush for these reasons:

  1. This is surprising for users because of inconsistency. Why call Flush in WriteMore and not in WriteObjectEnd?
  2. It is not necessary; callers are free to call Flush if their use case demands it.
  3. It harms performance in the common case by flushing the buffer much more frequently than it needs to be flushed.

The stream benchmark shows a 7% benefit to removing the Flush call, and I
observed a similar speedup in my real-world use case.

benchmark                                        old ns/op     new ns/op     delta
Benchmark_encode_string_with_SetEscapeHTML-8     442           437           -1.13%
Benchmark_jsoniter_large_file-8                  21222         21062         -0.75%
Benchmark_json_large_file-8                      40187         40266         +0.20%
Benchmark_stream_encode_big_object-8             8611          7956          -7.61%

benchmark                                        old allocs     new allocs     delta
Benchmark_encode_string_with_SetEscapeHTML-8     6              6              +0.00%
Benchmark_jsoniter_large_file-8                  78             78             +0.00%
Benchmark_json_large_file-8                      13             13             +0.00%
Benchmark_stream_encode_big_object-8             0              0              +0.00%

benchmark                                        old bytes     new bytes     delta
Benchmark_encode_string_with_SetEscapeHTML-8     760           760           +0.00%
Benchmark_jsoniter_large_file-8                  4920          4920          +0.00%
Benchmark_json_large_file-8                      6640          6640          +0.00%
Benchmark_stream_encode_big_object-8             0             0             +0.00%

Backwards compatibility - I believe there is little to no risk that this breaks
callers. WriteMore does not leave the JSON in a valid state, so it must be
followed by other Write* methods. To get the finished JSON out, the caller must
already be calling Flush.

Rob Figueiredo added 3 commits January 17, 2020 16:50
Previously it would append to the end of the buffer instead of reusing the
now-free space.

Benchmark demonstrates the improvement, run with -benchtime=10s

    benchmark                                        old ns/op     new ns/op     delta
    Benchmark_encode_string_with_SetEscapeHTML-8     447           442           -1.12%
    Benchmark_jsoniter_large_file-8                  20998         21222         +1.07%
    Benchmark_json_large_file-8                      39593         40187         +1.50%
    Benchmark_stream_encode_big_object-8             10787         8611          -20.17%

    benchmark                                        old allocs     new allocs     delta
    Benchmark_encode_string_with_SetEscapeHTML-8     6              6              +0.00%
    Benchmark_jsoniter_large_file-8                  78             78             +0.00%
    Benchmark_json_large_file-8                      13             13             +0.00%
    Benchmark_stream_encode_big_object-8             31             0              -100.00%

    benchmark                                        old bytes     new bytes     delta
    Benchmark_encode_string_with_SetEscapeHTML-8     760           760           +0.00%
    Benchmark_jsoniter_large_file-8                  4920          4920          +0.00%
    Benchmark_json_large_file-8                      6640          6640          +0.00%
    Benchmark_stream_encode_big_object-8             10056         0             -100.00%

Fixes #438
I believe that WriteMore should not call Flush for these reasons:

1. This is surprising for users because of inconsistency. Why call Flush in
   WriteMore and not in WriteObjectEnd?

2. It is not necessary; callers are free to call Flush if their use case demands
   it.

3. It harms performance in the common case by flushing the buffer much more
   frequently than it needs to be flushed.

The stream benchmark shows a 7% benefit to removing the Flush call, and I
observed a similar speedup in my real-world use case.

    benchmark                                        old ns/op     new ns/op     delta
    Benchmark_encode_string_with_SetEscapeHTML-8     442           437           -1.13%
    Benchmark_jsoniter_large_file-8                  21222         21062         -0.75%
    Benchmark_json_large_file-8                      40187         40266         +0.20%
    Benchmark_stream_encode_big_object-8             8611          7956          -7.61%

    benchmark                                        old allocs     new allocs     delta
    Benchmark_encode_string_with_SetEscapeHTML-8     6              6              +0.00%
    Benchmark_jsoniter_large_file-8                  78             78             +0.00%
    Benchmark_json_large_file-8                      13             13             +0.00%
    Benchmark_stream_encode_big_object-8             0              0              +0.00%

    benchmark                                        old bytes     new bytes     delta
    Benchmark_encode_string_with_SetEscapeHTML-8     760           760           +0.00%
    Benchmark_jsoniter_large_file-8                  4920          4920          +0.00%
    Benchmark_json_large_file-8                      6640          6640          +0.00%
    Benchmark_stream_encode_big_object-8             0             0             +0.00%

Backwards compatibility - I believe there is little to no risk that this breaks
callers. WriteMore does not leave the JSON in a valid state, so it must be
followed by other Write* methods. To get the finished JSON out, the caller must
already be calling Flush.
@robfig robfig requested a review from taowen January 18, 2020 03:23
@codecov
Copy link

codecov bot commented Jan 18, 2020

Codecov Report

Merging #441 into master will decrease coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #441      +/-   ##
==========================================
- Coverage   86.46%   86.38%   -0.09%     
==========================================
  Files          41       41              
  Lines        5106     5105       -1     
==========================================
- Hits         4415     4410       -5     
- Misses        555      558       +3     
- Partials      136      137       +1
Impacted Files Coverage Δ
stream.go 87.87% <100%> (-2.13%) ⬇️
reflect_struct_decoder.go 81.09% <0%> (-0.27%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49c900e...11a37a0. Read the comment docs.

@@ -177,7 +177,6 @@ func (stream *Stream) WriteEmptyObject() {
func (stream *Stream) WriteMore() {
stream.writeByte(',')
stream.writeIndention(0)
stream.Flush()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, except this line. Not sure what it's here for and if it could be removed. Pls have a check @taowen

@robfig
Copy link
Contributor Author

robfig commented Jan 27, 2020

@taowen Any thoughts?

@robfig
Copy link
Contributor Author

robfig commented Feb 3, 2020

Ping @taowen

1 similar comment
@robfig
Copy link
Contributor Author

robfig commented Apr 7, 2020

Ping @taowen

@AllenX2018 AllenX2018 merged commit 0f8241d into json-iterator:master Apr 29, 2020
zhenzou pushed a commit to zhenzou/jsoniter that referenced this pull request Feb 2, 2022
(*Stream).WriteMore: do not Flush
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants