Skip to content
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 unnecessary memory retention paths #1232

Merged
merged 2 commits into from
Jul 2, 2022
Merged

Conversation

mhofman
Copy link
Contributor

@mhofman mhofman commented Jul 1, 2022

While investigating Agoric/agoric-sdk#5447, I found that endo was possibly unnecessarily holding onto memory in a couple places. This PR fixes them:

  • SES's v8 Error taming no longer holds onto the raw CallSite stack for error objects after it has created a string for them. It does so by keeping a second WeakMap with the generates stack string, and nulls out the content of the first WeakMap. That way the stack is still processed lazily.
  • stream-node possibly held onto the finalResolution promise and other things in context even after the stream closed.

@mhofman mhofman requested review from kriskowal and erights July 1, 2022 20:21
Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New rule added to head canon: Law of the Conservation of Event Handles: every on must be balanced by an equal but opposite off.

@mhofman
Copy link
Contributor Author

mhofman commented Jul 1, 2022

New rule added to head canon: Law of the Conservation of Event Handles: every on must be balanced by an equal but opposite off.

It's preferable.

@erights I'll wait for you to review the v8 Error taming changes

Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the writer.js changes, I have never used .on or .off but see they are inherited from EventEmitter, which is fine since this whole file is Node specific. But is the problem Node specific? Should we have some Writer-level cleanup that applies to all Writers, as opposed to just Writables?

@mhofman
Copy link
Contributor Author

mhofman commented Jul 1, 2022

@erights the cleanup for iterator based streams is to use return or throw on the iterator. The thing is node can be a little finicky, and for some reason I can't recall, we had to add listeners directly on the node writable stream as well. However those have cleanup semantics which were not respected here. I'm not saying this listener cleanup was entirely necessary, just that it's always better to do an explicit cleanup if you can.

In this PR, I think the more meaningful change is to the v8 error stack taming.

packages/ses/src/error/tame-v8-error-constructor.js Outdated Show resolved Hide resolved
packages/ses/src/error/tame-v8-error-constructor.js Outdated Show resolved Hide resolved
packages/ses/src/error/tame-v8-error-constructor.js Outdated Show resolved Hide resolved
packages/ses/src/error/tame-v8-error-constructor.js Outdated Show resolved Hide resolved
@mhofman
Copy link
Contributor Author

mhofman commented Jul 1, 2022

@erights PTAL, I decided to redo the logic to make clear we either have a raw structured stack trace or a parsed string.

Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New unified map is better. LGTM

After the stack string is generated, discard the structured stack objects
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants