Skip to content

[wasm] benchmark sample modularized - #62395

Merged
pavelsavara merged 3 commits into
dotnet:mainfrom
pavelsavara:wasm_benchmark_sample
Dec 6, 2021
Merged

[wasm] benchmark sample modularized#62395
pavelsavara merged 3 commits into
dotnet:mainfrom
pavelsavara:wasm_benchmark_sample

Conversation

@pavelsavara

@pavelsavarapavelsavara commented Dec 4, 2021•
edited
Loading

Copy link
Copy Markdown
Member
  • Silence the errors just before we abort the iframe, because there all fetch promises in flight would be aborted inside the iframe and the errors would be printed
  • Make benchmark more readable, separate the application in the iframe
  • bind console.log and console.error as Kate suggested
  • use Module.printErr to report all errors

Fixes#62357

@ghost

ghost commented Dec 4, 2021

Copy link
Copy Markdown

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

Issue Details

Make it more readable

Author:pavelsavara
Assignees:-
Labels:

area-Infrastructure-mono

Milestone:-

@pavelsavara
pavelsavara marked this pull request as ready for review December 4, 2021 17:00
@pavelsavarapavelsavara mentioned this pull request Dec 4, 2021
Closed
kg
kg previously requested changes Dec 4, 2021
View reviewed changes

@kgkg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good overall but there are parts of this I'm not familiar with

Comment threadsrc/mono/wasm/runtime/exports.ts Outdated
@ghostghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Dec 4, 2021
@radicalradical added the arch-wasm WebAssembly architecture label Dec 5, 2021
@ghost

ghost commented Dec 5, 2021

Copy link
Copy Markdown

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details
  • Silence the errors just before we abort the iframe, because there all fetch promises in flight would be aborted inside the iframe and the errors would be printed
  • Make benchmark more readable, separate the application in the iframe
  • bind console.log and console.error as Kate suggested
  • use Module.printErr to report all errors

Fixes #62357

Author:pavelsavara
Assignees:-
Labels:

arch-wasm, area-Infrastructure-mono, needs-author-action

Milestone:-

@ghostghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Dec 5, 2021
@pavelsavara
pavelsavara requested a review from kgDecember 5, 2021 12:25
radekdoulik
radekdoulik reviewed Dec 6, 2021
View reviewed changes
Comment threadsrc/mono/sample/wasm/browser-bench/frame-main.js Outdated
radekdoulik
radekdoulik approved these changes Dec 6, 2021
View reviewed changes

@radekdoulikradekdoulik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Besides the done event it LGTM.

@pavelsavara@radekdoulik
no need for "done"
2b6e9ce
Co-authored-by: Radek Doulik <radek.doulik@gmail.com>
@pavelsavara
pavelsavara dismissed kg’s stale reviewDecember 6, 2021 12:05

I fixed the comment and we discussed it on Discord

@pavelsavara
pavelsavara merged commit 1872ef5 into dotnet:mainDec 6, 2021
@pavelsavara
pavelsavara deleted the wasm_benchmark_sample branch January 4, 2022 13:06
@ghostghost locked as resolved and limited conversation to collaborators Feb 3, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@radekdoulikradekdoulikradekdoulik approved these changes
@marek-safarmarek-safarAwaiting requested review from marek-safar
@kgkgAwaiting requested review from kg

Assignees

No one assigned

Labels

arch-wasmWebAssembly architecturearea-Infrastructure-mono

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[wasm] browser-bench appstart is broken on main

4 participants

@pavelsavara@kg@radekdoulik@radical