Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Async Runner with Message Channel #458
Async Runner with Message Channel #458
Changes from 31 commits
b594115
f154a4c
9459c00
d462764
d037f89
79f8450
14f1f6d
d08606d
4540937
a02d7cc
5281af4
d4fa570
764b61b
ace8c3b
147c38f
92f9024
7cca4e0
2b92c1f
acc7522
c778807
211ba6a
81b815e
37ffb46
63910a3
6b6f8d4
110c94a
a8a4059
05ce8bf
930472c
4b69e1d
16dec03
9a66132
daf60ab
ac0b4e0
69350a9
064e1cc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
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.
Do we really need a separate lookup table? Now that timer-based measurement is gone,
why don't we just have a single lookup table with "raf" and "async" as measurement methods.
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.
cleaned up!
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.
This seems to have a side effect of always pushing the measurement end until when a promise resolves.
I don't think we want to do that for rAF based measurement method (since that can affect the score)
so maybe we should check the return value of this function and await conditionally?
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.
More concretely, the following code logs: 1, 2, 3:
whereas the following code logs 1, 3, 2:
i.e.
await
will always delay the execution of the continuation until the end of the current micro-task. While that's highly desirable for async measurement, I don't think we want to affect rAF measurement.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 made it a bit simpler and just passing along a type that we can check to see if we need to await or not
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.
Instead of changing the test runner based on the suite type for some suites, can we initially add a toggle to developer menu so that we can try out async runner?
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.
To my understanding the agreement in the meeting was to unconditionally enable the new runner for the preact workloads and add the global developer-mode menu in a separate PR.
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.
correct - below is the list on enabled suites (for now):
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.
If we're doing that, we need before/after numbers across browsers.
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'll run crossbench for the ones on the list and post results here.
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.
Here are metrics for the next.js workload:

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.
So Safari's total time goes up while the total time for Chrome & Firefox both go down? With that kind of magnitude of a difference, we need a much more thorough investigation before we can approve of the change. I suggest we land this method initially as an optional developer-menu only feature instead.
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'll add an option for the developer menu 👍