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

*: reduce ResetContextOfStmt() and NewRuntimeStatsColl() object allocation #26007

Closed
wants to merge 12 commits into from

Conversation

tiancaiamao
Copy link
Contributor

What problem does this PR solve?

Reduce prepared point get object allocation, see #25573

Set EnableCollectExecutionInfo config to false can reduce the allocation from 101 allocs/op to 89 allocs/op, it does have some performance impact on the point get case @crazycs520

What is changed and how it works?

Reduce the allocation rate by 2%

Before:

BenchmarkPreparedPointGet-16 3945436 9160 ns/op 7830 B/op 101 allocs/op

After:

BenchmarkPreparedPointGet-16 4047969 8913 ns/op 7038 B/op 99 allocs/op

What's Changed:

Avoid allocating StatementContext every time, reuse it.
Avoid allocating RuntimeStatsColl every time, reuse it.

How it Works:

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • No code

Release note

  • No release note

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 6, 2021
@tiancaiamao
Copy link
Contributor Author

image

Each time we eliminate a allocation from the list, we can reduce the allocation by 2%-1%, and the performance improve a bit.
As estimated in #25573 (comment), cut down about a half of the allocation can speed up the point get function by 35%

@github-actions github-actions bot added the sig/execution SIG execution label Jul 6, 2021
@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 7, 2021
@github-actions github-actions bot added sig/planner SIG: Planner sig/sql-infra SIG: SQL Infra labels Jul 7, 2021
Copy link
Contributor

@xhebox xhebox left a comment

Choose a reason for hiding this comment

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

Rest LGTM

executor/executor.go Show resolved Hide resolved
bindinfo/bind_test.go Outdated Show resolved Hide resolved
executor/builder.go Outdated Show resolved Hide resolved
@ti-chi-bot ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 12, 2021
sc.RuntimeStatsColl = execdetails.NewRuntimeStatsColl()
// In ExplainFor case, RuntimeStatsColl should not be reset for reuse,
// because ExplainFor need to display the last statement information.
if _, ok := s.(*ast.ExplainForStmt); ok {
Copy link
Contributor Author

@tiancaiamao tiancaiamao Jul 12, 2021

Choose a reason for hiding this comment

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

Now I do not reset RuntimeStatsColl when the statement is ExplainFor, the test passes.

// Reuse the object to reduce allocation when *RuntimeStatsColl is not nil.
func NewRuntimeStatsColl(reuse *RuntimeStatsColl) *RuntimeStatsColl {
if reuse != nil {
for k := range reuse.rootStats {
Copy link
Contributor Author

@tiancaiamao tiancaiamao Jul 12, 2021

Choose a reason for hiding this comment

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

Reuse map is cheaper than create a new map object.
Go compiler optimize this cleanup code pattern to a clearmap() function.

@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 12, 2021
@tiancaiamao
Copy link
Contributor Author

Temporary close it.
I need to reconsider this PR after I find #26161

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution sig/planner SIG: Planner sig/sql-infra SIG: SQL Infra size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants