Log::Metadata should put parent entries first on extend (like Hash#merge)
#16098
Conversation
This will reduce elements that needs to be copied to only
non-overwritten parent elements.
One result of this is that an overwritten parent entry will be moved
from the "parent" part of the Metadata to the "own" part. E.g.
`{a: 1, b: 2} extend {a: 3}` results in `{b: 2, a:3}`
spuun
force-pushed
the
log-metadata-extend
branch
from
779ff50 to
e31c739
Compare
src/log/metadata.cr
Outdated
| parent_size = (@parent.try(&.max_total_size) || 0) | ||
| @max_total_size = @size + parent_size |
There was a problem hiding this comment.
polish: The variable parent_size is a bit unfortunate to have the same name as @parent_size, but the two differ semantically. Maybe we can rename this to parent_max_total_size or just inline it back as it was before. We don't necessarily need this variable.
src/log/metadata.cr
Outdated
| # | ||
| # * @parent.nil? signals if the defrag is needed/done | ||
| # * The values of @overridden_size, pointerof(@first) are never changed | ||
| # * The value of pointerof(@first) are never changed |
There was a problem hiding this comment.
| # * The value of pointerof(@first) are never changed | |
| # * The value of pointerof(@first) never changes. |
src/log/metadata.cr
Outdated
| overwritten = false | ||
| @size.times do |i| | ||
| if ptr_entries[i][:key] == key | ||
| overridden = true | ||
| overwritten = true | ||
| break | ||
| end | ||
| end | ||
|
|
||
| unless overridden | ||
| next_free_entry.value = {key: key, value: value} | ||
| next_free_entry += 1 | ||
| total_size += 1 | ||
| end | ||
| next if overwritten |
There was a problem hiding this comment.
polish: I believe this can be simplified to a single #any? call.
| overwritten = false | |
| @size.times do |i| | |
| if ptr_entries[i][:key] == key | |
| overridden = true | |
| overwritten = true | |
| break | |
| end | |
| end | |
| unless overridden | |
| next_free_entry.value = {key: key, value: value} | |
| next_free_entry += 1 | |
| total_size += 1 | |
| end | |
| next if overwritten | |
| overwritten = Slice.new(ptr_entries, @size).any? { |entry| entry[:key] == key} | |
| next if overwritten |
src/log/metadata.cr
Outdated
| parent_size.times do |i| | ||
| entry = ptr_entries[i + local_size] | ||
| yield({entry[:key], entry[:value]}) | ||
| end |
There was a problem hiding this comment.
suggestion: The local_size offset is static, so we could pull this addition out of the loop.
I'm not sure if this would have any significant performance impact. But it shouldn't hurt either.
Also we can generally simplify using Slice#each. I don't think this should have any performance impact over iterating the pointer indices directly.
| parent_size.times do |i| | |
| entry = ptr_entries[i + local_size] | |
| yield({entry[:key], entry[:value]}) | |
| end | |
| Slice.new(ptr_entries + local_size, parent_size).each do |entry| | |
| yield({entry[:key], entry[:value]}) | |
| end |
|
@spuun there are a number of changes pending. |
Log::Metadatawrite its entries in the beginning of the allocated memory for all entries. When extending parent entries are prepended to the memory.This will change so entries are written to the end of allocated memory to make it possible to write parent entries first when extending the
Metadatainstance.#defragwill move the "local" entries to the end of the parent entries or overwrite any existing parent entry for the current key.Fixes #13360