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

workload: fix performance regresion from accessing query execution mode #102193

Merged
merged 2 commits into from
Apr 26, 2023

Conversation

sean-
Copy link
Collaborator

@sean- sean- commented Apr 24, 2023

sync.RWMutex RLock() became a hot path for essentially a read-only value. The
execution mode of a pool and query doesn't change after program initialization.

Release note: None
Fixes: #101494

sync.RWMutex RLock() became a hot path for essentially a read-only value.  The
execution mode of a pool and query doesn't change after program initialization.

Release note: None
@sean- sean- requested a review from srosenberg April 24, 2023 21:28
@sean- sean- requested a review from a team as a code owner April 24, 2023 21:28
@sean- sean- self-assigned this Apr 24, 2023
@sean- sean- requested review from smg260 and removed request for a team April 24, 2023 21:28
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@srosenberg srosenberg left a comment

Choose a reason for hiding this comment

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

:lgtm:

I am doing another full run as a sanity check.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @smg260)

Copy link
Collaborator Author

@sean- sean- left a comment

Choose a reason for hiding this comment

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

Thank you. I'm tempted to make the pgx logger's presence conditional behind yet-another-connection-flag, but am not sure the juice is worth the squeeze to do that vs just removing it outright atm since I've never seen any actual log output flow through the logger or tracer interface. LMK.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @smg260)

@srosenberg
Copy link
Member

I'm tempted to make the pgx logger's presence conditional behind yet-another-connection-flag, but am not sure the juice is worth the squeeze to do that vs just removing it outright atm since I've never seen any actual log output flow through the logger or tracer interface. LMK.

Let's leave that for a follow-up story. I would also like to add a prometheus exporter (e.g., [1]) to get more insights into the connection pool. Both changes seem trivial but they need to take performance into consideration.

[1] https://github.com/IBM/pgxpoolprometheus

@srosenberg
Copy link
Member

First run looks good modulo a blip ~13:15,

kv95_workload_run_1

Second run should be finishing up: https://grafana.testeng.crdb.io/d/I2_Fu704k/kv95?orgId=1&from=now-1h&to=now&viewPanel=4

Please update the commit to mention that it Resolves: https://github.com/cockroachdb/cockroach/issues/101494, and we're good to merge!

@sean-
Copy link
Collaborator Author

sean- commented Apr 25, 2023

bors r+

@craig
Copy link
Contributor

craig bot commented Apr 25, 2023

Build failed (retrying...):

@yuzefovich
Copy link
Member

This PR needs ./dev gen bazel.

bors r-

@craig
Copy link
Contributor

craig bot commented Apr 25, 2023

Canceled.

In benchmarking workloads, pgx v5's tracelog incurs too much CPU overhead
and GC pressure and wasn't being used.

Release note: None

Epic: none
@srosenberg srosenberg force-pushed the workload-fix-method branch from 5273fe3 to f13f93a Compare April 26, 2023 01:19
@srosenberg
Copy link
Member

This PR needs ./dev gen bazel.

bors r-

Thanks @yuzefovich and sorry! I should have verified CI passed.

@srosenberg
Copy link
Member

bors r+

@craig
Copy link
Contributor

craig bot commented Apr 26, 2023

Build succeeded:

@craig craig bot merged commit 2066afb into master Apr 26, 2023
@craig craig bot deleted the workload-fix-method branch April 26, 2023 03:07
@sean-
Copy link
Collaborator Author

sean- commented Apr 26, 2023

Thank you, @yuzefovich !

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

Successfully merging this pull request may close these issues.

roachperf: regression in tpcc on 2023/04/11
4 participants