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

tikv_util: make memory quota alloc and free simpler #16938

Merged
merged 15 commits into from
May 8, 2024

Conversation

glorv
Copy link
Contributor

@glorv glorv commented Apr 30, 2024

What is changed and how it works?

Issue Number: ref #15990

What's Changed:

Replace `compare_exchange_weak` loop with single `fetch_add` and `fetch_sub` to make the memory alloc and free simpler.
I do this change base on the fact that we does not expect the memory control to be precise on every operation. In high concurrency scenario and when the conflict is heavy, the real peak memory usage may be small than the capacity, but not smaller than `capacity - num_of_concurrency * avg_alloc_bytes`.

NOTE: In order to avoid counter overflow, this change introduces a hard limit(256PiB) that one alloc can acquire as it's big enough for any real world application. Passing higher value to `alloc` will result in an error, passing higher value to `alloc_force` and `free` will result in noop.

benchmark Result: (The benchmark was running with a modified version of https://github.com/overvenus/benches-rust/blob/82c75f87bb7b9fabdbb2bbcf2bb11cc8f726d3a8/benches/memory.rs to compare the old and new version of MemoryQuota)

Benchmarking Alloc Only/Alloc/ok: Collecting 100 samples in estimated 5.0000 s --  Alloc Only/Alloc/ok     
                        time:   [11.627 ns 11.707 ns 11.783 ns]
Benchmarking Alloc Only New/AllocNew/ok: Collecting 100 samples in estimated..  -- Alloc Only New/AllocNew/ok   
                        time:   [13.263 ns 15.141 ns 17.126 ns]


Benchmarking Alloc Only/Alloc/fail: Collecting 100 samples in estimated 5.0000 s -- Alloc Only/Alloc/fail   
                        time:   [2.7350 ns 2.7457 ns 2.7566 ns]
Benchmarking Alloc Only New/AllocNew/fail: Collecting 100 samples in estimated 5 -- Alloc Only New/AllocNew/fail    
                        time:   [2.5062 ns 2.5333 ns 2.5629 ns]


Benchmarking Alloc Free/MemoryQuota/alloc free: Collecting 100 samples in esti.. --  Alloc Free/MemoryQuota/alloc free
                        time:   [22.268 ns 22.455 ns 22.656 ns]
Benchmarking Alloc Free New/MemoryQuotaNew/alloc free: Collecting 100 samples .. -- Alloc Free New/MemoryQuotaNew/alloc free
                        time:   [19.653 ns 19.878 ns 20.154 ns]


Benchmarking Alloc Free/OwnedAllocated/alloc free: Collecting 100 samples in est -- Alloc Free/OwnedAllocated/alloc free
                        time:   [34.812 ns 35.084 ns 35.397 ns]
Benchmarking Alloc Free New/OwnedAllocatedNew/alloc free: Collecting 100 samples -- Alloc Free New/OwnedAllocatedNew/alloc free
                        time:   [35.152 ns 37.407 ns 40.522 ns]


Benchmarking 2 Threads/MemoryQuota/alloc free: Collecting 100 samples in estimat -- 2 Threads/MemoryQuota/alloc free
                        time:   [133.58 ns 136.84 ns 140.07 ns]
Benchmarking 2 Threads New/MemoryQuota/alloc free: Collecting 100 samples in est -- 2 Threads New/MemoryQuota/alloc free
                        time:   [23.941 ns 24.808 ns 25.850 ns]


Benchmarking 2 Threads/OwnedAllocated/alloc free: Collecting 100 samples in esti -- 2 Threads/OwnedAllocated/alloc free
                        time:   [208.20 ns 214.79 ns 221.67 ns]
Benchmarking 2 Threads New/OwnedAllocated/alloc free: Collecting 100 samples in  -- 2 Threads New/OwnedAllocated/alloc free
                        time:   [41.930 ns 45.758 ns 51.506 ns]

Benchmarking 4 Threads/MemoryQuota/alloc free: Collecting 100 samples in estimat -- 4 Threads/MemoryQuota/alloc free
                        time:   [421.70 ns 435.66 ns 452.09 ns]
Benchmarking 4 Threads New/MemoryQuota/alloc free: Collecting 100 samples in est -- 4 Threads New/MemoryQuota/alloc free
                        time:   [38.904 ns 42.572 ns 48.319 ns]


Benchmarking 4 Threads/OwnedAllocated/alloc free: Collecting 100 samples in esti -- 4 Threads/OwnedAllocated/alloc free
                        time:   [619.40 ns 634.09 ns 651.68 ns]
Benchmarking 4 Threads New/OwnedAllocated/alloc free: Collecting 100 samples in  -- 4 Threads New/OwnedAllocated/alloc free
                        time:   [54.774 ns 56.300 ns 57.955 ns]


Benchmarking 8 Threads/MemoryQuota/alloc free: Collecting 100 samples in estimat -- 8 Threads/MemoryQuota/alloc free
                        time:   [1.9759 µs 2.0775 µs 2.1650 µs]
Benchmarking 8 Threads New/MemoryQuota/alloc free: Collecting 100 samples in est -- 8 Threads New/MemoryQuota/alloc free
                        time:   [119.88 ns 154.39 ns 199.66 ns]


Benchmarking 8 Threads/OwnedAllocated/alloc free: Collecting 100 samples in esti -- 8 Threads/OwnedAllocated/alloc free
                        time:   [2.7435 µs 2.8082 µs 2.8620 µs]
Benchmarking 8 Threads New/OwnedAllocated/alloc free: Collecting 100 samples in  -- 8 Threads New/OwnedAllocated/alloc free
                        time:   [390.64 ns 461.45 ns 539.01 ns]


Benchmarking 16 Threads/MemoryQuota/alloc free: Collecting 100 samples in estima -- 16 Threads/MemoryQuota/alloc free
                        time:   [1.0091 µs 1.0961 µs 1.1982 µs]
Benchmarking 16 Threads New/MemoryQuota/alloc free: Collecting 100 samples in es -- 16 Threads New/MemoryQuota/alloc free
                        time:   [263.66 ns 305.28 ns 353.69 ns]


Benchmarking 16 Threads/OwnedAllocated/alloc free: Collecting 100 samples in est -- 16 Threads/OwnedAllocated/alloc free
                        time:   [1.1606 µs 1.1975 µs 1.2382 µs]
Benchmarking 16 Threads New/OwnedAllocated/alloc free: Collecting 100 samples in -- 16 Threads New/OwnedAllocated/alloc free
                        time:   [166.27 ns 172.57 ns 181.63 ns]


Benchmarking 32 Threads/MemoryQuota/alloc free: Collecting 100 samples in estima -- 32 Threads/MemoryQuota/alloc free
                        time:   [1.1244 µs 1.2449 µs 1.3840 µs]
Benchmarking 32 Threads New/MemoryQuota/alloc free: Collecting 100 samples in es -- 32 Threads New/MemoryQuota/alloc free
                        time:   [428.08 ns 500.57 ns 582.01 ns]


Benchmarking 32 Threads/OwnedAllocated/alloc free: Collecting 100 samples in est -- 32 Threads/OwnedAllocated/alloc free
                        time:   [1.9411 µs 2.1136 µs 2.3348 µs]
Benchmarking 32 Threads New/OwnedAllocated/alloc free: Collecting 100 samples in -- 32 Threads New/OwnedAllocated/alloc free
                        time:   [541.50 ns 603.69 ns 661.78 ns]


Benchmarking 64 Threads/MemoryQuota/alloc free: Collecting 100 samples in estima -- 64 Threads/MemoryQuota/alloc free
                        time:   [1.9853 µs 2.2539 µs 2.5148 µs]
Benchmarking 64 Threads New/MemoryQuota/alloc free: Collecting 100 samples in es -- 64 Threads New/MemoryQuota/alloc free
                        time:   [802.77 ns 852.78 ns 895.60 ns]



Benchmarking 64 Threads/OwnedAllocated/alloc free: Collecting 100 samples in est -- 64 Threads/OwnedAllocated/alloc free
                        time:   [3.5248 µs 3.7999 µs 4.1271 µs]
Benchmarking 64 Threads New/OwnedAllocated/alloc free: Collecting 100 samples in -- 64 Threads New/OwnedAllocated/alloc free
                        time:   [1.2609 µs 1.3517 µs 1.5102 µs]

Related changes

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

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Release note

None

Signed-off-by: glorv <glorvs@163.com>
Copy link
Contributor

ti-chi-bot bot commented Apr 30, 2024

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • Connor1996
  • overvenus

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.

Signed-off-by: glorv <glorvs@163.com>
Signed-off-by: glorv <glorvs@163.com>
Signed-off-by: glorv <glorvs@163.com>
Signed-off-by: glorv <glorvs@163.com>
Signed-off-by: glorv <glorvs@163.com>
Signed-off-by: glorv <glorvs@163.com>
@glorv
Copy link
Contributor Author

glorv commented May 6, 2024

@overvenus @Connor1996 PTAL, thanks~

Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-chi-bot ti-chi-bot bot added the status/LGT1 Status: PR - There is already 1 approval label May 6, 2024
Copy link
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

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

Ok(_) => return,
Err(current) => in_use_bytes = current,
}
let bytes = bytes as isize;
Copy link
Member

