-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
(2.12) Elastic pointers in the filestore write-through cache #7180
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
Conversation
f9a8e76
to
c84f772
Compare
Want to investigate some flaking tests first before review. |
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.
In general looking better. A few concerns around some logic.
// The reference mb.cache shouldn't have changed ordinarily, but | ||
// stash it away into the weak pointer again just in case. | ||
mb.ecache.Set(mb.cache) | ||
mb.cache = nil |
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.
We have two levels in the cache, the indexing and the data itself. Both IIRC are under mb.cache but they have different lifetimes under normal operations. This seems to undo that?
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.
They're still held in the ecache
until it is GC'd (and for that there would need to be no strong references left too), but if that happens, we'll just reload the block and re-index it the next time we need to.
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.
fss is held outside this scope. Not sure if we want this extended to that in a follow on PR or not.
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.
Yep, I can look into it as a follow-up for sure.
mbcache.wp = 0 | ||
} | ||
|
||
mb.ecache.Weaken() |
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.
This a no-op if line 5557 excuted?
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.
Yes, it would have no effect in that case.
55e5157
to
ee49624
Compare
"weak" | ||
) | ||
|
||
func Make[T any](ptr *T) *Pointer[T] { |
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 this is unused, can it be removed?
// can reuse. | ||
if mb.cacheAlreadyLoaded() { | ||
return | ||
} |
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.
mb.cache
is initialized below, but then mb.ecache
is not set to it?
This would probably lead to the mb.ecache.Strengthen()
call not actually doing work.
Signed-off-by: Neil Twigg <[email protected]>
ee49624
to
789c199
Compare
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.
LGTM
This PR modifies the write-through cache in the filestore to use elastic pointers. Elastic pointers are weak by default and can be reclaimed by the GC under pressure, but can be strengthened when there are pending writes for a block. Once the flush has completed, we weaken the cache again.
There are now two cache references within each
msgBlock
:mb.ecache
: The elastic pointer, populated when the cache is set up and only reset back tonil
either by the GC or whenclearCache
is calledmb.cache
: Left asnil
by default, populated with a strong reference frommb.ecache
when we need to use the cache, and then reset back tonil
after we're done with itThis PR replaces and therefore closes #7097.
Signed-off-by: Neil Twigg [email protected]