Skip to content

sql: revive BenchmarkTracing#62118

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
irfansharif:210316.update-BenchmarkTracing
Mar 23, 2021
Merged

sql: revive BenchmarkTracing#62118
craig[bot] merged 1 commit intocockroachdb:masterfrom
irfansharif:210316.update-BenchmarkTracing

Conversation

@irfansharif
Copy link
Copy Markdown
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
Copy Markdown
Member

cockroach-teamcity commented Mar 17, 2021

This change is Reviewable

@irfansharif irfansharif requested review from a team and tbg 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
Copy Markdown
Contributor Author

irfansharif commented Mar 18, 2021

(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
Copy Markdown
Contributor

@knz knz Mar 18, 2021

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
Copy Markdown
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
Copy Markdown
Contributor Author

irfansharif commented Mar 23, 2021

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Mar 23, 2021

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Mar 23, 2021

Build failed (retrying...):

@irfansharif
Copy link
Copy Markdown
Contributor Author

irfansharif commented Mar 23, 2021

Unrelated flakes.

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Mar 23, 2021

Already running a review

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Mar 23, 2021

Build failed (retrying...):

@craig
Copy link
Copy Markdown
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

@tbg tbg Awaiting requested review from tbg

1 more reviewer

@knz knz knz approved these changes

Reviewers whose approvals may not affect merge requirements

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