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

Add Dart flute WasmGC workload #33

Merged
merged 4 commits into from
Feb 26, 2025
Merged

Conversation

danleh
Copy link
Contributor

@danleh danleh commented Jan 22, 2025

See README for more details.
One iteration takes about 500-600ms on d8, hence 15 iterations to make it take not too long. Seems to be the first workload that uses ES6 modules (dart2wasm generates one, see build/flute.dart2wasm.mjs and benchmark.js)

CC @mkustermann

@danleh
Copy link
Contributor Author

danleh commented Jan 23, 2025

Recording offline discussion with @mkustermann, Dart2wasm expert: They expect to land more improvements to the Dart toolchain in the coming weeks/months. So before a major release with this line item included by default, we should make sure to re-build and profile. I also measured locally with d8 that we spend ~10-15% in the GC, which Martin confirmed to be in the ballpark what the native Dart VM does as well.

@eqrion
Copy link

eqrion commented Jan 24, 2025

@danleh @mkustermann Can you confirm if this issue has been fixed in this benchmark? We ran into it when running this benchmark last year.

See README for more details.
One iteration takes about 500-600ms on d8, hence 15 iterations to make it take not too long.
Seems to be the first workload that uses ES6 modules (dart2wasm generates one, see `build/flute.dart2wasm.mjs` and `benchmark.js`)
@danleh danleh force-pushed the dart-flute-wasm-squash branch from 621c87d to a88fb7c Compare January 27, 2025 15:59
@mkustermann
Copy link

@danleh @mkustermann Can you confirm if dart-lang/flute#23 has been fixed in this benchmark? We ran into it when running this benchmark last year.

Correct, that was missing. I've just rolled new flute (as well as js shells, binaryen, ...) & recompiled the benchmarks in https://github.com/mkustermann/wasm_gc_benchmarks

@danleh
Copy link
Contributor Author

danleh commented Jan 28, 2025

Thanks Martin! I just updated, so should be fixed now here as well @eqrion. I also briefly double checked the workload: profile/flamegraph looks pretty similar to before, so fine from my side to merge.

@danleh
Copy link
Contributor Author

danleh commented Feb 11, 2025

@kmiller68 Updated to main, passes CI. I also double checked that it runs with a local up-to-date JSC build. Ready to merge from my side.

Copy link
Contributor

@kmiller68 kmiller68 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM too.

Comment on lines +5 to +7
// Excerpt from `build/run_wasm.js` to add own task queue implementation, since
// `setTimeout` and `queueMicrotask` are not always available in shells.
function addTaskQueue(self) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't be opposed to adding a queueMicrotask to JSC shell. jsc/v8 already have setTimout (although the delay time is ignored for jsc not sure about v8). AFAIK, all the shell runners have a microtask queue anyway. I assume those queues are mostly equivalent to the browser one too. Is this something V8 would be willing to do? @eqrion for thoughts on adding these to sm?

All that said, I'm also happy to leave this as no browser seems to show any samples in this code. If we want to change this let's do this as a follow up.

@kmiller68 kmiller68 merged commit 8f56a4e into WebKit:main Feb 26, 2025
3 checks passed
@danleh danleh deleted the dart-flute-wasm-squash branch February 27, 2025 16:54
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.

4 participants