Update deps, add additional unit tests, add signals-polyfill and signia, update results #13
Conversation
|
Looks like we have some iterating to do on the performance of the signal polyfill. |
…om with random package
|
Added Svelte v5 as well
|
|
Wow these new results are really exciting! |
|
Quick update: I'm aiming to test and merge this weekend |
|
I don't think this should merge until the callcount assertions are uncommented out, and cellx bench is re-enabled. Was there a reason dynamic bench was reduced from 5 to 1 runs? If some FWs are failing 5 runs of some dynamic tests then that could be hinting at a memory leak in those FWs. If there's a certain benchmark that is commonly failing for some frameworks then maybe a case can be added to the configs like |
Great feedback & catches. I had disabled certain things while debugging different frameworks and not everything got put back in the toy bin :) Will update shortly. |
|
My current changes in addition to the commented out checks:
|
I think the reason I changed this is because some of these frameworks take 10+ seconds for 1 run on the dynamic tests, so it's a minute per test per framework which is pretty annoying to wait for. It's not nearly as bad for the better optimized frameworks, but TC39 signals polyfill and angular signals are a pain to benchmark, making the tweak => iteration cycle difficult. Maybe in a follow-up PR, we can address how long it takes to run a benchmark end-to-end? I changed it back to 5 runs for now.
Done.
Great catches; done. See |
|
After making these changes, here's the latest output from We may want to consider automating the creation of the benchmark results graph as a follow-up to this PR. I've been manually uploading the CSV into ChatGPT with the following prompt: |
src/dynamicBench.ts
Outdated
| @@ -10,7 +10,7 @@ import { fastestTest } from "./util/benchRepeat"; | |||
| */ | |||
| export async function dynamicBench( | |||
| frameworkTest: FrameworkInfo, | |||
| testRepeats = 1 | |||
| testRepeats = 5 | |||
There was a problem hiding this comment.
changing it back to the original value
|
Yeah, for now I was just replacing the CSV file here with an updated one, but it should be possible to create a chartjs/d3 script to autogenerate results |
| @@ -22,7 +22,7 @@ export const solidFramework: ReactiveFramework = { | |||
| read: () => memo(), | |||
| }; | |||
| }, | |||
| effect: (fn) => createEffect(fn), | |||
| effect: (fn) => createRenderEffect(fn), | |||
There was a problem hiding this comment.
same reason as above; the new unit tests weren't passing without it.
|
|
||
| for (let i = 0; i < iterations; i++) { | ||
| sum = readLeaves.reduce((total, leaf) => leaf.read() + total, 0); | ||
| } else { |
There was a problem hiding this comment.
This allows for an algorithm that defers all changes until after a batch is finished to only walk the graph once instead of iterations times (especially because we have no verification on what leaf.read returns.
Would it be possible to have all frameworks use the same path? (and in a future change I'll add assertions to ensure that every framework's leaf.read is actually getting the right result)
| @@ -16,7 +16,10 @@ export const reactivelyFramework: ReactiveFramework = { | |||
| read: () => r.get(), | |||
| }; | |||
| }, | |||
| effect: (fn) => new Reactive(fn, true), | |||
| effect: (fn) => { | |||
| fn(); | |||
There was a problem hiding this comment.
Why is this needed? Eagerly evaluating effects should probably be done by calling the required function in the framework (stabilize, although that should be called by batch anyways) instead of calling the effect function twice (which gives reactively and oby a disadvantage if the effect is slow because they can't memoize it)
There was a problem hiding this comment.
the new unit tests that I added to ensure we're comparing apples to apples across the different frameworks only work with this added.
happy to change it, but I feel like it's important for the unit tests across all frameworks to work the same way.
There was a problem hiding this comment.
I agree with having tests to ensure that the benchmark is fair, although I think the unit tests may be more strict than we expect. Afaik we don't have any benchmarks that read or write during the build stage (they always return functions that are run outside of the build stage).
Then, making Reactively's withBuild run a stabilize tick after construction should get the effects to run as intended without needing every effect to run twice
There was a problem hiding this comment.
This seems to pass tests for me
export const reactivelyFramework: ReactiveFramework = {
name: "@reactively",
signal: (initialValue) => {
const r = new Reactive(initialValue);
return {
write: (v) => r.set(v),
read: () => r.get(),
};
},
computed: (fn) => {
const r = new Reactive(fn);
return {
read: () => r.get(),
};
},
effect: (fn) => {
return new Reactive(fn, true);
},
withBatch: (fn) => {
fn();
stabilize();
},
withBuild: (fn) => {
const out = fn();
stabilize();
return out;
},
};
|
I've been trying to incorporate features from this PR into the main branch. Is there anything else that's missing for you (if so I'd love to add it), or can I close this PR? |
This PR rebases on top of #12 by @NullVoxPopuli. It updates all deps and contains the changes I needed to get everything working.
compostatesince when I updated to latest it no longer worked)mobxand@vue/reactivityto use shallow signals (to make sure we're comparing apples to apples)For some reason several frameworks are hanging or throwing errors on the
cellxbenchmark, so I've disabled it for now. The same goes for the largest dynamic benchmark, which I've commented out for now.