Choose a reason for hiding this comment

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

What if the bytes is larger than isize::MAX?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. In order to handle overflow, I added a hard limit for a single alloc call.

@ti-chi-bot ti-chi-bot bot removed the status/LGT1 Status: PR - There is already 1 approval label May 6, 2024
Signed-off-by: glorv <glorvs@163.com>
glorv added 2 commits May 6, 2024 17:37
Signed-off-by: glorv <glorvs@163.com>
@glorv
Copy link
Contributor Author

glorv commented May 6, 2024

/retest

Copy link
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

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

It's better to check numeric bounds with every method call.

Err(current) => in_use_bytes = current,
}
}
self.in_use.fetch_add(bytes as isize, Ordering::Relaxed);
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@glorv glorv May 7, 2024

Choose a reason for hiding this comment

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

But the previous implementation also has this issue that the value of in_use can overflow.

Since the API design for alloc_force does not return an error, I think it is the caller's responsibility to ensure that both the actual memory allocation and this counter increase should success after calling alloc_force .

Copy link
Member

Choose a reason for hiding this comment

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

The second alloc will return an error in the previous implementation.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=5da24bccc3031d787b7541a841247663


It's okay that sometimes in_use is larger than capacity, but it's not okay that alloc continues returning success.

glorv added 2 commits May 7, 2024 15:07
Signed-off-by: glorv <glorvs@163.com>
Signed-off-by: glorv <glorvs@163.com>
@glorv
Copy link
Contributor Author

