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.

Sign up for GitHub

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

Jump to bottom

sorter: reduce memory malloc to avoid too much CPU overhead #1854

Merged
merged 6 commits into from
May 27, 2021
Merged

sorter: reduce memory malloc to avoid too much CPU overhead #1854

merged 6 commits into from
May 27, 2021

Conversation

amyangfei
Copy link
Contributor

@amyangfei amyangfei commented May 26, 2021 •
edited
Loading

What problem does this PR solve?

ref: #1853

What is changed and how it works?

Since the access of h.heap is not concurrent, we can use the same slice

The heap will be used in an async pool. We choose a conservative initialize size for sort heap slice.

  • the heap is flushed every 100ms or accumulated data size execeeds 1GB?

Check List

Tests

  • Unit test
  • Integration test

Release note

  • Reduce memory malloc in sort heap to avoid too much CPU overhead.

@amyangfei amyangfei added needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. needs-cherry-pick-release-5.0 Should cherry pick this PR to release-5.0 branch. labels May 26, 2021
@amyangfei amyangfei added this to the v5.0.2 milestone May 26, 2021
@ti-chi-bot ti-chi-bot requested review from liuzix and lonng May 26, 2021 15:44
@ti-chi-bot ti-chi-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 26, 2021
@ti-chi-bot ti-chi-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 26, 2021
@amyangfei
Copy link
Contributor Author

/run-all-tests

@amyangfei amyangfei changed the title sorter: reuse a slice in heap sorter to avoid too much CPU overhead. sorter: reduce memory malloc to avoid too much CPU overhead May 27, 2021
@amyangfei
Copy link
Contributor Author

/run-kafka-tests

lonng
lonng reviewed May 27, 2021
View reviewed changes
cdc/puller/sorter/heap_sorter.go
@@ -159,7 +160,7 @@ func (h *heapSorter) flush(ctx context.Context, maxResolvedTs uint64) error {
return nil
}
oldHeap = h.heap
h.heap = make(sortHeap, 0, 65536)
h.heap = make(sortHeap, 0, sortHeapCapacity)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to allocate a new slice for heap? Can we reuse the previous one, eg: h.heap = h.heap[:0]

Copy link
Contributor

Choose a reason for hiding this comment

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

No. Flushing the heap to disk is done in a separate goroutine, so we need to keep the original underlying array intact until it's been flushed.

@amyangfei
Copy link
Contributor Author

/run-kafka-tests

@liuzix
Copy link
Contributor

liuzix commented May 27, 2021

/lgtm

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 27, 2021
@lonng
Copy link
Contributor

lonng commented May 27, 2021

/lgtm

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • liuzix
  • lonng

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 writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 27, 2021
@lonng
Copy link
Contributor

lonng commented May 27, 2021

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: c9bafa6

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 27, 2021
@amyangfei
Copy link
Contributor Author

/run-integration-tests
/run-leak-tests

@amyangfei
Copy link
Contributor Author

/run-leak-tests

@amyangfei amyangfei added the release-blocker This issue blocks a release. Please solve it ASAP. label May 27, 2021
@amyangfei
Copy link
Contributor Author

/run-unit-tests
/run-leak-tests

@amyangfei
Copy link
Contributor Author

/run-integration-tests
/run-kafka-tests

@amyangfei
Copy link
Contributor Author

/run-unit-tests

@amyangfei
Copy link
Contributor Author

/run-kafka-tests

@amyangfei
Copy link
Contributor Author

/run-integration-tests
/run-unit-tests

@amyangfei
Copy link
Contributor Author

/run-unit-tests

@amyangfei
Copy link
Contributor Author

/merge

@amyangfei
Copy link
Contributor Author

/run-unit-tests

@codecov-commenter
Copy link

Codecov Report

Merging #1854 (3056ddb) into master (99ee8fb) will increase coverage by 0.3908%.
The diff coverage is 65.2542%.

@@               Coverage Diff                @@
##             master      #1854        +/-   ##
================================================
+ Coverage   53.4083%   53.7992%   +0.3908%     
================================================
  Files           154        158         +4     
  Lines         16166      16385       +219     
================================================
+ Hits           8634       8815       +181     
- Misses         6608       6616         +8     
- Partials        924        954        +30

@amyangfei amyangfei merged commit 84df40d into pingcap:master May 27, 2021
@amyangfei amyangfei deleted the update-heap-sorter-memory-alloc branch May 27, 2021 10:12
@ti-chi-bot ti-chi-bot mentioned this pull request May 27, 2021
Merged
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #1862.

@ti-chi-bot ti-chi-bot mentioned this pull request May 27, 2021
Merged
@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #1863.

ti-chi-bot added a commit that referenced this pull request May 27, 2021
@ti-chi-bot
sorter: reduce memory malloc to avoid too much CPU overhead (#1854) (#…
669c904 
…1863)
ti-chi-bot added a commit that referenced this pull request May 28, 2021
@ti-chi-bot
sorter: reduce memory malloc to avoid too much CPU overhead (#1854) (#…
bc932b5 
…1862)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lonng lonng lonng left review comments

@liuzix liuzix Awaiting requested review from liuzix

Assignees
No one assigned
Labels
needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. needs-cherry-pick-release-5.0 Should cherry pick this PR to release-5.0 branch. release-blocker This issue blocks a release. Please solve it ASAP. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
v5.0.2
Development

Successfully merging this pull request may close these issues.
5 participants
@amyangfei @liuzix @lonng @ti-chi-bot @codecov-commenter