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

zapcore: add stopped for Stop method called exactly once #966

Merged
merged 4 commits into from Jun 24, 2021

Conversation

zchee
Copy link
Contributor

@zchee zchee commented Jun 16, 2021

Add stopOnce field to BufferedWriteSyncer for Stop method called exactly once.

benchmark results mostly no-op:

name                                           old time/op    new time/op    delta
BufferedWriteSyncer/write_file_with_buffer-20     137ns ± 1%     139ns ± 2%    ~     (p=0.082 n=5+6)
MultiWriteSyncer/2_discarder-20                  8.05ns ± 6%    7.93ns ± 2%    ~     (p=0.662 n=6+5)
MultiWriteSyncer/4_discarder-20                  9.15ns ± 4%    9.14ns ± 4%    ~     (p=1.000 n=5+5)
MultiWriteSyncer/4_discarder_with_buffer-20       131ns ± 2%     130ns ± 0%    ~     (p=0.370 n=6+6)
WriteSyncer/write_file_with_no_buffer-20         4.36µs ± 1%    4.24µs ± 2%  -2.96%  (p=0.004 n=5+6)
ZapConsole-20                                     787ns ± 1%     800ns ± 2%    ~     (p=0.052 n=5+6)
JSONLogMarshalerFunc-20                           738ns ± 1%     737ns ± 0%    ~     (p=0.970 n=6+6)
ZapJSON-20                                        586ns ± 4%     582ns ± 3%    ~     (p=0.818 n=6+6)
StandardJSON-20                                   991ns ± 9%     977ns ± 2%    ~     (p=0.892 n=6+5)
Sampler_Check/7_keys-20                          8.58ns ± 3%    8.49ns ± 2%    ~     (p=0.485 n=6+6)
Sampler_Check/50_keys-20                         8.18ns ± 1%    8.20ns ± 1%    ~     (p=0.690 n=5+5)
Sampler_Check/100_keys-20                        8.02ns ± 4%    8.02ns ± 2%    ~     (p=0.589 n=6+6)
Sampler_CheckWithHook/7_keys-20                  23.4ns ± 2%    23.2ns ± 2%    ~     (p=0.370 n=6+6)
Sampler_CheckWithHook/50_keys-20                 24.3ns ± 3%    24.8ns ± 7%    ~     (p=0.732 n=6+6)
Sampler_CheckWithHook/100_keys-20                24.1ns ± 2%    24.2ns ± 1%    ~     (p=0.662 n=5+6)
TeeCheck-20                                       118ns ± 1%     120ns ± 5%    ~     (p=1.000 n=4+6)
[Geo mean]                                       77.7ns         77.6ns       -0.08%

name                                           old alloc/op   new alloc/op   delta
BufferedWriteSyncer/write_file_with_buffer-20     16.0B ± 0%     16.0B ± 0%    ~     (all equal)
MultiWriteSyncer/2_discarder-20                   16.0B ± 0%     16.0B ± 0%    ~     (all equal)
MultiWriteSyncer/4_discarder-20                   16.0B ± 0%     16.0B ± 0%    ~     (all equal)
MultiWriteSyncer/4_discarder_with_buffer-20       16.0B ± 0%     16.0B ± 0%    ~     (all equal)
WriteSyncer/write_file_with_no_buffer-20          16.0B ± 0%     16.0B ± 0%    ~     (all equal)
ZapConsole-20                                    1.36kB ± 0%    1.36kB ± 0%    ~     (all equal)
JSONLogMarshalerFunc-20                          1.31kB ± 0%    1.31kB ± 0%    ~     (all equal)
ZapJSON-20                                       1.25kB ± 0%    1.25kB ± 0%    ~     (all equal)
StandardJSON-20                                  1.45kB ± 0%    1.45kB ± 0%    ~     (all equal)
Sampler_Check/7_keys-20                           0.00B          0.00B         ~     (all equal)
Sampler_Check/50_keys-20                          0.00B          0.00B         ~     (all equal)
Sampler_Check/100_keys-20                         0.00B          0.00B         ~     (all equal)
Sampler_CheckWithHook/7_keys-20                   0.00B          0.00B         ~     (all equal)
Sampler_CheckWithHook/50_keys-20                  0.00B          0.00B         ~     (all equal)
Sampler_CheckWithHook/100_keys-20                 0.00B          0.00B         ~     (all equal)
TeeCheck-20                                       64.0B ± 0%     64.0B ± 0%    ~     (all equal)
[Geo mean]                                         108B           108B       +0.00%

