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

fix(perf): update end mark for rules #1303

Merged
merged 2 commits into from
Jan 18, 2019

Conversation

paulirish
Copy link
Contributor

@paulirish paulirish commented Jan 4, 2019

With performanceTimer on, we get some nice usertiming measures added. But in #701, some jerk picked an endpoint for rules that I, personally, think can be improved. :)

This moves the end mark of each rule to when the checks have synchronously finished. This change means we don't include the asynchronous bit afterwards, but that async bit is the subject of #1172.

For accurate timing, it makes more sense to keep these measures synchronous. It also makes reading the flame chart more logical.

Screenshots of before and after:
image

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Has documentation updated, a DU ticket, or requires no documentation change
  • Includes new tests, or was unnecessary
  • Code is reviewed for security by: @WilcoFiers

@paulirish paulirish requested a review from a team as a code owner January 4, 2019 01:36
@paulirish
Copy link
Contributor Author

And FWIW this PR still makes sense, because of

q.defer(resolve => setTimeout(resolve, 0));

This setTimeout pushes the end mark to after the end of the big axe block of execution. Basically making it look like all these Rules take 1-10 seconds longer than they really do.

@stephenmathieson
Copy link
Member

What's the status of this PR? Is there anything that needs to happen before this gets reviewed?

/cc @dequelabs/axe-team

@WilcoFiers WilcoFiers merged commit a28674e into dequelabs:develop Jan 18, 2019
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.

3 participants