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.

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

Experiment with building a proposal API on top of alien-signals #44

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

johnsoncodehk
Copy link

@johnsoncodehk johnsoncodehk commented Jan 8, 2025

This PR is research-based, and we are re-constructing surface APIs based on alien-signals to obtain performance improvements, now faster than most frameworks.

We intentionally relies on the alien-signals package rather than duplicating code in order to easily discover code specific to the signal proposal.

Regarding the differences in test results.

  • Prohibited contexts - allows writes during computed: The alien-signals algorithm is able to handle computed side effects, so the expected results in the test are now modified to the correct values.

  • type checks - checks types in methods: I'm not sure what I should do to make the current implementation pass these tests, but since this PR is for research purposes only, I don't think this test is worth solving, so I just skipped it.

Chart:

image

(Please note that since this PR is just to explore speed improvements, we will not try to align with all the details in the proposal. If you want to continue exploring this approach please feel free to fork this branch, thanks. 🙏)

@trueadm
Copy link

trueadm commented Jan 8, 2025

@NullVoxPopuli Did you add prohibited-contexts.test? Does this test need to be updated or is there a genuine issue here do you think?

@NullVoxPopuli
Copy link
Contributor

I did not add it -- it was implemented with the original spec -- but I did pull it out of a massive test file, as we need to be describing why tests exist, and what they're testing, and why that behavior is important.

It is def a behavior question about whether or not folks want to allow synchronous mutation while reading another value.

Personally, I don't think this is a good idea, as it prior-reads of the mutated state are now out of date, and if the consumer is entangled with the mutated state, that usually leads to infinite looping when a renderer is involved

@johnsoncodehk
Copy link
Author

We have already solved the problem of infinite loops at the algorithmic level. If synchronous mutations in computed is not handled, it can not pass the Vue core test suite.

@NullVoxPopuli
Copy link
Contributor

Sounds fine since it's solved.
Not sure if proposal text will need to update

@jkrems
Copy link

jkrems commented Jan 9, 2025

Can you link to the source of the benchmark? I'd be curious to see what the test setup was like. Was it based on https://github.com/transitive-bullshit/js-reactivity-benchmark?

johnsoncodehk added a commit to johnsoncodehk/js-reactivity-benchmark that referenced this pull request Jan 12, 2025
@johnsoncodehk
Copy link
Author

johnsoncodehk commented Jan 12, 2025

@jkrems yes, I just add a test for this PR to a new branch.

https://github.com/johnsoncodehk/js-reactivity-benchmark/tree/alien-polyfill

@jkrems
Copy link

jkrems commented Jan 13, 2025

@johnsoncodehk I was asking because that benchmark has some known measuring artifacts for the signal polyfill specifically (see https://x.com/synalx/status/1868235387812053167). So it might be less predictive for this particular PR. That doesn't mean that this PR isn't a performance improvement. But it might just require additional validation before it's clear how it compares.

@alxhub
Copy link

alxhub commented Jan 13, 2025

Hi @johnsoncodehk, this looks cool! It's awesome to see the TC39 proposal implemented on a different signals core.

I was looking through the code and I have a question about the semantics of alien-signals around cleanup of the graph.

One of the design goals for the TC39 proposal (and for Angular signals which I worked on) that Computeds can be dropped / garbage collected. That is, this code:

const width = new Signal(10);
const height = new Signal(15);

function calculateArea(): number {
  // For whatever reason, create a temporary `Computed` for the area. Perhaps we're doing a complex
  // calculation and want to memoize intermediate parts.
  const area = new Computed(() => width.get() * height.get());

  // Note that `area` itself doesn't escape this function, we only return the result.
  // The Computed is dropped and left to the garbage collector.
  return area.get();  
}

// Does repeatedly calling `calculateArea()` "leak" memory by adding many subscribers to
// `width` and `height`?
console.log(calculateArea());
console.log(calculateArea());
console.log(calculateArea());

Currently this is not the case in the polyfill, as we only track a dependency -> subscriber edge for signals which are "watched". Unwatched signals like area don't create such edges, and thus can be cleaned up by GC. In order to implement Computed without such forward edges, dependencies are polled on reads to check if their values have changed, which is a transitive operation.

How does alien-signals handle the garbage collection of Computed nodes?

This is where the benchmarking issue arises. Some of the js-framework-benchmarks create large graphs of unwatched Computed nodes only, updating them and measuring the time to recompute results. This is indeed slow because of the reliance on this polling. Real applications, however, will largely be using watched signals, which propagate via the fast path. This difference can be significant: with Angular signals, I've measured performance under real-world (watched) conditions as being 25x faster than what the benchmarks show.

@tomByrer
Copy link

Real applications, however, will largely be using watched signals, which propagate via the fast path.

Interesting insights @alxhub! Do you have performance test suggestions that better aligns with 'real applications' please?

@johnsoncodehk
Copy link
Author

I implemented a pull model-based createReactiveSystem API based on preact’s approach in stackblitz/alien-signals#41, which can solve the GC problem mentioned by @alxhub.

The performance improvement is still significant, but due to the overhead of the proposed's surface API, it is still far from alien-signals.

I've update this branch to https://github.com/johnsoncodehk/js-reactivity-benchmark/tree/alien-polyfill.

@johnsoncodehk johnsoncodehk marked this pull request as ready for review January 24, 2025 05:55
@johnsoncodehk
Copy link
Author

The GC problem is now solved by a cooling mechanism. Computed will enter cooling in the next microtask every time it loses all subscribers (no longer referenced by dependencies), and warm up (re-referenced by dependencies to receive updates) when the getter is called next time .

If computed is triggered every time a microtask is triggered, cooling/warming up may occur frequently, so we may need to implement more reliable scheduling for cooling.

The latest benchmark result has been updated to #44 (comment).

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.

6 participants