Skip to content

fix(signal): reduce duplicate observer registrations in repeated signal reads#2660

Open
oanlit wants to merge 2 commits intosolidjs:mainfrom
oanlit:fix/signal-dedup-observers
Open

fix(signal): reduce duplicate observer registrations in repeated signal reads#2660
oanlit wants to merge 2 commits intosolidjs:mainfrom
oanlit:fix/signal-dedup-observers

Conversation

@oanlit
Copy link
Copy Markdown

@oanlit oanlit commented Apr 2, 2026

Summary

When a signal is read repeatedly within the same computation (e.g. inside loops),
the current implementation registers the same observer multiple times.

This leads to unnecessary growth of internal tracking structures, including:

  • observers
  • observerSlots
  • sources
  • sourceSlots

This issue is especially noticeable in patterns such as:

  • Tight loops:
createEffect(() => {
  for (let i = 0; i < 1000; i++) a();
});
  • Iteration over store arrays:
createEffect(() => {
  for (const item of arrayStore) {
    _ = item;
  }
});

In these cases, repeated signal reads (e.g. length, indexed access, or direct signal calls)
cause the same computation to be registered multiple times.


This PR introduces a lightweight deduplication mechanism by tracking the last registered observer per signal.

  • Adds lastObserver to SignalState
  • Only registers the current computation if it differs from lastObserver
  • Resets lastObserver during cleanup

This significantly reduces redundant registrations in common scenarios while keeping the cost of deduplication O(1) and preserving existing behavior.


How did I test this change?

To enable verification of internal observer behavior in tests, this PR slightly adjusts the return value of createSignal:

const result = [readSignal.bind(s), setter] as Signal<T | undefined>;
if (IS_DEV) (result as any).state = s;
return result;
  1. Added a test case that repeatedly reads a signal inside a loop:
const a = createSignal(0);
const dispose = createRoot(dispose => {
  createEffect(() => {
    for (let i = 0; i < 1000; i++) {
      a[0]();
    }
  });
  return dispose;
});
expect((a as any).state.observers.length).toBe(1);
expect((a as any).state.observerSlots.length).toBe(1);
expect((a as any).state.observers[0].sources.length).toBe(1);
expect((a as any).state.observers[0].sourceSlots.length).toBe(1);
dispose();

Before this change:

  • observers.length === 1000

After this change:

  • observers.length === 1

before_change

  1. Verified similar behavior with store array iteration:
const [arrayStore] = createStore([1, 2, 3, 4, 5]);

createEffect(() => {
  for (const item of arrayStore) {
    _ = item;
  }
});

Confirmed that repeated reads (e.g. length and indexed access) no longer
cause excessive observer registrations.

  1. Ran full test suite:
pnpm build
pnpm test

All tests pass.

  1. Manually inspected that:
  • observerSlots and sourceSlots no longer grow excessively
  • No regression in reactivity behavior

after_change

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 2, 2026

⚠️ No Changeset found

Latest commit: 0bb4dad

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

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.

1 participant