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.

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

[@lit-labs/preact-signals]: Fix memory leak after disconnected element updates. #4740

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[@lit-labs/preact-signals]: Fix memory leak after disconnected element updates. #4740

wants to merge 1 commit into from

Conversation

sorvell
Copy link
Member

@sorvell sorvell commented Aug 21, 2024

Fixes #4735. The signal effect in SignalWatcher references the element weakly, and the signal subscription in watch also does so for the directive. This is accomplished via WeakRef, which is widely supported.

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Aug 21, 2024

🦋 Changeset detected

Latest commit: 31330ae

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@lit-labs/preact-signals Patch

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

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 21, 2024 •
edited
Loading

📊 Tachometer Benchmark Results

Summary

nop-update

  • this-change, tip-of-tree, previous-release: unsure 🔍 -5% - +4% (-0.58ms - +0.51ms)
    this-change vs tip-of-tree

render

  • this-change: 43.58ms - 44.76ms
  • this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +5% (-0.19ms - +0.92ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +1% (-0.59ms - +0.30ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +3% (-0.24ms - +0.84ms)
    this-change vs tip-of-tree

update

  • this-change: 464.97ms - 473.25ms
  • this-change, tip-of-tree, previous-release: unsure 🔍 -5% - +8% (-1.69ms - +2.80ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +2% (-1.62ms - +1.50ms)
    this-change vs tip-of-tree
  • this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +2% (-5.84ms - +7.31ms)
    this-change vs tip-of-tree

update-reflect

  • this-change: 466.98ms - 476.23ms
  • this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +2% (-6.81ms - +7.94ms)
    this-change vs tip-of-tree

Results

this-change

render

VersionAvg timevs
43.58ms - 44.76ms-

update

VersionAvg timevs
464.97ms - 473.25ms-

update-reflect

VersionAvg timevs
466.98ms - 476.23ms-
this-change, tip-of-tree, previous-release

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree		vs previous-release
previous-release
this-change
18.38ms - 19.13ms-unsure 🔍
-1% - +5%
-0.19ms - +0.92ms		unsure 🔍
-4% - +3%
-0.70ms - +0.50ms
tip-of-tree
tip-of-tree		17.99ms - 18.80msunsure 🔍
-5% - +1%
-0.92ms - +0.19ms		-unsure 🔍
-6% - +1%
-1.09ms - +0.15ms
previous-release
previous-release		18.39ms - 19.33msunsure 🔍
-3% - +4%
-0.50ms - +0.70ms		unsure 🔍
-1% - +6%
-0.15ms - +1.09ms		-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree		vs previous-release
previous-release
this-change
35.71ms - 38.85ms-unsure 🔍
-5% - +8%
-1.69ms - +2.80ms		unsure 🔍
-6% - +6%
-2.39ms - +2.30ms
tip-of-tree
tip-of-tree		35.12ms - 38.33msunsure 🔍
-7% - +4%
-2.80ms - +1.69ms		-unsure 🔍
-8% - +5%
-2.97ms - +1.77ms
previous-release
previous-release		35.58ms - 39.06msunsure 🔍
-6% - +6%
-2.30ms - +2.39ms		unsure 🔍
-5% - +8%
-1.77ms - +2.97ms		-

nop-update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree		vs previous-release
previous-release
this-change
11.33ms - 12.03ms-unsure 🔍
-5% - +4%
-0.58ms - +0.51ms		unsure 🔍
-3% - +5%
-0.38ms - +0.58ms
tip-of-tree
tip-of-tree		11.30ms - 12.13msunsure 🔍
-4% - +5%
-0.51ms - +0.58ms		-unsure 🔍
-3% - +6%
-0.39ms - +0.67ms
previous-release
previous-release		11.25ms - 11.91msunsure 🔍
-5% - +3%
-0.58ms - +0.38ms		unsure 🔍
-6% - +3%
-0.67ms - +0.39ms		-
this-change, tip-of-tree, previous-release

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree		vs previous-release
previous-release
this-change
33.26ms - 33.90ms-unsure 🔍
-2% - +1%
-0.59ms - +0.30ms		unsure 🔍
-1% - +1%
-0.43ms - +0.44ms
tip-of-tree
tip-of-tree		33.42ms - 34.03msunsure 🔍
-1% - +2%
-0.30ms - +0.59ms		-unsure 🔍
-1% - +2%
-0.28ms - +0.58ms
previous-release
previous-release		33.27ms - 33.87msunsure 🔍
-1% - +1%
-0.44ms - +0.43ms		unsure 🔍
-2% - +1%
-0.58ms - +0.28ms		-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree		vs previous-release
previous-release
this-change
66.21ms - 68.27ms-unsure 🔍
-2% - +2%
-1.62ms - +1.50ms		unsure 🔍
-2% - +2%
-1.42ms - +1.50ms
tip-of-tree
tip-of-tree		66.13ms - 68.47msunsure 🔍
-2% - +2%
-1.50ms - +1.62ms		-unsure 🔍
-2% - +2%
-1.46ms - +1.67ms
previous-release
previous-release		66.16ms - 68.23msunsure 🔍
-2% - +2%
-1.50ms - +1.42ms		unsure 🔍
-2% - +2%
-1.67ms - +1.46ms		-
this-change, tip-of-tree, previous-release

render

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree		vs previous-release
previous-release
this-change
30.88ms - 31.68ms-unsure 🔍
-1% - +3%
-0.24ms - +0.84ms		unsure 🔍
-1% - +2%
-0.41ms - +0.73ms
tip-of-tree
tip-of-tree		30.61ms - 31.35msunsure 🔍
-3% - +1%
-0.84ms - +0.24ms		-unsure 🔍
-2% - +1%
-0.70ms - +0.41ms
previous-release
previous-release		30.71ms - 31.53msunsure 🔍
-2% - +1%
-0.73ms - +0.41ms		unsure 🔍
-1% - +2%
-0.41ms - +0.70ms		-

update

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree		vs previous-release
previous-release
this-change
474.78ms - 483.85ms-unsure 🔍
-1% - +2%
-5.84ms - +7.31ms		unsure 🔍
-1% - +1%
-6.44ms - +6.22ms
tip-of-tree
tip-of-tree		473.82ms - 483.34msunsure 🔍
-2% - +1%
-7.31ms - +5.84ms		-unsure 🔍
-2% - +1%
-7.34ms - +5.65ms
previous-release
previous-release		475.01ms - 483.84msunsure 🔍
-1% - +1%
-6.22ms - +6.44ms		unsure 🔍
-1% - +2%
-5.65ms - +7.34ms		-

update-reflect

VersionAvg timevs this-change
vs tip-of-tree
tip-of-tree		vs previous-release
previous-release
this-change
511.24ms - 521.61ms-unsure 🔍
-1% - +2%
-6.81ms - +7.94ms		unsure 🔍
-1% - +1%
-6.83ms - +7.60ms
tip-of-tree
tip-of-tree		510.62ms - 521.10msunsure 🔍
-2% - +1%
-7.94ms - +6.81ms		-unsure 🔍
-1% - +1%
-7.44ms - +7.07ms
previous-release
previous-release		511.03ms - 521.06msunsure 🔍
-1% - +1%
-7.60ms - +6.83ms		unsure 🔍
-1% - +1%
-7.07ms - +7.44ms		-

tachometer-reporter-action v2 for Benchmarks

@github-actions GitHub Actions
Copy link
Contributor

The size of lit-html.js and lit-core.min.js are as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinpschaaf kevinpschaaf Awaiting requested review from kevinpschaaf kevinpschaaf is a code owner

@justinfagnani justinfagnani Awaiting requested review from justinfagnani

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[@lit-labs/preact-signals] Memory leak if element updates while disconnected
1 participant
@sorvell