execinfra: remove possible logging for each output row of processors #89461
Conversation
This commit removes possible logging (hidden behind level 3 verbosity) for each row that flows through `ProcessRowHelper` (which effectively all processors use). The verbosity check itself has non-trivial performance cost in this case. I think it's ok to remove it given that this logging was commented out until ea559df, and I don't recall ever having a desire to see all of the rows. ``` name old time/op new time/op delta Noop/cols=1-24 907µs ± 0% 749µs ± 0% -17.43% (p=0.000 n=9+9) Noop/cols=2-24 906µs ± 0% 748µs ± 0% -17.44% (p=0.000 n=9+10) Noop/cols=4-24 908µs ± 0% 748µs ± 0% -17.64% (p=0.000 n=10+9) Noop/cols=16-24 908µs ± 0% 749µs ± 0% -17.57% (p=0.000 n=10+10) Noop/cols=256-24 911µs ± 0% 751µs ± 0% -17.50% (p=0.000 n=10+10) name old speed new speed delta Noop/cols=1-24 578MB/s ± 0% 700MB/s ± 0% +21.12% (p=0.000 n=9+9) Noop/cols=2-24 1.16GB/s ± 0% 1.40GB/s ± 0% +21.13% (p=0.000 n=9+10) Noop/cols=4-24 2.31GB/s ± 0% 2.80GB/s ± 0% +21.41% (p=0.000 n=10+9) Noop/cols=16-24 9.24GB/s ± 0% 11.20GB/s ± 0% +21.32% (p=0.000 n=10+10) Noop/cols=256-24 147GB/s ± 0% 179GB/s ± 0% +21.22% (p=0.000 n=10+10) name old alloc/op new alloc/op delta Noop/cols=1-24 1.45kB ± 0% 1.45kB ± 0% -0.07% (p=0.000 n=10+10) Noop/cols=2-24 1.46kB ± 0% 1.46kB ± 0% -0.07% (p=0.000 n=10+9) Noop/cols=4-24 1.47kB ± 0% 1.47kB ± 0% -0.07% (p=0.000 n=10+10) Noop/cols=16-24 1.57kB ± 0% 1.57kB ± 0% -0.06% (p=0.000 n=10+10) Noop/cols=256-24 3.49kB ± 0% 3.49kB ± 0% -0.03% (p=0.000 n=10+10) name old allocs/op new allocs/op delta Noop/cols=1-24 5.00 ± 0% 5.00 ± 0% ~ (all equal) Noop/cols=2-24 5.00 ± 0% 5.00 ± 0% ~ (all equal) Noop/cols=4-24 5.00 ± 0% 5.00 ± 0% ~ (all equal) Noop/cols=16-24 5.00 ± 0% 5.00 ± 0% ~ (all equal) Noop/cols=256-24 5.00 ± 0% 5.00 ± 0% ~ (all equal) ``` Release note: None
|
This change is |
|
@DrewKimball friendly ping |
There was a problem hiding this comment.
Sorry, guess I missed this one.
Do you think we could improve these cases without having to remove the logging by just doing the check once and storing the result? Or do we need to be able to respond to changes in the verbosity level? But if we don't think this logging is necessary I definitely prefer to remove it.
Reviewable status:
complete! 1 of 0 LGTMs obtained
|
Good point. I don't recall ever needing this kind of logging, and I don't think anyone ever used it. It doesn't seem particularly useful since it's extremely verbose because it would print out a log message for each row that passes through each processor of each query (i.e. including the intermediate steps), so I think we should just remove it. TFTR! bors r+ |
|
Build succeeded: |
This commit removes possible logging (hidden behind level 3 verbosity) for each row that flows through
ProcessRowHelper(which effectively all processors use). The verbosity check itself has non-trivial performance cost in this case. I think it's ok to remove it given that this logging was commented out until ea559df, and I don't recall ever having a desire to see all of the rows.Addresses: #87685.
Release note: None