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.

Sign up for GitHub

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

Jump to bottom

sql: revive BenchmarkTracing #62118

Merged
merged 1 commit into from
Mar 23, 2021
Merged

sql: revive BenchmarkTracing #62118

merged 1 commit into from
Mar 23, 2021

Conversation

irfansharif
Copy link
Contributor

@irfansharif irfansharif commented Mar 17, 2021 •
edited
Loading

Fixes #62081. It had rotted after we removed the trace.mode setting. It
now makes use of a testing knob if enabled, installs real spans all sql
statements. We'll defer the actual investigation into the cause of the
slow-down in future PRs (prototyped in #62227).

$ make bench PKG=./pkg/bench  TESTFLAGS='-benchtime=10000x -count 20' \
    BENCHES='BenchmarkTracing' BENCHTIMEOUT=50m > benchmark-tracing.txt
$ cat benchmark-tracing.txt | grep -v tracing=t | sed 's/tracing=f//' > old.txt
$ cat benchmark-tracing.txt | grep -v tracing=f | sed 's/tracing=t//' > new.txt
$ benchstat old.txt new.txt

name                                   old time/op    new time/op    delta
Tracing/Cockroach//Scan1-24               382µs ± 2%     412µs ± 8%   +7.68%  (p=0.000 n=10+10)
Tracing/Cockroach//Insert-24              579µs ± 2%     609µs ± 6%   +5.10%  (p=0.000 n=10+10)
Tracing/MultinodeCockroach//Scan1-24      799µs ± 2%     885µs ± 1%  +10.76%  (p=0.000 n=10+9)
Tracing/MultinodeCockroach//Insert-24    1.09ms ± 1%    1.14ms ± 2%   +5.04%  (p=0.000 n=9+10)

name                                   old alloc/op   new alloc/op   delta
Tracing/Cockroach//Scan1-24              25.3kB ± 5%    32.0kB ± 4%  +26.55%  (p=0.000 n=10+10)
Tracing/Cockroach//Insert-24             38.0kB ± 6%    42.2kB ± 5%  +11.02%  (p=0.000 n=10+10)
Tracing/MultinodeCockroach//Scan1-24     77.7kB ± 8%    97.1kB ±12%  +25.05%  (p=0.000 n=10+10)
Tracing/MultinodeCockroach//Insert-24     107kB ± 8%     115kB ± 7%   +7.66%  (p=0.023 n=10+10)

name                                   old allocs/op  new allocs/op  delta
Tracing/Cockroach//Scan1-24                 256 ± 1%       280 ± 1%   +9.10%  (p=0.000 n=9+9)
Tracing/Cockroach//Insert-24                301 ± 2%       321 ± 1%   +6.64%  (p=0.000 n=10+10)
Tracing/MultinodeCockroach//Scan1-24        787 ± 2%       921 ± 2%  +17.04%  (p=0.000 n=9+9)
Tracing/MultinodeCockroach//Insert-24       748 ± 1%       805 ± 2%   +7.61%  (p=0.000 n=9+9)

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@irfansharif irfansharif requested review from tbg and a team March 17, 2021 00:31
@irfansharif irfansharif force-pushed the 210316.update-BenchmarkTracing branch from 5cacaaa to 209b26a Compare March 17, 2021 15:54
@irfansharif
Copy link
Contributor Author

(bump)

knz
knz reviewed Mar 18, 2021
View reviewed changes
pkg/sql/exec_util.go Outdated
// debugForceRealSpans can be used to force the use of real tracing spans for
// every statement. It's used primarily to measure the tracing overhead in
// BenchmarkTracing.
var debugForceRealSpans = settings.RegisterBoolSetting(
Copy link
Contributor

Choose a reason for hiding this comment

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

May I suggest a debug knob instead of a cluster setting. The cluster setting increases the chance that users will shoot themselves in the foot.

@irfansharif irfansharif mentioned this pull request Mar 18, 2021
Closed
@irfansharif irfansharif force-pushed the 210316.update-BenchmarkTracing branch from 209b26a to ed216e9 Compare March 23, 2021 13:48
irfansharif
irfansharif commented Mar 23, 2021
View reviewed changes
Copy link
Contributor Author

@irfansharif irfansharif left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz and @tbg)

pkg/sql/exec_util.go, line 196 at r1 (raw file):

Previously, knz (kena) wrote…

May I suggest a debug knob instead of a cluster setting. The cluster setting increases the chance that users will shoot themselves in the foot.

Done.

@irfansharif irfansharif requested a review from a team March 23, 2021 14:03
@irfansharif irfansharif force-pushed the 210316.update-BenchmarkTracing branch 2 times, most recently from 08a2d62 to dffba28 Compare March 23, 2021 14:12
@irfansharif irfansharif force-pushed the 210316.update-BenchmarkTracing branch from dffba28 to 20356ec Compare March 23, 2021 14:27
@irfansharif
sql: revive BenchmarkTracing
af9e646 
Fixes cockroachdb#62081. It had rotted after we removed the trace.mode setting. It
now makes use of a testing knob if enabled, installs real spans all sql
statements. We'll defer the actual investigation into the cause of the
slow-down in future PRs (prototyped in cockroachdb#62227).

    $ make bench PKG=./pkg/bench  TESTFLAGS='-benchtime=10000x -count 20' \
        BENCHES='BenchmarkTracing' BENCHTIMEOUT=50m > benchmark-tracing.txt
    $ cat benchmark-tracing.txt | grep -v tracing=t | sed 's/tracing=f\///' > old.txt
    $ cat benchmark-tracing.txt | grep -v tracing=f | sed 's/tracing=t\///' > new.txt
    $ benchstat old.txt new.txt

    name                                   old time/op    new time/op    delta
    Tracing/Cockroach/Scan1-24                382µs ± 2%     412µs ± 8%   +7.68%  (p=0.000 n=10+10)
    Tracing/Cockroach/Insert-24               579µs ± 2%     609µs ± 6%   +5.10%  (p=0.000 n=10+10)
    Tracing/MultinodeCockroach/Scan1-24       799µs ± 2%     885µs ± 1%  +10.76%  (p=0.000 n=10+9)
    Tracing/MultinodeCockroach/Insert-24     1.09ms ± 1%    1.14ms ± 2%   +5.04%  (p=0.000 n=9+10)

    name                                   old alloc/op   new alloc/op   delta
    Tracing/Cockroach/Scan1-24               25.3kB ± 5%    32.0kB ± 4%  +26.55%  (p=0.000 n=10+10)
    Tracing/Cockroach/Insert-24              38.0kB ± 6%    42.2kB ± 5%  +11.02%  (p=0.000 n=10+10)
    Tracing/MultinodeCockroach/Scan1-24      77.7kB ± 8%    97.1kB ±12%  +25.05%  (p=0.000 n=10+10)
    Tracing/MultinodeCockroach/Insert-24      107kB ± 8%     115kB ± 7%   +7.66%  (p=0.023 n=10+10)

    name                                   old allocs/op  new allocs/op  delta
    Tracing/Cockroach/Scan1-24                  256 ± 1%       280 ± 1%   +9.10%  (p=0.000 n=9+9)
    Tracing/Cockroach/Insert-24                 301 ± 2%       321 ± 1%   +6.64%  (p=0.000 n=10+10)
    Tracing/MultinodeCockroach/Scan1-24         787 ± 2%       921 ± 2%  +17.04%  (p=0.000 n=9+9)
    Tracing/MultinodeCockroach/Insert-24        748 ± 1%       805 ± 2%   +7.61%  (p=0.000 n=9+9)

Release note: None
@irfansharif irfansharif force-pushed the 210316.update-BenchmarkTracing branch from 20356ec to af9e646 Compare March 23, 2021 14:34
@irfansharif
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 23, 2021

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 23, 2021

Build failed (retrying...):

@irfansharif
Copy link
Contributor Author

Unrelated flakes.

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 23, 2021

Already running a review

@craig
Copy link
Contributor

craig bot commented Mar 23, 2021

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 23, 2021

Build succeeded:

@craig craig bot merged commit 46620dd into cockroachdb:master Mar 23, 2021
@irfansharif irfansharif deleted the 210316.update-BenchmarkTracing branch March 23, 2021 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knz knz knz approved these changes

@tbg tbg Awaiting requested review from tbg

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bench: BenchmarkTracing broken on master
3 participants
@irfansharif @cockroach-teamcity @knz