In-Memory Engine: WriteBatch with Skiplist Engine #16433

afeinberg · 2024-01-23T17:34:08Z

What is changed and how it works?

Issue Number: ref #16323 #16141

What's Changed:

Update WriteBatch to assume a single skiplist and use RangeManager::contains.
Implement and test `get_value_cf_opt` for `HybridEngineSnapshot`.
Integrate single WriteBatch with HybridEngine.

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

ti-chi-bot · 2024-01-23T17:34:10Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

SpadeA-Tang
tonyxuqqi

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.

afeinberg · 2024-01-25T04:51:17Z

/cc @SpadeA-Tang @tonyxuqqi

components/hybrid_engine/src/snapshot.rs

afeinberg

Should be ready to publish soon.

afeinberg · 2024-01-26T00:16:18Z

PR published.
/assign @tonyxuqqi

SpadeA-Tang · 2024-01-26T05:43:05Z

components/region_cache_memory_engine/src/write_batch.rs

+                    .collect::<Vec<_>>(),
+            )
+        };
+        filtered_keys


what if the range is just evicted before this line?

In this case those keys will still be written too memory, range_manager will prevent them from being read. This is less efficient, but still correct; I can hold the lock on core when writing, but that will reduce write throughput. Another approach is to move from mutex to a read-write lock (or sharded lock) for core: perhaps I'll do this.

Addressed this: switched to sharded read write lock from cross beam, using (relatively low-cost) read() lock during WriteBatch::write.

Holding a lock (even the read/write lock) is too heavy for write, which blocks the snapshot acquisition.

Although we will not read it due to the filter of range_manager, we don't have the logic to prevent a range load. That is say we cannot avoid the following situation:
we have cache range k1..k10

write_impl: should_write_to_memory for k1..k10

evict k1..k10

load k1..k10

write_impl: write_to_memory

We can have these steps to handle it:

get ranges to write

recording the ranges

write_to_memory

for ranges that are still valid, clear the range. for ranges evicted, schedule a task to delete the range and clear the range after that.

So, load a range should if the range is being written.

What do youi mean by 'clear' the range?

struct RangeManager { ... ranges_being_written: BtreeSet<CacheRange>, }

So, clear just mean delete the range in the ranges_being_written.

@tonyxuqqi thoughts on this approach? we can also re-compute this during garbage collection.

We can have these steps to handle it:

get ranges to write

recording the ranges

write_to_memory

for ranges that are still valid, clear the range. for ranges evicted, schedule a task to delete the range and clear the range after that.

So, load a range should if the range is being written.

I worry that it's not more efficient than a simple read-write lock. For read lock, actually it's pretty fast.

afeinberg · 2024-01-26T20:21:22Z

/test

afeinberg · 2024-01-26T20:40:11Z

/cc @v01dstar

Signed-off-by: Alex Feinberg <alex@strlen.net>

…s to avoid writing keys for evicted range. Signed-off-by: Alex Feinberg <alex@strlen.net>

Signed-off-by: Alex Feinberg <alex@strlen.net>

SpadeA-Tang · 2024-01-31T03:02:51Z

/merge

ti-chi-bot · 2024-01-31T03:02:53Z

@SpadeA-Tang: 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.

ti-chi-bot · 2024-01-31T03:02:56Z

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

Commit hash: a6621af

SpadeA-Tang · 2024-01-31T04:41:35Z

/run-test retry=7

…e_batch_p2

ti-chi-bot · 2024-01-31T05:11:03Z

@afeinberg: Your PR was out of date, I have automatically updated it for you.

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.

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.

ref tikv#16323 Update WriteBatch to assume a single skiplist and use RangeManager::contains. Implement and test `get_value_cf_opt` for `HybridEngineSnapshot`. Integrate single WriteBatch with HybridEngine. Signed-off-by: Alex Feinberg <alex@strlen.net> Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com> Signed-off-by: dbsid <chenhuansheng@pingcap.com>

ti-chi-bot bot added do-not-merge/work-in-progress release-note-none contribution Type: PR - From contributors size/L labels Jan 23, 2024

afeinberg mentioned this pull request Jan 23, 2024

In-Memory Engine: write path #16323

Open

13 tasks

afeinberg force-pushed the afeinberg/memory_engine/hybrid_engine_write_batch_p2 branch from 4d5b30e to a83dadd Compare January 24, 2024 18:53

ti-chi-bot bot requested review from SpadeA-Tang and tonyxuqqi January 25, 2024 04:51

ti-chi-bot bot added size/XL and removed size/L labels Jan 25, 2024

afeinberg commented Jan 25, 2024

View reviewed changes

components/hybrid_engine/src/snapshot.rs Show resolved Hide resolved

afeinberg commented Jan 25, 2024

View reviewed changes

afeinberg force-pushed the afeinberg/memory_engine/hybrid_engine_write_batch_p2 branch 2 times, most recently from e129e3a to 35fe837 Compare January 26, 2024 00:14

afeinberg marked this pull request as ready for review January 26, 2024 00:15

