Skip to content

chore: bit of code cleanup #10218

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

Merged
merged 1 commit into from
Jan 18, 2024
Merged

chore: bit of code cleanup #10218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions packages/svelte/src/internal/client/runtime.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,13 +379,15 @@ function execute_signal_fn(signal) {
// If we have more than 16 elements in the array then use a Set for faster performance
// TODO: evaluate if we should always just use a Set or not here?
const full_current_dependencies_set =
current_dep_length > 16 ? new Set(full_current_dependencies) : null;
current_dep_length > 16 && deps_length - current_dependencies_index > 1
Copy link
Member Author

Choose a reason for hiding this comment

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

additional perf optimization

? new Set(full_current_dependencies)
: null;
for (i = current_dependencies_index; i < deps_length; i++) {
const dependency = dependencies[i];
if (
(full_current_dependencies_set !== null &&
!full_current_dependencies_set.has(dependency)) ||
!full_current_dependencies.includes(dependency)
full_current_dependencies_set !== null
? !full_current_dependencies_set.has(dependency)
: !full_current_dependencies.includes(dependency)
Comment on lines +388 to +390
Copy link
Member Author

Choose a reason for hiding this comment

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

this always ran both when the dependency wasn't in the set previously, slowing things down.
I don't know if this was profiled, but if it was and it didn't appear that this was slower, I'm wondering if this kind of perf optimization is worth it in general

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch! I guess we didn't ever get the Set that much, so it wasn't as obvious.

) {
remove_consumer(signal, dependency);
}
Expand Down Expand Up @@ -1084,7 +1086,7 @@ function mark_subtree_children_inert(signal, inert, visited_blocks) {
for (i = 0; i < references.length; i++) {
const reference = references[i];
if ((reference.f & IS_EFFECT) !== 0) {
mark_subtree_inert(references[i], inert, visited_blocks);
mark_subtree_inert(reference, inert, visited_blocks);
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion packages/svelte/tests/signals/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ describe('signals', () => {
};
});

// outside of test function so that they are unowned signals
let count = $.source(0);
let calc = $.derived(() => {
if ($.get(count) >= 2) {
Expand All @@ -207,7 +208,7 @@ describe('signals', () => {
return $.get(count) * 2;
});

test('effect with derived using new derived every time', () => {
test('effect with derived using unowned derived every time', () => {
const log: Array<number | string> = [];

const effect = $.user_effect(() => {
Expand Down