Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Introduce Thread-local storage variable to replace atomic when update used_memory metrics to reduce contention. #308
base: unstable
Are you sure you want to change the base?
Introduce Thread-local storage variable to replace atomic when update used_memory metrics to reduce contention. #308
Changes from 1 commit
69676d4
bc4cc6e
45071b5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do something like this? I think we need to implement a new define for BIO thread count. Then we don't have to worry about this changing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IO_THREADS_MAX_NUM
is defined innetworking.c
andBIO_THREAD_COUNT
has a similar defineBIO_WORKER_NUM
inbio.c
. How do we reuse the define in thezmalloc.c
? It is a very low level API.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah there is a layering concern here. Can you add a
c_assert
to make sure the condition of(IO_THREADS_MAX_NUM + BIO_THREAD_COUNT + 1) < IO_THREADS_MAX_NUM
is not violated?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, if we go with this route, we should probably introduce a new header to host all thread local storage related definition. It feels a bit out of place to define
MAX_THREADS_NUM
in zmalloc.c. We might have other consumers of this macro in the future. I wonder if we should call the new headersystem.h
, which would be at the lowest layer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since both
total_active_threads
andthread_index
are ofsize_t
, whose sizes match the native word size of the platform, the proposed change should work.Strictly speaking though (if
size_t
was 64 bit on a 32-bit platform), L112 and L113 need to be flipped. We also shouldn't touch the global vartotal_active_threads
without the atomic operation; instead, we should access the thread local varthread_index
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering how important is it for zmalloc_used_memory to return the correct used_memory? As we loop through to add up the memory used by each thread, the threads that had been counted may continue to allocate or to free memory, so the sum might under or over account the actual memory used by all threads.
I don't think this is an issue if the function is only used for metrics, but overMaxmemoryAfterAlloc (evict.c) is not for metrics, we probably should ensure (and call out) that undercounting memory usage would not be an issue there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@qlong I understand your concern, at the beginning, I want to keep an approximate value for used_memory, each thread keep its own used_memory_thread, only used_memory_thread greater than a threshold(e.g. 1k) then update to global atomic used_memory, the pseudocode will be like this, but I saw the test case in https://github.com/valkey-io/valkey/blob/unstable/src/zmalloc.c#L917, seems it need an accurate used_memory? Back to this PR, each element in used_memory_thread array only have one writer to update the value, when read, the reader may have gap between
oldvalue
andnewvalue
, depends on the read timing.@madolson What's your thought?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few quick thoughts
size_t
here for the counters, which should be natively aligned as well, the worst we can get would be staleness as opposed to inconsistency.INFO
command returns, whether it is absolutely accurate or not. As long as the value is close enough I think we are good. We can change the testsoverMaxmemoryAfterAlloc
is a higher bar than metrics. My gut feeling is that the staleness brought by the relaxed memory access should still be negligible. I doubt we could accumulate a lot of changes before the CPU flushes out its cache to the main memory. Curious to know if anyone has more definitive information.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems we all had concern about the absolute accuracy of
used_memory
inoverMaxmemoryAfterAlloc
, IMO, this is not a problem, because in the definition ofoverMaxmemoryAfterAlloc
, the comparison betweenmoremem
andused_memory
(Their semantics are different) is not absolute accuracy if we consider the gap between requested memory and actual allocated memory from allocator.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kindly ping @PingXie , any comments about this, do we need to continue this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a way to address this concern while retaining the most of the performance benefit if we keep the existing io-threading model. The idea would be to keep a mirrored per-thread counter array such that each thread periodically "flushes" its thread local counter value to the corresponding element in the global per-thread counter array using atomic operations. This would work with the current io-threading model where io-threads and main threads run in lock-steps. Every io thread would need to push its local value to this global array when it is done with its work, but only for once. Then by the time the main thread starts executing, it would have the accurate data. We can reduce the number of atomic operations to one per io-thread per epoll session.
We would have to revisit this if we went for a different threading model (such as #22) .
That said, I am still not sure how accurate this information needs to be.
zmalloc_used_memory
doesn't represent the RSS usage of the process anyways. I think it would be really helpful if we could somehow establish the worst possible case, empirically or even a SWAG.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, there is no meaningful of the absolute accurate
zmalloc_used_memory
, to achieve the goal, the cost is very high, not only the expensive atomic operation in everyzmalloc/zfree
operation, but also the call ofje_malloc_usable_size
is very high (From the cycles hotspot snapshot in top comment, you can find the cycles ratio ofje_malloc_usable_size
is the top 1 hotspot). If we can accept the a little latency ofzmalloc_used_memory
, ideally we may have at least 10% perf gain.@PingXie Do you think we need open an issue to discuss with the rest of @valkey-io/core-team of the necessary of absolute accurate
zmalloc_used_memory
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw you opened #467. Thanks @lipzhu.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.