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
[labs/ssr] fix patched directives memory leak #4515
Conversation
Each time a patched directive is encountered, it gets patched again creating a subtle memory leak
🦋 Changeset detectedLatest commit: 7e4bd67 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📊 Tachometer Benchmark ResultsSummarynop-update
render
update
update-reflect
Resultsthis-change
render
update
update-reflect
this-change, tip-of-tree, previous-release
render
update
nop-update
this-change, tip-of-tree, previous-release
render
update
this-change, tip-of-tree, previous-release
render
update
update-reflect
|
The size of lit-html.js and lit-core.min.js are as expected. |
Nice! |
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.
Good find and fix!
I looked into whether it might be better to add the sentinel property in overrideDirectiveResolve
but that lives in lit-html
exported via private-ssr-support so it seems better to keep everything in this ssr package.
Note, this has to be done in a way that is backwards compatible. If lit-html is updated, and ssr is not updated, then overrideDirectiveResolve must continue to be accessible. Thus we add a new "public" private ssr api for patching _$resolve.
Great feedback which has been incorporated. Description has also been updated. Thank you! |
This reverts commit a1971f1.
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
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.
nice work!
Context
A memory leak was reported in Lit SSR.
Found by testing with #4514, and going test by test.
repeat
directive test contained this leak. After this PR the directive test no longer leaks.So far no other test shows memory leak characteristics.
Why?
Because we have a cache of
WeakMap<non patched directive, patched directive>
. This is an issue because we can pull a patched directive off thevalue
, and then it won't be found in the WeakMap. Because it's in the value of the WeakMap and not thekey
. Thus the patched directive gets patched again and stored in the WeakMap. This repeats for the number of patch calls resulting in a nested doll of patched directive constructors.Fix
I implemented a super simple naive fix. Just flag the patched directive ctor with a sentinel property. Now when pulling the directive constructor off thevalue
, we can immediately check if it's patched by looking at the sentinel.Fix was updated per Justin's feedback. It now patches the
_$resolve
field. This simplifies a lot of the book keeping code. Which has a much nicer memory footprint.However, this fix does need to be backwards compatible hence the new API,
patchDirectiveResolve
. This allows for the following:overrideDirectiveResolve
.patchDirectiveResolve
, v3.1.2 (unreleased).Test plan
No memory test for this change because memory tests are expensive and slow. Instead this is covered by existing behavior tests. I can add tests if required.
The memory characteristics of this change were verified by pulling this change into #4514
Plan is to commit a memory test as a separate PR.