glorv commented May 7, 2024

@overvenus @Connor1996 PTAL again, thanks~

Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-chi-bot ti-chi-bot bot added the status/LGT1 Status: PR - There is already 1 approval label May 7, 2024
Copy link
Member

@overvenus overvenus 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

Comment on lines 232 to 233
if bytes > MAX_MEMORY_ALLOC_SIZE {
return;
Copy link
Member

Choose a reason for hiding this comment

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

Can we add some log or metrics here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Add a warn log for oversized alloc. Maybe we can also add a metrics for the alloc size distribution histogram later if we really need it.

glorv added 3 commits May 8, 2024 16:13
Signed-off-by: glorv <glorvs@163.com>
Signed-off-by: glorv <glorvs@163.com>
Copy link
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-chi-bot ti-chi-bot bot added status/LGT2 Status: PR - There are already 2 approvals and removed status/LGT1 Status: PR - There is already 1 approval labels May 8, 2024
@glorv
Copy link
Contributor Author

glorv commented May 8, 2024

/merge

Copy link
Contributor

ti-chi-bot bot commented May 8, 2024

@glorv: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

Copy link
Contributor

ti-chi-bot bot commented May 8, 2024

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

Commit hash: e9c10c4

@ti-chi-bot ti-chi-bot bot added the status/can-merge Status: Can merge to base branch label May 8, 2024
@ti-chi-bot ti-chi-bot bot merged commit 3c4358f into tikv:master May 8, 2024
6 of 7 checks passed
@ti-chi-bot ti-chi-bot bot added this to the Pool milestone May 8, 2024
@glorv glorv deleted the mem-quota branch May 9, 2024 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none size/L status/can-merge Status: Can merge to base branch status/LGT2 Status: PR - There are already 2 approvals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants