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

Decoupling local cache function and cache algorithm #38048

Merged
merged 18 commits into from
Aug 11, 2022
Merged

Decoupling local cache function and cache algorithm #38048

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
7f54fa7
Decoupling cache functions and algorithms
KinderRiven Jun 14, 2022
ffaf44c
fix style
KinderRiven Jun 14, 2022
43cf771
better
KinderRiven Jun 14, 2022
c5f9022
fix
KinderRiven Jun 25, 2022
50fd740
fix
KinderRiven Jun 26, 2022
9d83b93
fix rebase
KinderRiven Aug 10, 2022
081cd49
fix style
KinderRiven Jun 27, 2022
61b580a
add note
KinderRiven Jun 27, 2022
164fa1a
fix build and style
KinderRiven Jun 28, 2022
1b01cc8
fix
KinderRiven Jun 29, 2022
d2b5581
fix
KinderRiven Jun 29, 2022
f6a58bf
fix build
KinderRiven Jun 29, 2022
fbaa70b
fix
KinderRiven Jul 28, 2022
76e0aad
fix
KinderRiven Aug 9, 2022
2ae02a4
fix style
KinderRiven Aug 9, 2022
9ba94e6
fix
KinderRiven Aug 10, 2022
9b7f876
fix
KinderRiven Aug 10, 2022
1aa7bbc
fix unique_ptr
KinderRiven Aug 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix build
  • Loading branch information
@KinderRiven
KinderRiven committed Aug 10, 2022
commit f6a58bff4ca1ef3ada0b0b942f675a812dec6e47
6 changes: 0 additions & 6 deletions src/Common/IFileCachePriority.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,14 @@
#pragma once

#include <list>
#include <memory>
#include <mutex>
#include <Core/Types.h>
#include <Common/Exception.h>
#include <Common/FileCache.h>
#include <Common/FileCacheType.h>

namespace DB
{

class FileCache;
class IFileCachePriority;
using FileCachePriorityPtr = std::shared_ptr<IFileCachePriority>;

Expand All @@ -20,9 +17,6 @@ class IFileCachePriority
{
public:
class IIterator;
friend class IIterator;
friend class FileCache;

using Key = FileCacheKey;
using ReadIterator = std::shared_ptr<const IIterator>;
using WriteIterator = std::shared_ptr<IIterator>;
Copy link
Member

@kssenii kssenii Aug 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

may be unique_ptr? or it is shared somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ReadIterator can be unique_ptr. But for WriteIterator, I don't know if it should be set to unique_ptr, because it may be copied with the FileSegment, do we need to invalidate the iterator before copying?

Copy link
Member

Choose a reason for hiding this comment

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

because it may be copied with the FileSegment, do we need to invalidate the iterator before copying?

but file segment cannot be copied

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like this, I tried to modify it, but sometimes we need to use a temporary pointer to keep some code readable, such as the following cases, so I think it may be better to keep it as it is, what do you think?

auto priority_iter = record->second;
priority_iter->use(cache_lock);
result_state = priority_iter->hits() >= enable_cache_hits_threshold ? FileSegment::State::EMPTY : FileSegment::State::SKIP_CACHE;

Copy link
Member

@kssenii kssenii Aug 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

but looks like here auto & is possible, then unique_ptr will be ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to fix it, but I always get an error when compiling involves unordered_map. Maybe there is something wrong with my understanding of uniqueu_ptr :(

Copy link
Member

Choose a reason for hiding this comment

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

may be not use a pointer at all - just an object? (and in FileSegmentCell put std::optional to iterator).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds feasible, do you mean all ReadIterator and WriteIterator as objects? In this case, there will be more revisions, but I may not have much time to make revisions recently (there is a lot of work to deal with in reality), and I will reconsider the relevant implementation when I have time :).

Copy link
Member

Choose a reason for hiding this comment

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

do you mean all ReadIterator and WriteIterator as objects

yes

but I may not have much time to make revisions recently (there is a lot of work to deal with in reality), and I will reconsider the relevant implementation when I have time

ok, let's leave it as it is for now

Expand Down
13 changes: 6 additions & 7 deletions src/Common/LRUFileCachePriority.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@ namespace ErrorCodes
extern const int LOGICAL_ERROR;
}

IFileCachePriority::WriteIterator
LRUFileCachePriority::add(const Key & key, size_t offset, size_t size, std::lock_guard<std::mutex> &) override
IFileCachePriority::WriteIterator LRUFileCachePriority::add(const Key & key, size_t offset, size_t size, std::lock_guard<std::mutex> &)
{
#ifndef NDEBUG
for (const auto & entry : queue)
Expand All @@ -28,7 +27,7 @@ LRUFileCachePriority::add(const Key & key, size_t offset, size_t size, std::lock
return std::make_shared<LRUFileCacheIterator>(this, iter);
}

bool LRUFileCachePriority::contains(const Key & key, size_t offset, std::lock_guard<std::mutex> &) override
bool LRUFileCachePriority::contains(const Key & key, size_t offset, std::lock_guard<std::mutex> &)
{
for (const auto & record : queue)
{
Expand All @@ -38,23 +37,23 @@ bool LRUFileCachePriority::contains(const Key & key, size_t offset, std::lock_gu
return false;
}

void LRUFileCachePriority::removeAll(std::lock_guard<std::mutex> &) override
void LRUFileCachePriority::removeAll(std::lock_guard<std::mutex> &)
{
queue.clear();
cache_size = 0;
}

IFileCachePriority::ReadIterator LRUFileCachePriority::getLowestPriorityReadIterator(std::lock_guard<std::mutex> &) override
IFileCachePriority::ReadIterator LRUFileCachePriority::getLowestPriorityReadIterator(std::lock_guard<std::mutex> &)
{
return std::make_shared<const LRUFileCacheIterator>(this, queue.begin());
}

IFileCachePriority::WriteIterator LRUFileCachePriority::getLowestPriorityWriteIterator(std::lock_guard<std::mutex> &) override
IFileCachePriority::WriteIterator LRUFileCachePriority::getLowestPriorityWriteIterator(std::lock_guard<std::mutex> &)
{
return std::make_shared<LRUFileCacheIterator>(this, queue.begin());
}

size_t LRUFileCachePriority::getElementsNum(std::lock_guard<std::mutex> &) const override
size_t LRUFileCachePriority::getElementsNum(std::lock_guard<std::mutex> &) const
{
return queue.size();
}
Expand Down
3 changes: 2 additions & 1 deletion src/Common/LRUFileCachePriority.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#pragma once

#include <list>
#include <Common/IFileCachePriority.h>

namespace DB
Expand All @@ -10,9 +11,9 @@ namespace DB
class LRUFileCachePriority : public IFileCachePriority
{
private:
class LRUFileCacheIterator;
using LRUQueue = std::list<FileCacheRecord>;
using LRUQueueIterator = typename LRUQueue::iterator;
class LRUFileCacheIterator;

public:
LRUFileCachePriority() = default;
Expand Down