Experiment: atomic buf page fields - #6130
Conversation
| @return true if the state is BUF_BLOCK_MEMORY, or false. */ | ||
| [[nodiscard]] bool is_memory() const noexcept { | ||
| return state == BUF_BLOCK_MEMORY; | ||
| return state.load(std::memory_order_seq_cst) == BUF_BLOCK_MEMORY; |
There was a problem hiding this comment.
seq_cst is default, do we want to specify it?
| << ",\"old\":" << page.old | ||
| << ",\"first_accessed\":" << time_elapsed(page.access_time) | ||
| << ",\"old\":" << page.old << ",\"first_accessed\":" | ||
| << time_elapsed(page.access_time.load(std::memory_order_seq_cst)) |
There was a problem hiding this comment.
Here we are not using relaxed, while in other peek reads we do - why?
| atomic, but was never protected by block_mutex to begin with (see | ||
| its invariant at the field declaration and buf_block_modify_clock_inc() | ||
| in buf0buf.ic) -- this read is an inherently best-effort snapshot, | ||
| reconfirmed below (line ~4796) under a real latch, same as before |
| checked earlier. This check is to reduce contention on page mutex | ||
| for hot pages (access time would be set only if it was zero anyway). */ | ||
| if (access_time == std::chrono::steady_clock::time_point{}) { | ||
| buf_page_mutex_enter(block); |
There was a problem hiding this comment.
Why it is still here?
Shouldn't all this code, access_time local variable, all the comments etc be removed?
Just check if the field is empty and call buf_page_set_accessed - I don't understand why we are doing all that here.
buf_page_set_accessed should return bool if the access time was changed out from empty.
A local bool was_not_accessed_before{} should be added and set to the buf_page_set_accessed result.
Furthermore, look at all usages of buf_page_set_accessed. Some have similar pattern:
if (access_time == std::chrono::steady_clock::time_point{}) {
buf_page_mutex_enter(block);
buf_page_set_accessed(&block->page);
buf_page_mutex_exit(block);
}
We could rewrite them to buf_page_set_accessed(&block->page); only.
Also, the if (access_time == std::chrono::steady_clock::time_point{}) { should be inside the buf_page_set_accessed itself - doing costly CAS directly alone would not be best.
This all is not a must, the code works as it is now, but it seems we could prune more mutex enters from hot-ish paths and remove a lot of code.
| /* Make this the time of the first access. */ | ||
| bpage->access_time = std::chrono::steady_clock::now(); | ||
| } | ||
| /* Make this the time of the first access, unless access_time is already |
There was a problem hiding this comment.
unconditional CAS is more costly than a check first, especially when we assume the CAS will fail most of the time - the value will usually not be empty.
This is a problem in buf_page_get_known_nowait most probably - a test using a lot of AHI could regress.
This comment is very related to my longer review comment in the optimistic get function.
Add a global acquire fence in place of the removed mutex to preserve the memory ordering guarantee for the state/modify_clock check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
bc194b8 to
95c183a
Compare
2 participants
No description provided.