Skip to content

fix(core): measure perf for async checks #4609

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

Merged
merged 3 commits into from
Nov 1, 2024
Merged

Conversation

WilcoFiers
Copy link
Contributor

Performance of async checks isn't measured correctly. No tests, since we don't generally test perf timer stuff.

@WilcoFiers WilcoFiers requested a review from a team as a code owner October 11, 2024 12:46
}

q.then(() => resolve(ruleResult)).catch(error => reject(error));
q.then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be preferable to put this before L289, no? If there's no other event-loop-blocking work pending then it won't make a difference, but if that setTimeout ends up being what causes other event-loop-blocking work to have a chance to run, I don't think it'd give us a better picture of rule performance to log independently of that other work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. But your solve doesn't work. If I leave it in the function where it is now then it's called synchronous. If I put it in the defer with the setTimeout then its called as soon as the async check starts to await. Either way it doesn't test the time the async check took to complete. The only way to be sure the check finished is to put the time end in the resolve. Even that's a little problematic because while we're awaiting check A, axe will run other checks, so the complete time of A will include the complete time of other checks too.

So I'm a little conflicted about how to best do this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Per discussion in standup, we decided we're fine with doing a version that will only be accurate in a { runOnly: 'single-rule' } scenario. It'd be nice to leave a comment explaining the limitation before merging, though.

@dylanb dylanb changed the title fix(core): measure perf or async checks fix(core): measure perf for async checks Oct 11, 2024
@WilcoFiers WilcoFiers merged commit 7e9bacf into develop Nov 1, 2024
22 checks passed
@WilcoFiers WilcoFiers deleted the async-perf-timer branch November 1, 2024 10:38
straker pushed a commit that referenced this pull request Mar 4, 2025
Performance of async checks isn't measured correctly. No tests, since we
don't generally test perf timer stuff.
@straker straker mentioned this pull request Mar 4, 2025
WilcoFiers added a commit that referenced this pull request Mar 5, 2025
### Bug Fixes

- **aria-allowed-role:** Add math to allowed roles for img element
([#4658](#4658))
([f6dddd9](f6dddd9)),
closes [#4657](#4657)
- **captions:** fix grammar in captions check incomplete message
([#4661](#4661))
([3ef7058](3ef7058))
- consistently parse tabindex, following HTML 5 spec
([#4637](#4637))
([3b0a361](3b0a361)),
closes [#4632](#4632)
- **core:** measure perf for async checks
([#4609](#4609))
([e7dc26e](e7dc26e))
- **locale:** fixed typos in german (DE) locale
([#4631](#4631))
([0740980](0740980))
- **locale:** proofread and updated de.json
([#4643](#4643))
([910cdb2](910cdb2))
- **no-autoplay-audio:** don't timeout for preload=none media elements
([#4684](#4684))
([b7f1ad1](b7f1ad1))
- **target-size:** do not treat focusable tabpanels as targets
([#4702](#4702))
([67d4e4f](67d4e4f)),
closes [#4421](#4421)
[#4701](#4701)
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.

2 participants