Skip to content

Make ReactiveSet.clear() and ReactiveMap.clear() more efficient by only notifying listeners of existing members #784

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

Merged
merged 2 commits into from
Apr 23, 2025

Conversation

webstrand
Copy link
Contributor

@webstrand webstrand commented Apr 2, 2025

Listeners of non-existing members cannot observe different values before or after .clear() .get(nonexsting) -> undefined or .has(nonexisting) -> false so there's no use in notifying them.

Copy link

changeset-bot bot commented Apr 2, 2025

🦋 Changeset detected

Latest commit: 1d1c686

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

This PR includes changesets to release 2 packages
Name Type
@solid-primitives/set Minor
@solid-primitives/map Minor

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

this.#triggers.dirty($KEYS);
for (const member of super.values()) {
this.#triggers.dirty(member);
}
super.clear();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did some benchmarking and did not see a performance hit to moving super.clear() into the batch operation. But if you like, I can save a snapshot prior to .clear() and iterate over that in batch.

@atk
Copy link
Member

atk commented Apr 22, 2025

This is still missing a changeset. Run npx changeset in the project root.

@webstrand
Copy link
Contributor Author

Ah, I had no idea that message was for me and not the maintainers. Fixed.

@atk atk merged commit f5f0c4e into solidjs-community:main Apr 23, 2025
3 checks passed
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.

2 participants