-
Notifications
You must be signed in to change notification settings - Fork 14
Sqlite Wasm build: build script, README, shell polyfill #2
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
Conversation
Regarding test duration/workload size: There is a convenient Regarding noise/variance: Running the benchmark 20 times with the reduced workload size in
1-3% standard deviation sounds fine to me. |
Nice! Looks good so far! One thing that might be worthwhile is to put the harness/driver hooks in their own file. I noticed when I tried to recompile some of the existing tests we couldn't just drop in emcc's new |
It now works in the CLI runner of JetStream and when running standalone via
Good point. The polyfills and runner are fully separated from the generated code. This PR also currently includes/depends on #15. |
@kmiller68 This is ready for your review from my side. It's still using and scored with the "old" |
sqlite3/runner.js
Outdated
// These arguments should match the upstream browser runner `speedtest1.html`. | ||
let argv = [ | ||
"speedtest1", | ||
"--singlethread", |
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.
I guess for webpages they probably only need single threaded? Does it work without single threaded on the web or is that argument de facto required?
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.
I copied this argument from the upstream speedtest1.html
harness, which also uses --singlethread
in the Web version of this benchmark. I don't see any observable difference with or without it, however. I have a minor preferences for leaving it as-is, to keep it consistent with the upstream benchmark harness. WDYT?
Overall looks good to me. Can we fix the one indentation issue first though? |
Do you want to land that one first or do it here. I'm ok with either. |
Done. Also renamed
Let's just land this here, I'll close #15 subsequently. |
Ready from my side (sorry again for the delay.) |
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.
Looks good! Thanks.
Runs in JSC, SpiderMonkey shell, and D8. TODOs:
jshell
.cli.js
and the browser one).Sample output: