search

milomg / js-reactivity-benchmark

106 stars 18 forks source link

Add Gc benchmark and Debugging #8

Open robbiespeed opened 1 year ago

robbiespeed commented 1 year ago

Thank you for the benchmark!

This PR adds some important GC related tests, collectOutOfScopeConsumers is most important since it can catch the common issue of out of scope computed's causing memory leaks.

The timings of theses test results aren't really important, but more so if the assertions fail. I first tried making them regular tests instead of benchmarks, but vitest doesn't seem to have any way of accepting the --allow-natives-syntax v8 argument which means there's no way to trigger GC.

Open to other ideas of how these tests could be better run, one option is calling vitest from node, but I'm not sure if that would work. I use mocha for testing GC stuff in my lib since it allows passing v8 arguments.

It also adds some quality of life tweaks like adding debug path, and allowing esm only libraries by switching tsconfig moduleResolution to bundler.

robbiespeed commented 1 year ago

Hmm I may need to rethink the debugging flow. I switched to using the direct build from esbuild, but it seems it does not produce correct source maps (bad debug experience). I was previously using tsx (maps correctly to source), but that doesn't seem to work with enabling --allow-natives-syntax out of the box I'll try harder though.

robbiespeed commented 1 year ago

esbuildkit/esm loader had no issues with --allow-natives-syntax so switched to that and removed the option to build to dist. I suspect it's doing the correct thing and not even trying to transpile anything in node_modules, seems like a bug in tsx that it does.

robbiespeed commented 1 year ago

Hmm I just realized there is an issue with collectDynamicSources and collectDynamicSourcesAfterUpdate. They aren't doing the right check since all the actual reactivity objects are wrapped it's only checking if the wrapper gets collected. The underlying source object that's part of the reactive graph isn't being checked, and can't be checked with the current design of the framework wrapper.