name                                           old allocs/op  new allocs/op  delta
BufferedWriteSyncer/write_file_with_buffer-20      1.00 ± 0%      1.00 ± 0%    ~     (all equal)
MultiWriteSyncer/2_discarder-20                    1.00 ± 0%      1.00 ± 0%    ~     (all equal)
MultiWriteSyncer/4_discarder-20                    1.00 ± 0%      1.00 ± 0%    ~     (all equal)
MultiWriteSyncer/4_discarder_with_buffer-20        1.00 ± 0%      1.00 ± 0%    ~     (all equal)
WriteSyncer/write_file_with_no_buffer-20           1.00 ± 0%      1.00 ± 0%    ~     (all equal)
ZapConsole-20                                      7.00 ± 0%      7.00 ± 0%    ~     (all equal)
JSONLogMarshalerFunc-20                            5.00 ± 0%      5.00 ± 0%    ~     (all equal)
ZapJSON-20                                         3.00 ± 0%      3.00 ± 0%    ~     (all equal)
StandardJSON-20                                    27.0 ± 0%      27.0 ± 0%    ~     (all equal)
Sampler_Check/7_keys-20                            0.00           0.00         ~     (all equal)
Sampler_Check/50_keys-20                           0.00           0.00         ~     (all equal)
Sampler_Check/100_keys-20                          0.00           0.00         ~     (all equal)
Sampler_CheckWithHook/7_keys-20                    0.00           0.00         ~     (all equal)
Sampler_CheckWithHook/50_keys-20                   0.00           0.00         ~     (all equal)
Sampler_CheckWithHook/100_keys-20                  0.00           0.00         ~     (all equal)
TeeCheck-20                                        1.00 ± 0%      1.00 ± 0%    ~     (all equal)
[Geo mean]                                         2.21           2.21       +0.00%

name                                           old time/op    new time/op    delta
BufferedWriteSyncer/write_file_with_buffer-20     137ns ± 1%     139ns ± 2%    ~     (p=0.082 n=5+6)
MultiWriteSyncer/2_discarder-20                  8.05ns ± 6%    7.93ns ± 2%    ~     (p=0.662 n=6+5)
MultiWriteSyncer/4_discarder-20                  9.15ns ± 4%    9.14ns ± 4%    ~     (p=1.000 n=5+5)
MultiWriteSyncer/4_discarder_with_buffer-20       131ns ± 2%     130ns ± 0%    ~     (p=0.370 n=6+6)
WriteSyncer/write_file_with_no_buffer-20         4.36µs ± 1%    4.24µs ± 2%  -2.96%  (p=0.004 n=5+6)
ZapConsole-20                                     787ns ± 1%     800ns ± 2%    ~     (p=0.052 n=5+6)
JSONLogMarshalerFunc-20                           738ns ± 1%     737ns ± 0%    ~     (p=0.970 n=6+6)
ZapJSON-20                                        586ns ± 4%     582ns ± 3%    ~     (p=0.818 n=6+6)
StandardJSON-20                                   991ns ± 9%     977ns ± 2%    ~     (p=0.892 n=6+5)
Sampler_Check/7_keys-20                          8.58ns ± 3%    8.49ns ± 2%    ~     (p=0.485 n=6+6)
Sampler_Check/50_keys-20                         8.18ns ± 1%    8.20ns ± 1%    ~     (p=0.690 n=5+5)
Sampler_Check/100_keys-20                        8.02ns ± 4%    8.02ns ± 2%    ~     (p=0.589 n=6+6)
Sampler_CheckWithHook/7_keys-20                  23.4ns ± 2%    23.2ns ± 2%    ~     (p=0.370 n=6+6)
Sampler_CheckWithHook/50_keys-20                 24.3ns ± 3%    24.8ns ± 7%    ~     (p=0.732 n=6+6)
Sampler_CheckWithHook/100_keys-20                24.1ns ± 2%    24.2ns ± 1%    ~     (p=0.662 n=5+6)
TeeCheck-20                                       118ns ± 1%     120ns ± 5%    ~     (p=1.000 n=4+6)
[Geo mean]                                       77.7ns         77.6ns       -0.08%

name                                           old alloc/op   new alloc/op   delta
BufferedWriteSyncer/write_file_with_buffer-20     16.0B ± 0%     16.0B ± 0%    ~     (all equal)
MultiWriteSyncer/2_discarder-20                   16.0B ± 0%     16.0B ± 0%    ~     (all equal)
MultiWriteSyncer/4_discarder-20                   16.0B ± 0%     16.0B ± 0%    ~     (all equal)
MultiWriteSyncer/4_discarder_with_buffer-20       16.0B ± 0%     16.0B ± 0%    ~     (all equal)
WriteSyncer/write_file_with_no_buffer-20          16.0B ± 0%     16.0B ± 0%    ~     (all equal)
ZapConsole-20                                    1.36kB ± 0%    1.36kB ± 0%    ~     (all equal)
JSONLogMarshalerFunc-20                          1.31kB ± 0%    1.31kB ± 0%    ~     (all equal)
ZapJSON-20                                       1.25kB ± 0%    1.25kB ± 0%    ~     (all equal)
StandardJSON-20                                  1.45kB ± 0%    1.45kB ± 0%    ~     (all equal)
Sampler_Check/7_keys-20                           0.00B          0.00B         ~     (all equal)
Sampler_Check/50_keys-20                          0.00B          0.00B         ~     (all equal)
Sampler_Check/100_keys-20                         0.00B          0.00B         ~     (all equal)
Sampler_CheckWithHook/7_keys-20                   0.00B          0.00B         ~     (all equal)
Sampler_CheckWithHook/50_keys-20                  0.00B          0.00B         ~     (all equal)
Sampler_CheckWithHook/100_keys-20                 0.00B          0.00B         ~     (all equal)
TeeCheck-20                                       64.0B ± 0%     64.0B ± 0%    ~     (all equal)
[Geo mean]                                         108B           108B       +0.00%

