-
Notifications
You must be signed in to change notification settings - Fork 229
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
fix(notifier)!: Stop retaining durable publish kit values in RAM #7459
fix(notifier)!: Stop retaining durable publish kit values in RAM #7459
Conversation
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 all looks quite sensible to me, but it's far enough outside my wheelhouse that I'm not comfortable giving it a thumbs up (or down) just on my own. So consider this a kinda-sorta approval conditional on someone whose promise-fu is greater than mine giving it their blessing.
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. IIUC, avoiding keeping currentP
around where possible and reconstituting it when necessary is what makes this change use less memory.
I'll defer to your preference on whether if
or switch
is better for dispatching state.status
.
assert(status === 'live'); | ||
return tail; | ||
} | ||
if (status === 'live' || status === 'finished') { |
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.
Maybe use switch (status)
here?
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.
Considered, but rejected because I want provideCurrentP
and provideDurablePublishKitEphemeralData
to remain compact.
const { status } = state; | ||
let tailP; | ||
let tailR; | ||
if (status === 'live') { |
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.
Again?
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 admittedly do not grok enough the publisher/subscribers to consider my review authoritative, but I have concerns about the handling of non-durable finish values and fail reasons. Arguably this was a problem before, but I have a hard time figuring out the state in which it leaves the ephemeral data.
On the memory usage, an empirical test seem to indicate we no longer hold virtual objects through the ephemeral data, so that's good! Of course we do still have a bunch of heap promise per publisher, but not much to do about this until we implement pseudo-promises.
packages/notifier/src/publish-kit.js
Outdated
* @property {Promise<*> | undefined} currentP The current-result promise | ||
* (undefined unless resolved with unrecoverable ephemeral data) | ||
* @property {Promise<*>} tailP The next-result promise | ||
* @property {import('@endo/promise-kit').PromiseKit<*>['resolve'] | undefined} tailR The next-result resolver |
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.
Why import the resolve definition? It's pretty standard:
* @property {import('@endo/promise-kit').PromiseKit<*>['resolve'] | undefined} tailR The next-result resolver | |
* @property {((value: *) => void) | undefined} tailR The next-result resolver |
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.
Done.
packages/notifier/src/publish-kit.js
Outdated
const makeTooFarRejection = () => | ||
makeQuietRejection(new Error('Cannot read past end of iteration.')); |
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.
So I actually found one more leak, at least in v8. Apparently the Error
s structured stack keeps references to each call site, including in this case the context object of the finish
call. That object is the {state, facets}
context of the notifier exo kit. While this is most likely not a problem in XS, and likely applies to other Exo class based patterns, I figured it'd be easy enough to fix in this case since I don't believe there is much value in capturing the stack of when the publisher was finished.
const makeTooFarRejection = () => | |
makeQuietRejection(new Error('Cannot read past end of iteration.')); | |
const tooFarRejection = harden( | |
makeQuietRejection(new Error('Cannot read past end of iteration.')), | |
); |
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.
Fixed.
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.
So the reason I suggested to harden
was because the rejection would be shared instead of built for each publisher when reaching a terminal state.
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, makes sense. And makeQuietRejection
already hardens the returned promise.
Reduces memory leakage, cf. #7459 (comment)
c4f17bd
to
ac1d604
Compare
Reduces memory leakage, cf. #7459 (comment)
ac1d604
to
c5b950b
Compare
Reduces memory leakage, cf. #7459 (comment)
c5b950b
to
af34ebb
Compare
Reduces memory leakage, cf. #7459 (comment)
af34ebb
to
e5372b3
Compare
Reduces memory leakage, cf. #7459 (comment)
e5372b3
to
2fd53e6
Compare
Reduces memory leakage, cf. #7459 (comment)
Fixes #7298 This change is breaking only because the identity of promises and `head` objects is no longer preserved.
good hygiene and avoids leaking some memory
Reduces memory leakage, cf. #7459 (comment)
2fd53e6
to
7764e36
Compare
deploy-script-support can't have nice things
Fixes #7298
Description
Refactor ephemeral data to retain a promise resolved to the current value only when that value is ephemeral, which is currently prohibited by the restriction of valueDurability to 'mandatory'.
Security Considerations
n/a
Scaling Considerations
This PR reduces RAM consumption by allowing durable promise kit resolution values to be paged out.
Documentation Considerations
This change is breaking only because the identity of promises and
head
objects is no longer preserved, but I don't believe we ever promised such preservation and it isn't observable across vat boundaries anyway.Testing Considerations
I still want a test that verifies lack of retention, and am open to ideas regarding the best pattern for that.