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

Move Perf Dashboard suite out of tentative directory. #244

Merged
merged 1 commit into from
Jun 28, 2023

Conversation

rniwa
Copy link
Member

@rniwa rniwa commented Jun 17, 2023

No description provided.

@camillobruni
Copy link
Contributor

Generally looking good.

I reran this locally and noticed that the first run in a fresh tab (in all browsers), the first data point in Perf-Dashboard/Render/Async is quite a big outlier.

First-run in tab (Firefox):

Screenshot 2023-06-20 at 14 41 23

Second-run in tab (Firefox):
Screenshot 2023-06-20 at 14 41 51

In Chrome's DevTools, this is the main diff:
Screenshot 2023-06-20 at 14 42 30

@camillobruni
Copy link
Contributor

Firefox first step in first run in tab:

Screenshot 2023-06-20 at 14 48 25

Firefox first step in second run in tab:
Screenshot 2023-06-20 at 14 48 52

So I wonder whether we somehow have to put the canvas initialisation in the prepare step?

@camillobruni
Copy link
Contributor

At least in Chrome it's a one-time-per-tab overhead for initialising font caches.
I can't imeditaley figure out whether that's the same case in other browsers.

Screenshot 2023-06-20 at 14 56 02

@flashdesignory
Copy link
Contributor

super nit: can we remove the console log here?

console.log('_mouseDown', event.offsetX, event.offsetY, event.screenX, event.screenY, event.target, event.target.getBoundingClientRect());

it's printing out when running the test

@bgrins
Copy link
Contributor

bgrins commented Jun 20, 2023

We're still looking into this and planning to continue throughout the week. One initial finding is that we are seeing significant time spent building iterators in generator functions. We don't currently optimize these very much because we haven't seen much evidence that they are performance critical in the wild.

It's not clear to me if this is an incorrect assumption and they are actually used widely / in performance critical ways (and thus we should be driving resources into optimizing this) or if the particular usage within this workload is somewhat unrepresentative of real world content (and thus the workload should be changed).

@camillobruni @rniwa do you have thoughts on this point (and/or data about usage of this feature)?

@rniwa
Copy link
Member Author

rniwa commented Jun 23, 2023

We're still looking into this and planning to continue throughout the week. One initial finding is that we are seeing significant time spent building iterators in generator functions. We don't currently optimize these very much because we haven't seen much evidence that they are performance critical in the wild.

It's not clear to me if this is an incorrect assumption and they are actually used widely / in performance critical ways (and thus we should be driving resources into optimizing this) or if the particular usage within this workload is somewhat unrepresentative of real world content (and thus the workload should be changed).

@camillobruni @rniwa do you have thoughts on this point (and/or data about usage of this feature)?

Oh, interesting. I'm open to replacing that code but there is a bit of chicken & egg question here in that if we don't include anything that's already performance critical then we'd be sample-biasing to only test features that are already fast because authors would avoid using slow APIs in performance critical ways.

@rniwa
Copy link
Member Author

rniwa commented Jun 23, 2023

At least in Chrome it's a one-time-per-tab overhead for initialising font caches. I can't imeditaley figure out whether that's the same case in other browsers.

Screenshot 2023-06-20 at 14 56 02

Addressing this problem in #255.

@rniwa
Copy link
Member Author

rniwa commented Jun 23, 2023

We're still looking into this and planning to continue throughout the week. One initial finding is that we are seeing significant time spent building iterators in generator functions. We don't currently optimize these very much because we haven't seen much evidence that they are performance critical in the wild.
It's not clear to me if this is an incorrect assumption and they are actually used widely / in performance critical ways (and thus we should be driving resources into optimizing this) or if the particular usage within this workload is somewhat unrepresentative of real world content (and thus the workload should be changed).
@camillobruni @rniwa do you have thoughts on this point (and/or data about usage of this feature)?

Oh, interesting. I'm open to replacing that code but there is a bit of chicken & egg question here in that if we don't include anything that's already performance critical then we'd be sample-biasing to only test features that are already fast because authors would avoid using slow APIs in performance critical ways.

I've put up a PR to avoid using generators in #256 but this doesn't seem to affect Firefox's score much so not sure if it's worth going that route.

@flashdesignory
Copy link
Contributor

@rniwa - noticed the lint check fails. Just skimming through it, looks pretty straight forward.

@rniwa
Copy link
Member Author

rniwa commented Jun 27, 2023

@rniwa - noticed the lint check fails. Just skimming through it, looks pretty straight forward.

I'm leaving those styling issues as is for now since they're happening inside the code being ported from perf.webkit.org. It's akin to having wrong formatting within a third party library/framework.

@flashdesignory
Copy link
Contributor

@rniwa - noticed the lint check fails. Just skimming through it, looks pretty straight forward.

I'm leaving those styling issues as is for now since they're happening inside the code being ported from perf.webkit.org. It's akin to having wrong formatting within a third party library/framework.

if that's the case, do you want to add it to the ignore files, so the check passes?

@rniwa
Copy link
Member Author

rniwa commented Jun 27, 2023

@rniwa - noticed the lint check fails. Just skimming through it, looks pretty straight forward.

I'm leaving those styling issues as is for now since they're happening inside the code being ported from perf.webkit.org. It's akin to having wrong formatting within a third party library/framework.

if that's the case, do you want to add it to the ignore files, so the check passes?

Good point. How do I do that?

@flashdesignory
Copy link
Contributor

in the root are two files .eslintignore and .prettierignore.
You should be able to add the path there

Copy link
Contributor

@bgrins bgrins left a comment

Choose a reason for hiding this comment

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

As discussed in the meeting today, r+ on this contingent on #256 (due to lack of data on generator usage)

Copy link
Contributor

@flashdesignory flashdesignory left a comment

Choose a reason for hiding this comment

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

Approved - assuming the linters will ignore these files and conflicts will be resolved.

@rniwa rniwa force-pushed the make-perf-dashboard-non-tentative branch 2 times, most recently from cf84e44 to 4ca1156 Compare June 28, 2023 19:32
@rniwa rniwa force-pushed the make-perf-dashboard-non-tentative branch from 4ca1156 to 0722253 Compare June 28, 2023 19:34
@rniwa rniwa merged commit 1c4d9c7 into main Jun 28, 2023
4 checks passed
@rniwa rniwa deleted the make-perf-dashboard-non-tentative branch June 28, 2023 19:52
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.

5 participants