Skip to content

sqlstats: Reuse sessionphase.Times on stats collector#141392

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
xinhaoz:phase-times
Feb 14, 2025
Merged

sqlstats: Reuse sessionphase.Times on stats collector#141392
craig[bot] merged 1 commit intocockroachdb:masterfrom
xinhaoz:phase-times

Conversation

@xinhaoz
Copy link
Copy Markdown
Contributor

@xinhaoz xinhaoz commented Feb 12, 2025

StatsCollector.Reset shows up in sqlstats section of cpu/mem
profiles. This seemed to mostly be allocations of new session.PhaseTimes
objects. This commit avoids new allocations of session.PhaseTimes
when resetting the stats collector.

Epic: none

Release note: None

name                                           old time/op    new time/op    delta
Sysbench/SQL/3node/oltp_read_write-24            13.2ms ± 2%    13.0ms ± 1%  -1.69%  (p=0.001 n=10+10)
ParallelSysbench/SQL/3node/oltp_read_write-24    1.22ms ± 3%    1.22ms ± 1%    ~     (p=0.315 n=10+10)

name                                           old errs/op    new errs/op    delta
Sysbench/SQL/3node/oltp_read_write-24              0.00           0.00         ~     (all equal)
ParallelSysbench/SQL/3node/oltp_read_write-24      0.01 ±20%      0.01 ±52%    ~     (p=0.231 n=9+10)

name                                           old alloc/op   new alloc/op   delta
Sysbench/SQL/3node/oltp_read_write-24            2.18MB ± 1%    2.18MB ± 1%    ~     (p=0.666 n=9+9)
ParallelSysbench/SQL/3node/oltp_read_write-24    2.03MB ± 1%    2.03MB ± 1%    ~     (p=0.971 n=10+10)

name                                           old allocs/op  new allocs/op  delta
Sysbench/SQL/3node/oltp_read_write-24             10.5k ± 2%     10.5k ± 2%    ~     (p=0.529 n=10+10)
ParallelSysbench/SQL/3node/oltp_read_write-24     9.16k ± 2%     9.17k ± 2%    ~     (p=0.853 n=10+10)

@blathers-crl
Copy link
Copy Markdown

blathers-crl bot commented Feb 12, 2025

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@xinhaoz
Copy link
Copy Markdown
Contributor Author

xinhaoz commented Feb 12, 2025

This function kept popping up in the sqlstats section of cpu/mem profiles. Super minor improvement if any.

@xinhaoz xinhaoz marked this pull request as ready for review February 12, 2025 21:41
@xinhaoz xinhaoz requested a review from a team as a code owner February 12, 2025 21:41
@xinhaoz xinhaoz requested review from angles-n-daemons and removed request for a team February 12, 2025 21:41
@dhartunian
Copy link
Copy Markdown
Collaborator

@xinhaoz which function? can you put the info in the commit msg

StatsCollector.Reset shows up in sqlstats section of cpu/mem
profiles. This seemed to mostly be allocations of new session.PhaseTimes
objects. This commit avoids new allocations of session.PhaseTimes
when resetting the stats collector.

Epic: none

Release note: None
@xinhaoz xinhaoz requested a review from a team February 12, 2025 22:10
@github-actions
Copy link
Copy Markdown
Contributor

🟢 Sysbench [SQL, 3node, oltp_read_write]
Metric Old Commit New Commit Delta Note Threshold
sec/op 9.928m ±2% 9.961m ±3% ~ p=0.579 n=10 3.0%
errs/op 0.000 ±0% 0.000 ±0% ~ p=1.000 n=10 0.0%
🟢 allocs/op 10.22k ±0% 10.18k ±0% -0.39% p=0.000 n=10 2.0%
B/op 2.193Mi ±0% 2.186Mi ±1% ~ p=0.123 n=10 2.0%
Reproduce

benchdiff binaries:

mkdir -p benchdiff/da8224e/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/da8224e7106bec44a9ab4211a1cc4caaea1ed986/bin/pkg_sql_tests benchdiff/da8224e/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/da8224e/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/d23e5d4/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/d23e5d4bdaed6204c58207c4d004a13fe652fa71/bin/pkg_sql_tests benchdiff/d23e5d4/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/d23e5d4/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

