-
Notifications
You must be signed in to change notification settings - Fork 35
Add Gc benchmark and Debugging #8
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
base: main
Are you sure you want to change the base?
Conversation
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 |
esbuildkit/esm loader had no issues with |
@@ -5,7 +5,7 @@ import { | |||
createMemo, | |||
createRoot, | |||
createSignal, | |||
} from "solid-js/dist/solid.cjs"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, how did you manage to resolve the browser version instead of the node version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating type in package.json to module allows loading libraries as esm. And switching moduleResolution to bundler in tsconfig makes TS work with the change.
pnpm-lock.yaml
Outdated
@@ -5,9 +5,6 @@ settings: | |||
excludeLinksFromLockfile: false | |||
|
|||
patchedDependencies: | |||
'@angular/[email protected]': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, pushed a commit updating this patch (but I would expect this should definitely be required to run the benchmark)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in rebase
@@ -5,11 +5,13 @@ | |||
"main": "index.js", | |||
"scripts": { | |||
"test": "vitest", | |||
"bench:debug": "node --allow-natives-syntax --loader @esbuild-kit/esm-loader src/index.ts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this loader?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes debugging much easier, as you can set breakpoints in the source files of this repo. I tried with esbuild building to dist folder with sourcemaps, which kindof works but the sourcemaps are incorrect leading to breakpoints not matching up correctly to the source, and when walking in the debugger it doesn't show the accurate line which is paused on.
8c3da8a
to
c79bf04
Compare
Hmm I just realized there is an issue with |
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.