Skip to content

Add unified cache#1

Open
NikitaEvs wants to merge 22 commits intomasterfrom
add-unified-cache
Open

Add unified cache#1
NikitaEvs wants to merge 22 commits intomasterfrom
add-unified-cache

Conversation

@NikitaEvs
Copy link
Copy Markdown
Owner

@NikitaEvs NikitaEvs commented Jan 17, 2023

Summary

PoC of the unified cache model with new/delete integration, custom Allocator support and Uncompressed cache and Marks cache integrations.

Features

  • Allocations which go through the new/delete methods or custom BuddyAllocator instances can trigger cache entries eviction from the Uncompressed cache and Marks cache
  • All allocations discussed above use the global memory arena that is initialized on the startup
  • The trigger for evictions is set up using a threshold of the consumed memory

Main known problems

  • Memory tracker isn't working with this schema
  • There are async evictions so there is no guarantee that memory will be released instantly in case of a memory shortage
  • Majority of allocations are done through custom Allocator instances (for example PODArray use Allocator class), so it is needed to change Allocator instances to BuddyAllocator to make these structures use BuddyArena
  • Buddy allocator schema isn't very efficient
  • Potential high contention on the global cache policy and global allocator state
  • Some allocations can be done aside from BuddyArena (e.g. static constructors cannot use BuddyArena)

Implementation details

The high-level architecture of the PoC:
caches (3)

More specific diagram:
caches (8)

You can find components from the picture above in the following places:

Test plan

The main components of the allocator were tested using separate unit tests, WIP.

Benchmarks

WIP

Comment thread src/Common/memory.h Outdated
Comment thread src/Common/memory.h Outdated
Comment thread programs/server/Server.cpp Outdated

StackTrace::setShowAddresses(config().getBool("show_addresses_in_stack_traces", true));

const size_t buddy_arena_size = 2048 * (1ull << 20); // 2024 MB
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

(could be written more succinctly using the KiB/MiB/GiB suffix literals in base/base/units.h)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

(not urgent but it would be nice if we could ask the OS for the total available memory here and then allocate a percentual fraction of it, e.g. 90%)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I guess l. 659, l. 660 and l. 662 should be moved into the (private) constructor of the buddy arena (which get's called once when BuddyArena::instance() is first called)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Should we move the allocator arena parameters inside the private constructor? We can otherwise configure it using settings

Comment thread src/Common/CacheBase.h Outdated
Comment thread src/Common/UnifiedCache.h Outdated

char * initializeMetaStorage()
{
// Calculate sizes
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For the final version, it would be nice to add a comment that explains the layout of the meta/directory structure.

Comment thread src/Common/UnifiedCache.h Outdated
meta_storage_size_round_up_to_power_of_2 *= 2;
}

/// Deallocate minimal blocks to fill the space between the meta storage and the size that is
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I am not sure if I understand the reason for below code in this method.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Added more detailed comments, will add more in the final version

Comment thread src/Common/UnifiedCache.h Outdated
{
auto level = calculateLevel(size);

std::lock_guard lock(mutex);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In case the locking becomes a bottleneck, you could try our new futex-based lock implementation (ClickHouse#44924). Or, alternatively, stride the lock, i.e. introduce multiple locks (e.g. one per equally-large range of the lowest level of the buddy allocator)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We do not have a faster std::mutex reimplementation yet, only faster std::shared_mutex (i.e. DB::SharedMutex). And I guess we won't get better performance than ordinary std::mutex in the same way.

Comment thread src/Common/UnifiedCache.h Outdated
static void * alloc(size_t size, size_t alignment = 0)
{
checkSize(size);
CurrentMemoryTracker::alloc(size);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I remember our discussion that the memory tracker doesn't know about allocations at runtime using the buddy allocator (i.e. it just sees the initial allocation of the buddy allocator).

You added instrumentation for memory tracking here. Does that solve the issue?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

For now we have a hack with CurrentMemoryTracker::free right after the initial arena allocation. It should add support of memory tracker instrumentation for the project

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.

3 participants