name                                           old allocs/op  new allocs/op  delta
BufferedWriteSyncer/write_file_with_buffer-20      1.00 ± 0%      1.00 ± 0%    ~     (all equal)
MultiWriteSyncer/2_discarder-20                    1.00 ± 0%      1.00 ± 0%    ~     (all equal)
MultiWriteSyncer/4_discarder-20                    1.00 ± 0%      1.00 ± 0%    ~     (all equal)
MultiWriteSyncer/4_discarder_with_buffer-20        1.00 ± 0%      1.00 ± 0%    ~     (all equal)
WriteSyncer/write_file_with_no_buffer-20           1.00 ± 0%      1.00 ± 0%    ~     (all equal)
ZapConsole-20                                      7.00 ± 0%      7.00 ± 0%    ~     (all equal)
JSONLogMarshalerFunc-20                            5.00 ± 0%      5.00 ± 0%    ~     (all equal)
ZapJSON-20                                         3.00 ± 0%      3.00 ± 0%    ~     (all equal)
StandardJSON-20                                    27.0 ± 0%      27.0 ± 0%    ~     (all equal)
Sampler_Check/7_keys-20                            0.00           0.00         ~     (all equal)
Sampler_Check/50_keys-20                           0.00           0.00         ~     (all equal)
Sampler_Check/100_keys-20                          0.00           0.00         ~     (all equal)
Sampler_CheckWithHook/7_keys-20                    0.00           0.00         ~     (all equal)
Sampler_CheckWithHook/50_keys-20                   0.00           0.00         ~     (all equal)
Sampler_CheckWithHook/100_keys-20                  0.00           0.00         ~     (all equal)
TeeCheck-20                                        1.00 ± 0%      1.00 ± 0%    ~     (all equal)
[Geo mean]                                         2.21           2.21       +0.00%

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee zchee force-pushed the zapcore-bufferedws-stoponce branch from 3306b8c to 69b22fd Compare June 16, 2021 07:41
@codecov
Copy link

codecov bot commented Jun 17, 2021

Codecov Report

Merging #966 (dedda5e) into master (aa3e73e) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #966   +/-   ##
=======================================
  Coverage   98.16%   98.17%           
=======================================
  Files          45       45           
  Lines        2020     2027    +7     
=======================================
+ Hits         1983     1990    +7     
  Misses         29       29           
  Partials        8        8           
Impacted Files Coverage Δ
zapcore/buffered_write_syncer.go 100.00% <100.00%> (ø)

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 aa3e73e...dedda5e. Read the comment docs.

@abhinav abhinav requested a review from prashantv June 17, 2021 15:13
Copy link
Collaborator

@prashantv prashantv left a comment

Choose a reason for hiding this comment

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

I'm supportive of allowing Stop to be called twice, though I wonder if we can do it with a simpler implementation, taking advantage of the existing mutex.

This patch changes behaviour of Stop slightly. Previously, if Stop was called before initialization, it was a no-op, and calls after it was initialized would actually stop it. Now, the first Stop will result in the sync.Once being completed, and further calls won't have any impact (nothing is stopped). Checking for a stopped bool under the mutex may be a simpler alternative that avoids the behaviour change.

Signed-off-by: Koichi Shiraishi <zchee.io@gmail.com>
@zchee
Copy link
Contributor Author

zchee commented Jun 19, 2021

@prashantv changed to stopped bool and add local value for avoid call mutex twice.
How about it?

@zchee zchee changed the title zapcore: add stopOnce for Stop method called exactly once zapcore: add stopped for Stop method called exactly once Jun 19, 2021
Copy link
Collaborator

@abhinav abhinav left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@abhinav abhinav requested a review from prashantv June 22, 2021 01:54
@prashantv prashantv merged commit fb71758 into uber-go:master Jun 24, 2021
@zchee
Copy link
Contributor Author

zchee commented Jun 25, 2021

@prashantv Thanks!

@zchee zchee deleted the zapcore-bufferedws-stoponce branch June 25, 2021 00:24
@prashantv
Copy link
Collaborator

Thanks for the contribution @zchee!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants