Skip to content

ssmemstorage: improve lock contention on RecordStatement#106860

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
j82w:106789
Jul 19, 2023
Merged

ssmemstorage: improve lock contention on RecordStatement#106860
craig[bot] merged 1 commit intocockroachdb:masterfrom
j82w:106789

Conversation

@j82w
Copy link
Copy Markdown
Contributor

@j82w j82w commented Jul 14, 2023 •
edited
Loading

  1. The stats object lock was being held even during the insights event
    creation and insert which has it's own lock. The insights logic is now
    moved above the stats lock.
  2. Moved latency calculation outside the stats lock.
  3. Moved error code calculation outside the stats lock.
  4. ss_mem_storage converted to use a RWMutex. Each object in the map
    has it's own lock. Read lock allows mulitple threads to read from the
    map at the same time which is the common case. Writes are only done the
    first time a statement is seen.

Fixes: #106789, Fixes: #55285

./dev bench pkg/sql/sqlstats/sslocal -f BenchmarkRecordStatement  --ignore-cache --verbose  --test-args='-test.mutexprofile=mutex_pr.out -test.cpuprofile=cpu_pr.out -test.benchtime=30s' --timeout "10m"

// Master no changes
BenchmarkRecordStatement/RecordStatement:_Parallelism_10-10         	64775088	       558.8 ns/op
    sql_stats_test.go:655: -- test log scope end --
PASS
================================================================================
INFO: Elapsed time: 37.529s, Critical Path: 37.26s
INFO: 2 processes: 1 internal, 1 darwin-sandbox.
INFO: Build completed successfully, 2 total actions
//pkg/sql/sqlstats/sslocal:sslocal_test                                  PASSED in 37.2s

// Changes after the PR
BenchmarkRecordStatement/RecordStatement:_Parallelism_10-10         	67960656	       523.8 ns/op
    sql_stats_test.go:655: -- test log scope end --
PASS
================================================================================
INFO: Elapsed time: 37.031s, Critical Path: 36.69s
INFO: 2 processes: 1 internal, 1 darwin-sandbox.
INFO: Build completed successfully, 2 total actions
//pkg/sql/sqlstats/sslocal:sslocal_test                                  PASSED in 36.7s

Release note (performance improvement): Reduced lock contention on ssmemstorage.RecordStatement. This is useful for workloads that execute the same statement concurrently on the same SQL instance.

@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Jul 14, 2023

This change is Reviewable

@j82w j82w marked this pull request as ready for review July 18, 2023 17:25
@j82w j82w requested a review from a team July 18, 2023 17:25
maryliag
maryliag reviewed Jul 18, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @j82w)

pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go line 569 at r1 (raw file):

	}

	// Key does

key does....? (also add period to your comments, including above)

pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go line 581 at r1 (raw file):

	// doesn't exist yet.
	stats, ok := s.mu.stmts[key]
	if ok {

can you combine the two things? so ok || !createIfNonexistent ?

pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go line 630 at r1 (raw file):

	// Avoid taking the full lock
	if !createIfNonexistent {

same comment as above

pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go line 651 at r1 (raw file):

	}

	if !createIfNonexistent {

same comment as above

j82w
j82w commented Jul 18, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@j82w j82w 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 @maryliag)

pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go line 569 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

key does....? (also add period to your comments, including above)

Done.

pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go line 581 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

can you combine the two things? so ok || !createIfNonexistent ?

Done.

pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go line 630 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

same comment as above

Done.

pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go line 651 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

same comment as above

Done.

@xinhaoz
Copy link
Copy Markdown
Contributor

xinhaoz commented Jul 18, 2023

pkg/sql/sqlstats/insights/detector.go line 97 at r2 (raw file):

	id appstatspb.StmtFingerprintID, shouldFlush bool,
) PercentileValues {
	// latencySummary.Query might modify its own state (Stream.flush), so a read-write lock is necessary.

Is this comment no longer true? If so it looks like it needs to be updated.

@xinhaoz
Copy link
Copy Markdown
Contributor

xinhaoz commented Jul 18, 2023

pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go line 982 at r2 (raw file):

	key sampledPlanKey,
) (lastSampled time.Time, found bool) {
	s.mu.RLock()

Should this be the muPlanCache Rlock?

j82w
j82w commented Jul 18, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@j82w j82w 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 @maryliag and @xinhaoz)

pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go line 982 at r2 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Should this be the muPlanCache Rlock?

Fixed

pkg/sql/sqlstats/insights/detector.go line 97 at r2 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Is this comment no longer true? If so it looks like it needs to be updated.

I'll revert it for now. In a future PR the latencySummary should have it's own lock.

maryliag
maryliag approved these changes Jul 19, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@maryliag maryliag 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 for looking into this! Really nice changes!
:lgtm:

Reviewed 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @xinhaoz)

ssmemstorage: improve lock contention on RecordStatement
a792cd3 
1. The stats object lock was being held even during the insights event
creation and insert which has it's own lock. The insights logic is now
moved above the stats lock.
2. Moved latency calculation outside the stats lock.
3. Moved error code calculation outside the stats lock.
4. ss_mem_storage converted to use a RWMutex. Each object in the map
has it's own lock. Read lock allows mulitple threads to read from the
map at the same time which is the common case. Writes are only done the
first time a statement is seen.

Fixes: #106789, Fixes: #55285

Release note (performance improvement): Reduced lock contention on
`ssmemstorage.RecordStatement`. This is useful for workloads that
execute the same statement concurrently on the same SQL instance.
@j82w
Copy link
Copy Markdown
Contributor Author

j82w commented Jul 19, 2023

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jul 19, 2023

This PR was included in a batch that timed out, it will be automatically retried

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jul 19, 2023

This PR was included in a batch that was canceled, it will be automatically retried

@craig craig bot merged commit e23993f into cockroachdb:master Jul 19, 2023
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jul 19, 2023

Build succeeded:

@cockroach-teamcity cockroach-teamcity mentioned this pull request Jul 20, 2023
Closed
@maryliag maryliag mentioned this pull request Aug 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@maryliag maryliag maryliag 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.

StatementStatistics: improve lock contention performance sql: refactor appStats to have a RWMutex instead of a Mutex

4 participants

@j82w @cockroach-teamcity @xinhaoz @maryliag