benchdiff --run=^BenchmarkSysbench/SQL/3node/oltp_read_write$ --old=d23e5d4 --new=da8224e ./pkg/sql/tests
⚪ Sysbench [KV, 1node, local, oltp_read_only]
Metric Old Commit New Commit Delta Note Threshold
sec/op 669.4µ ±1% 670.4µ ±2% ~ p=0.684 n=10 2.0%
errs/op 0.000 ±0% 0.000 ±0% ~ p=1.000 n=10 0.0%
allocs/op 439.0 ±0% 439.0 ±0% ~ p=1.000 n=10 1.5%
B/op 254.2Ki ±0% 254.2Ki ±0% ~ p=0.529 n=10 1.5%
Reproduce

benchdiff binaries:

mkdir -p benchdiff/da8224e/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/da8224e7106bec44a9ab4211a1cc4caaea1ed986/bin/pkg_sql_tests benchdiff/da8224e/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/da8224e/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/d23e5d4/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/d23e5d4bdaed6204c58207c4d004a13fe652fa71/bin/pkg_sql_tests benchdiff/d23e5d4/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/d23e5d4/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

benchdiff --run=^BenchmarkSysbench/KV/1node_local/oltp_read_only$ --old=d23e5d4 --new=da8224e ./pkg/sql/tests
⚪ Sysbench [KV, 1node, local, oltp_write_only]
Metric Old Commit New Commit Delta Note Threshold
sec/op 1.293m ±1% 1.297m ±0% ~ p=0.075 n=10 2.5%
errs/op 0.000 ±0% 0.000 ±0% ~ p=1.000 n=10 0.0%
allocs/op 1.390k ±0% 1.391k ±0% ~ p=0.062 n=10 1.8%
B/op 288.7Ki ±0% 288.5Ki ±0% ~ p=0.315 n=10 1.8%
Reproduce

benchdiff binaries:

mkdir -p benchdiff/da8224e/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/da8224e7106bec44a9ab4211a1cc4caaea1ed986/bin/pkg_sql_tests benchdiff/da8224e/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/da8224e/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
mkdir -p benchdiff/d23e5d4/bin/1058449141
gcloud storage cp gs://cockroach-microbench-ci/builds/d23e5d4bdaed6204c58207c4d004a13fe652fa71/bin/pkg_sql_tests benchdiff/d23e5d4/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests
chmod +x benchdiff/d23e5d4/bin/1058449141/cockroachdb_cockroach_pkg_sql_tests

benchdiff command:

benchdiff --run=^BenchmarkSysbench/KV/1node_local/oltp_write_only$ --old=d23e5d4 --new=da8224e ./pkg/sql/tests
Artifacts

download:

mkdir -p new
gcloud storage cp gs://cockroach-microbench-ci/artifacts/da8224e7106bec44a9ab4211a1cc4caaea1ed986/13295590003-1/\* new/
mkdir -p old
gcloud storage cp gs://cockroach-microbench-ci/artifacts/d23e5d4bdaed6204c58207c4d004a13fe652fa71/13295590003-1/\* old/
Legend
  • Neutral: No significant performance change.
  • 🟡 Warning: Slight degradation, likely due to variance, but still within thresholds.
  • 🔴 Regression: Likely performance regression, requiring investigation.
  • 🟢 Improvement: Possible performance gain.

No regressions detected!

built with commit: da8224e7106bec44a9ab4211a1cc4caaea1ed986

Copy link
Copy Markdown
Collaborator

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @angles-n-daemons)

@xinhaoz
Copy link
Copy Markdown
Contributor Author

xinhaoz commented Feb 14, 2025

tftr!
bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Feb 14, 2025

@craig craig bot merged commit e08af3f into cockroachdb:master Feb 14, 2025
19 checks passed
@dhartunian dhartunian added the o-perf-efficiency Related to performance efficiency label Mar 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

o-perf-efficiency Related to performance efficiency v25.2.0-prerelease

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants