PageStorage: Mvcc directory (without GC/restore) by JaySon-Huang · Pull Request #3637 · pingcap/tiflash

JaySon-Huang · 2021-12-13T06:14:02Z

What problem does this PR solve?

Issue Number: related to #3594

Problem Summary:

This PR focus on implementing the apply changes to / read PageEntry from MVCC directory.

These functions will be implemented in the following PRs:

Persisting to/restoring from WALStore
GC deleted pages

What is changed and how it works?

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

Documentation

Release note

None

ti-chi-bot · 2021-12-13T06:14:04Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

jiaqizho

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.

Details

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

dbms/src/Storages/Page/V3/PageDirectory.cpp

JaySon-Huang · 2021-12-13T15:51:14Z

/run-all-tests

dbms/src/Storages/Page/Snapshot.h

Signed-off-by: JaySon-Huang <tshent@qq.com>

…st entry Signed-off-by: JaySon-Huang <tshent@qq.com>

Signed-off-by: JaySon-Huang <tshent@qq.com>

JaySon-Huang · 2021-12-15T15:08:25Z

/run-all-tests

Signed-off-by: JaySon-Huang <tshent@qq.com>

JaySon-Huang · 2021-12-16T08:36:03Z

/run-all-tests

Signed-off-by: JaySon-Huang <tshent@qq.com>

JaySon-Huang · 2021-12-16T10:19:16Z

@jiaqizho PTAL

JaySon-Huang · 2021-12-16T11:32:06Z

/run-all-tests

Signed-off-by: JaySon-Huang <tshent@qq.com>

sre-bot · 2021-12-16T12:15:28Z

Coverage detail: https://ci-internal.pingcap.net/job/tics_ghpr_unit_test/415/cobertura/
(Coverage detail url is limited office network access)

lines: 42.5% (47422 out of 111506)
branches: 6.0% (72913 out of 1207629)

jiaqizho · 2021-12-17T04:02:36Z

dbms/src/Storages/Page/V3/PageDirectory.cpp

+    }
+
+    // stage 2, persisted the changes to WAL
+    // wal.apply(edit);


Why comment out this line?

Because WALStore has not been implemented in this PR (but its tests don't care about this stage). So I comment out this line.

dbms/src/Storages/Page/V3/PageDirectory.cpp

jiaqizho · 2021-12-20T02:29:21Z

dbms/src/Storages/Page/V3/PageDirectory.cpp

+        else
+            ++iter;
+    }
+    throw Exception("Not implemented", ErrorCodes::NOT_IMPLEMENTED);


Then after clean up snapshots, it must throw an exception, can we make a test version of this method or just not do GC ?

This PR doesn't focus on GC. So I won't test/fix this line in this PR.

jiaqizho · 2021-12-20T02:32:04Z

dbms/src/Storages/Page/V3/PageDirectory.h

+        {
+            assert(del == true);
+        }
+        explicit EntryOrDelete(const PageEntryV3 & entry_)


Just a suggestion, can we use nullptr and a entry ptr to replace it?

looks strange to me....

I don't think it is a good idea for using nullptr representing "delete". It will cause

many small heap allocation/freeing

need to dereference to get the actual entry, slowing down the query performance

So why we do not use real ptr to replace it?
ex.

std::map<PageVersionType, PageEntryV3 *> entries;

it does not mean entry will be in heap.

and I don't agree with many small heap allocation/freeing this point.

If lots of entries are in the stack, the single thread stack is 8M. Not sure will get an error from it.

Anyway, it is just a suggestion(change or not, all is fine), In my opinion, the expression of pointers and nullptr would be more appropriate here.

jiaqizho

LGTM,

There still remain many things to do. :)

jiaqizho · 2021-12-21T05:33:46Z

dbms/src/Storages/Page/V3/PageDirectory.h

+        {
+            assert(del == true);
+        }
+        explicit EntryOrDelete(const PageEntryV3 & entry_)


So why we do not use real ptr to replace it?
ex.

std::map<PageVersionType, PageEntryV3 *> entries;

it does not mean entry will be in heap.

and I don't agree with many small heap allocation/freeing this point.

If lots of entries are in the stack, the single thread stack is 8M. Not sure will get an error from it.

Anyway, it is just a suggestion(change or not, all is fine), In my opinion, the expression of pointers and nullptr would be more appropriate here.

jiaqizho · 2021-12-21T08:43:16Z

dbms/src/Storages/Page/V3/PageDirectory.cpp

