timeseries: fix memory leaks involving CardObserver and shareReplay #5254
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bmd3k
reviewed
Aug 18, 2021
| @@ -305,7 +306,7 @@ export class RunsTableContainer implements OnInit, OnDestroy { | |||
|
|
|||
| const getFilteredItems$ = this.getFilteredItems$( | |||
| this.allUnsortedRunTableItems$ | |||
| ).pipe(shareReplay(1)); | |||
| ).pipe(takeUntil(this.ngUnsubscribe), shareReplay(1)); | |||
There was a problem hiding this comment.
Do you mind putting a comma after shareReplay(1) here so it formats on multiple lines?
There was a problem hiding this comment.
I just tried, but unfortunately Prettier seems to undo the comma and remove the multiline, I think because this.getFilteredItems$(this.allUnsortedRunTableItems$) did not fit onto one line.
bmd3k
approved these changes
Aug 18, 2021
|
Thanks for the review! |
yatbear
pushed a commit
to yatbear/tensorboard
that referenced
this pull request
Mar 27, 2023
…ensorflow#5254) This addresses 2 non-trivial sources of memory leaks observed when filtering tags in the Time Series dashboard. a) CardObserver used to track Elements that were destroyed via ngOnDestroy and stored them in a `Set<Element>`. Now with a WeakSet instead, we cover cases when the browser's IntersectionObserverEntry is not handled for some reason. b) The rxjs operator "shareReplay" has some known gotchas [1], [2]. When more observables subscribe to a source containing `shareReplay(1)` in its operator chain, the source remains subscribed even after the other observables unsubscribe. This change uses the `takeUntil(ngUnsubscribe$)` pattern to ensure any component using a `shareReplay` will not keep it subscribed after the component is gone. Alternatively we could use `refCount`, but this opts for the `takeUntil` pattern already used widely in the codebase. Manually checked that the dashboard still operates properly, and in Angular's dev mode, using a specific demo logdir, adding and clearing a "." tag filter has these characteristics: Before this change: 5.8 MB, 5.5K DOM nodes added each cycle After this change: 0.05 MB, <10 DOM nodes added each cycle Googlers, see b/196996936 [1] ReactiveX/rxjs#5034 [2] ReactiveX/rxjs#5931
dna2github
pushed a commit
to dna2fork/tensorboard
that referenced
this pull request
May 1, 2023
…ensorflow#5254) This addresses 2 non-trivial sources of memory leaks observed when filtering tags in the Time Series dashboard. a) CardObserver used to track Elements that were destroyed via ngOnDestroy and stored them in a `Set<Element>`. Now with a WeakSet instead, we cover cases when the browser's IntersectionObserverEntry is not handled for some reason. b) The rxjs operator "shareReplay" has some known gotchas [1], [2]. When more observables subscribe to a source containing `shareReplay(1)` in its operator chain, the source remains subscribed even after the other observables unsubscribe. This change uses the `takeUntil(ngUnsubscribe$)` pattern to ensure any component using a `shareReplay` will not keep it subscribed after the component is gone. Alternatively we could use `refCount`, but this opts for the `takeUntil` pattern already used widely in the codebase. Manually checked that the dashboard still operates properly, and in Angular's dev mode, using a specific demo logdir, adding and clearing a "." tag filter has these characteristics: Before this change: 5.8 MB, 5.5K DOM nodes added each cycle After this change: 0.05 MB, <10 DOM nodes added each cycle Googlers, see b/196996936 [1] ReactiveX/rxjs#5034 [2] ReactiveX/rxjs#5931
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses 2 non-trivial sources of memory leaks observed when
filtering tags in the Time Series dashboard.
a) CardObserver used to track Elements that were destroyed via
ngOnDestroy and stored them in a
Set<Element>. Now with a WeakSetinstead, we cover cases when the browser's IntersectionObserverEntry
is not handled for some reason.
b) The rxjs operator "shareReplay" has some known gotchas [1], [2].
When more observables subscribe to a source containing
shareReplay(1)in its operator chain, the source remains subscribedeven after the other observables unsubscribe. This change uses the
takeUntil(ngUnsubscribe$)pattern to ensure any component using ashareReplaywill not keep it subscribed after the component is gone.Alternatively we could use
refCount, but this opts for thetakeUntilpattern already used widely in the codebase.
Manually checked that the dashboard still operates properly, and in
Angular's dev mode, using a specific demo logdir, adding and clearing a
"." tag filter has these characteristics:
Before this change: 5.8 MB, 5.5K DOM nodes added each cycle
After this change: 0.05 MB, <10 DOM nodes added each cycle
Googlers, see b/196996936
[1] ReactiveX/rxjs#5034
[2] ReactiveX/rxjs#5931