ti-chi-bot bot removed the do-not-merge/work-in-progress label Jan 26, 2024

ti-chi-bot bot assigned tonyxuqqi Jan 26, 2024

SpadeA-Tang reviewed Jan 26, 2024

View reviewed changes

afeinberg requested a review from SpadeA-Tang January 26, 2024 19:46

ti-chi-bot bot requested a review from v01dstar January 26, 2024 20:40

afeinberg added 5 commits January 30, 2024 11:57

In-Memory: Write Batch Part II.

d4d3cd0

Signed-off-by: Alex Feinberg <alex@strlen.net>

Cleanup.

2ee429d

Signed-off-by: Alex Feinberg <alex@strlen.net>

Test writing to both engines.

acd19b8

Signed-off-by: Alex Feinberg <alex@strlen.net>

HybridEngineSnapshot impl.

946d691

Signed-off-by: Alex Feinberg <alex@strlen.net>

Further cleanup, docs.

e387718

Signed-off-by: Alex Feinberg <alex@strlen.net>

afeinberg added 4 commits January 30, 2024 11:57

Remove redundant code from write_impl.

3c1975b

Signed-off-by: Alex Feinberg <alex@strlen.net>

Use ShardedLock for engine.core(), hold read lock when writing batche…

56ab380

…s to avoid writing keys for evicted range. Signed-off-by: Alex Feinberg <alex@strlen.net>

Fix doctest for hybrid_engine_for_tests

49fb3c0

Signed-off-by: Alex Feinberg <alex@strlen.net>

Smaller scope for lock.

7e38ce2

Signed-off-by: Alex Feinberg <alex@strlen.net>

afeinberg force-pushed the afeinberg/memory_engine/hybrid_engine_write_batch_p2 branch from 15e2e25 to 7e38ce2 Compare January 30, 2024 19:57

afeinberg added 2 commits January 30, 2024 13:53

Add test in raftstore/read.rs::test_hybrid_engine_read()

21ba4a1

Signed-off-by: Alex Feinberg <alex@strlen.net>

Remove apply_cb

a6621af

Signed-off-by: Alex Feinberg <alex@strlen.net>

tonyxuqqi approved these changes Jan 30, 2024

View reviewed changes

ti-chi-bot bot added the status/LGT1 Status: PR - There is already 1 approval label Jan 30, 2024

SpadeA-Tang approved these changes Jan 31, 2024

View reviewed changes

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 Jan 31, 2024

ti-chi-bot bot added the status/can-merge Status: Can merge to base branch label Jan 31, 2024

Merge branch 'master' into afeinberg/memory_engine/hybrid_engine_writ…

5030e00

…e_batch_p2

ti-chi-bot bot merged commit 87d9a97 into tikv:master Jan 31, 2024
7 checks passed

ti-chi-bot bot added this to the Pool milestone Jan 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In-Memory Engine: WriteBatch with Skiplist Engine #16433

In-Memory Engine: WriteBatch with Skiplist Engine #16433

afeinberg commented Jan 23, 2024 •

edited

ti-chi-bot bot commented Jan 23, 2024 •

edited

afeinberg commented Jan 25, 2024

afeinberg left a comment

afeinberg commented Jan 26, 2024

SpadeA-Tang Jan 26, 2024

afeinberg Jan 26, 2024

afeinberg Jan 26, 2024

SpadeA-Tang Jan 29, 2024

SpadeA-Tang Jan 29, 2024

SpadeA-Tang Jan 29, 2024 •

edited

afeinberg Jan 29, 2024

SpadeA-Tang Jan 30, 2024

afeinberg Jan 30, 2024

tonyxuqqi Jan 30, 2024

afeinberg commented Jan 26, 2024

afeinberg commented Jan 26, 2024

SpadeA-Tang commented Jan 31, 2024

ti-chi-bot bot commented Jan 31, 2024

ti-chi-bot bot commented Jan 31, 2024

SpadeA-Tang commented Jan 31, 2024

ti-chi-bot bot commented Jan 31, 2024

In-Memory Engine: WriteBatch with Skiplist Engine #16433

In-Memory Engine: WriteBatch with Skiplist Engine #16433

Conversation

afeinberg commented Jan 23, 2024 • edited

What is changed and how it works?

Related changes

Check List

Release note

ti-chi-bot bot commented Jan 23, 2024 • edited

afeinberg commented Jan 25, 2024

afeinberg left a comment

Choose a reason for hiding this comment

afeinberg commented Jan 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpadeA-Tang Jan 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afeinberg commented Jan 26, 2024

afeinberg commented Jan 26, 2024

SpadeA-Tang commented Jan 31, 2024

ti-chi-bot bot commented Jan 31, 2024

ti-chi-bot bot commented Jan 31, 2024

SpadeA-Tang commented Jan 31, 2024

ti-chi-bot bot commented Jan 31, 2024

afeinberg commented Jan 23, 2024 •

edited

ti-chi-bot bot commented Jan 23, 2024 •

edited

SpadeA-Tang Jan 29, 2024 •

edited