+
+void PageDirectory::apply(PageEntriesEdit && edit)
+{
+    std::unique_lock write_lock(table_rw_mutex); // TODO: It is totally serialized, make it a pipeline


lock_guard?

unique_lock/shared_lock is more suitable for write/read lock because table_rw_mutex is a shared_mutex.

Signed-off-by: JaySon-Huang <tshent@qq.com>

jiaqizho · 2021-12-22T06:35:47Z

/rebuild

JaySon-Huang · 2021-12-22T11:39:10Z

/run-all-tests

sre-bot · 2021-12-22T12:19:59Z

Coverage detail: https://ci-internal.pingcap.net/job/tics_ghpr_unit_test/548/cobertura/
(Coverage detail url is limited office network access)

lines: 42.7% (47925 out of 112314)
branches: 6.1% (73487 out of 1210490)

JaySon-Huang · 2021-12-23T03:30:03Z

/merge

ti-chi-bot · 2021-12-23T03:30:04Z

@JaySon-Huang: 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.

Details

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 · 2021-12-23T03:30:11Z

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

Details

Commit hash: 7e14e59

ti-chi-bot · 2021-12-23T03:39:48Z

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

At the same time I will also trigger all tests for you:

/run-all-tests

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.

Details

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.

sre-bot · 2021-12-23T04:21:03Z

Coverage detail: https://ci-internal.pingcap.net/job/tics_ghpr_unit_test/569/cobertura/
(Coverage detail url is limited office network access)

lines: 42.7% (48015 out of 112357)
branches: 6.1% (73578 out of 1210512)

JaySon-Huang requested a review from jiaqizho December 13, 2021 06:14

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 13, 2021

JaySon-Huang commented Dec 13, 2021

View reviewed changes

dbms/src/Storages/Page/V3/PageDirectory.cpp Show resolved Hide resolved

JaySon-Huang changed the title ~~PageStorage: Mvcc directory (without GC/restore)~~ [WIP] PageStorage: Mvcc directory (without GC/restore) Dec 13, 2021

ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 13, 2021

JaySon-Huang mentioned this pull request Dec 13, 2021

Introduce PageStorage V3 to reduce CPU usage and write amplification #3594

Closed

76 tasks

JaySon-Huang force-pushed the ps_mvcc_directory branch 2 times, most recently from 318da99 to e6bcc78 Compare December 13, 2021 11:24

jiaqizho reviewed Dec 14, 2021

View reviewed changes

dbms/src/Storages/Page/Snapshot.h Show resolved Hide resolved

JaySon-Huang and others added 5 commits December 15, 2021 13:46

Add check for mvcc directory

9fc5bab

Signed-off-by: JaySon-Huang <tshent@qq.com>

Split map utils tests out

c006447

Signed-off-by: JaySon-Huang <tshent@qq.com>

Add comments

f8ff4ad

Signed-off-by: JaySon-Huang <tshent@qq.com>

Add test for get multi ids

53b7cb7

Signed-off-by: JaySon-Huang <tshent@qq.com>

Implement delete, ref

661369f

Signed-off-by: JaySon-Huang <tshent@qq.com>

JaySon-Huang force-pushed the ps_mvcc_directory branch from 1e9906e to 661369f Compare December 15, 2021 05:47

JaySon-Huang added 2 commits December 15, 2021 15:06

put snapshot into snapshot list

08313cf

Signed-off-by: JaySon-Huang <tshent@qq.com>

Log warnings instead of throwing exception when adding ref to non-exi…

51fad8d

…st entry Signed-off-by: JaySon-Huang <tshent@qq.com>

JaySon-Huang force-pushed the ps_mvcc_directory branch from 20f3fab to 51fad8d Compare December 15, 2021 07:26

Init vars

b5380c4

Signed-off-by: JaySon-Huang <tshent@qq.com>

JaySon-Huang force-pushed the ps_mvcc_directory branch from 3e9bb5e to b5380c4 Compare December 15, 2021 07:43

JaySon-Huang added 2 commits December 16, 2021 11:46

Fix compile

3969955

Signed-off-by: JaySon-Huang <tshent@qq.com>

Merge branch 'master' into ps_mvcc_directory

51fa9bc

JaySon-Huang added 2 commits December 16, 2021 17:48

Copy the ref entry first

1757618

Signed-off-by: JaySon-Huang <tshent@qq.com>

Fix ut

d04a491

Signed-off-by: JaySon-Huang <tshent@qq.com>

JaySon-Huang changed the title ~~[WIP] PageStorage: Mvcc directory (without GC/restore)~~ PageStorage: Mvcc directory (without GC/restore) Dec 16, 2021

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 16, 2021

Rename PageDirectory::VersionType -> PageVersionType

1162af3

Signed-off-by: JaySon-Huang <tshent@qq.com>

jiaqizho reviewed Dec 20, 2021

View reviewed changes

jiaqizho approved these changes Dec 21, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 21, 2021

Merge remote-tracking branch 'upstream/master' into ps_mvcc_directory

d1be27d

jiaqizho reviewed Dec 21, 2021

View reviewed changes

JaySon-Huang added 2 commits December 21, 2021 20:17

Fix bug

c9abc28

Signed-off-by: JaySon-Huang <tshent@qq.com>

Merge branch 'master' into ps_mvcc_directory

7e14e59

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 23, 2021

Merge branch 'master' into ps_mvcc_directory

14f551b

ti-chi-bot merged commit 1e0a37a into pingcap:master Dec 23, 2021

JaySon-Huang mentioned this pull request Jan 7, 2022

Fix the exception caused when put and ref are in the same WriteBatch #3829

Merged

12 tasks

JaySon-Huang mentioned this pull request Jan 19, 2022

PageStorage: Fix locks on BlobStore, PageDirectory #3897

Closed

12 tasks

Conversation

JaySon-Huang commented Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot commented Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

JaySon-Huang commented Dec 13, 2021

Uh oh!

Uh oh!

JaySon-Huang commented Dec 15, 2021

Uh oh!

JaySon-Huang commented Dec 16, 2021

Uh oh!

JaySon-Huang commented Dec 16, 2021

Uh oh!

JaySon-Huang commented Dec 16, 2021

Uh oh!

sre-bot commented Dec 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiaqizho left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiaqizho commented Dec 22, 2021

Uh oh!

JaySon-Huang commented Dec 22, 2021

Uh oh!

sre-bot commented Dec 22, 2021

Uh oh!

JaySon-Huang commented Dec 23, 2021

Uh oh!

ti-chi-bot commented Dec 23, 2021

Uh oh!

ti-chi-bot commented Dec 23, 2021

Uh oh!

ti-chi-bot commented Dec 23, 2021

Uh oh!

sre-bot commented Dec 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JaySon-Huang commented Dec 13, 2021 •

edited

Loading

ti-chi-bot commented Dec 13, 2021 •

edited

Loading

jiaqizho left a comment •

edited

Loading