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

DNM: [Flash 763] Imporve delta's performance by DMFile #374

Closed
wants to merge 21 commits into from

Conversation

JaySon-Huang
Copy link
Contributor

@JaySon-Huang JaySon-Huang commented Dec 26, 2019

Use DMFile to store delta's data

@JaySon-Huang JaySon-Huang changed the title WIP: [Flash 763] Use DMFile to store delta's data WIP: [Flash 763] Imporve delta's performance by DMFile Dec 26, 2019
Signed-off-by: JaySon-Huang <[email protected]>
Signed-off-by: JaySon-Huang <[email protected]>
Signed-off-by: JaySon-Huang <[email protected]>
Signed-off-by: JaySon-Huang <[email protected]>
Signed-off-by: JaySon-Huang <[email protected]>
Signed-off-by: JaySon-Huang <[email protected]>
set column_id for incoming blocks
disable mark cached for APPENDING DMFile
drop tables with DeltaMerge engine

Signed-off-by: JaySon-Huang <[email protected]>
Signed-off-by: JaySon-Huang <[email protected]>
Signed-off-by: JaySon-Huang <[email protected]>
Signed-off-by: JaySon-Huang <[email protected]>
Signed-off-by: JaySon-Huang <[email protected]>
@@ -80,7 +80,7 @@ class DeltaMergeBlockInputStream final : public IBlockInputStream, Allocator<fal

public:
DeltaMergeBlockInputStream(const SkippableBlockInputStreamPtr & stable_input_stream_,
const DeltaValueSpacePtr & delta_value_space_,
const DeltaValuesPtr & delta_value_space_,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rename param name

for (auto & [extra_path, ids] : path_and_ids_vec)
{
for (auto id : ids)
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about move these code into StoragePool::gc

using DeltaSpacePtr = std::shared_ptr<DeltaSpace>;

// TODO: rename this class to DeltaValueSpace?
class DeltaSpace
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rename this class to DeltaValueSpace

@@ -82,6 +82,9 @@ class PageStorage

PageId getNormalPageId(PageId page_id, SnapshotPtr snapshot = {});

// Given a set of page ids, return valid page ids in that set.
PageIdSet filterValidPages(const PageIdSet & pages, SnapshotPtr snapshot = {});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need this function in actual.

dbms/src/Storages/DeltaMerge/Segment.cpp Show resolved Hide resolved
Comment on lines 115 to 119
if (writer)
writer->finalize();
writer.reset();
file_writting->enableGC();
file_writting.reset();
Copy link
Contributor

Choose a reason for hiding this comment

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

Put IO related logic here is quite weird and difficult to understand. Better do it clearly.
And I think writer->finalize() is not very necessary. While file_writting->enableGC() can be called after the first chunk committed in PageStorage.

}
for (const auto & file_id : file_ids)
{
DMFilePtr file = DMFile::restore(file_id, 0, parent_path);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we remove no gc file for DMFile while restoring DeltaValueSpace?

@JaySon-Huang JaySon-Huang changed the title WIP: [Flash 763] Imporve delta's performance by DMFile DNM: [Flash 763] Imporve delta's performance by DMFile Jan 8, 2020
@JaySon-Huang
Copy link
Contributor Author

This PR will make memory usage rapidly increase while size of data increase. Close it.
See https://internal.pingcap.net/jira/browse/FLASH-763 for details

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants