8000 clean up subscriptions & component references by mononaut · Pull Request #5713 · mempool/mempool · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

clean up subscriptions & component references #5713

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jan 9, 2025
Merged

clean up subscriptions & component references #5713

merged 3 commits into from
Jan 9, 2025

Conversation

mononaut
Copy link
Collaborator
@mononaut mononaut commented Jan 6, 2025 •
edited
Loading

Profiling revealed various memory issues involving the block overview component, which could prevent old copies of data used for the block visualization from being properly garbage collected after use:

  • missing rxjs unsubscriptions
    • where rxjs pipelines and subscription callbacks use arrow functions (or bound function expressions), those callbacks capture the lexical scope (including references to the component instance via this).
    • if we fail to unsubscribe in the ngOnDestroy handler, those subscription callbacks are never cleaned up and the references to the component instance are retained indefinitely, preventing the component object from being properly garbage collected after the end of its natural lifecycle.
  • shareReplay:
    • the default behavior of the shareReplay rxjs operator prevents the event source from being automatically unsubscribed, even if all of the downstream subscriptions are correctly cleaned up.
    • if the pipeline up to that point includes any arrow functions or bound function expression callbacks, those may be retained and prevent anything captured in their closures from being properly garbage collected as described above.
    • in those cases, the refCount parameter should be used to ensure the source is unsubscribed at the end of its lifetime, like shareReplay({ bufferSize: N, refCount: true }).
  • implicit block scene cleanup:
    • in various places we relied on the BlockOverviewGraphComponent being correctly garbage collected to clean up the memory intensive block visualization objects instead of proactively clearing this data at the end of its Angular lifecycle.

This PR attempts to resolve these issues by:

  • adding the missing rxjs subscriptions.
  • changing relevant uses of shareReplay to use refCount.
  • explicitly calling destroy() on the BlockScene and FastVertexArray instances in the BlockOverviewGraphComponent.ngOnDestroy handler to clean up that data.

Testing

  • click around between different pages with block visualizations (/mempool-block/...,/block/..., dashboard, acceleration dashboard) for a while.
  • take a heap snapshot.
  • verify that the appropriate number of BlockOverviewGraphComponent and/or BlockScene objects have been retained.

@mononaut mononaut requested a review from softsimon January 6, 2025 21:35
@cla-bot cla-bot bot added the cla-signed label Jan 6, 2025
softsimon
softsimon requested changes Jan 7, 2025
View reviewed changes
Copy link
Member
@softsimon softsimon left a comment •
edited
Loading

Choose a reason for hiding this comment

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

getting a couple of new errors when switching between views.

(Click on the next mempool block, navigate to the last mined block, and navigate back)

Uncaught TypeError: Cannot read properties of null (reading 'dirty')
(block-overview-graph.component.ts:463:28)

Uncaught TypeError: Cannot read properties of null (reading 'fill')
 at FastVertexArray.clearData (fast-vertex-array.ts:69:15)

@mononaut
Copy link
Collaborator Author
mononaut commented Jan 8, 2025

pushed a fix

@mononaut mononaut requested a review from softsimon January 8, 2025 13:11
softsimon
softsimon approved these changes Jan 8, 2025
View reviewed changes
Copy link
Member
@softsimon softsimon left a comment

Choose a reason for hiding this comment

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

tested ACK @ [7740908]

no crashes and the heap doesn't seem to grow uncontrollable after several hours

Screenshot 2025-01-09 at 00 26 34

@softsimon softsimon merged commit c3686a5 into master Jan 9, 2025
25 checks passed
@softsimon softsimon deleted the mononaut/fix-missing-unsubs branch January 9, 2025 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@softsimon softsimon softsimon approved these changes

Assignees
No one assigned
Labels
cla-signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mononaut @softsimon
0