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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Nov 1, 2024
Merged

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a453b00
fix(core): measure perf or async checks
WilcoFiers Oct 11, 2024
d769ba8
Move the rule defer to after the perf log
WilcoFiers Oct 28, 2024
f587dcc
Merge branch 'develop' into async-perf-timer
WilcoFiers Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 15 additions & 9 deletions lib/core/base/rule.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,15 +284,21 @@ Rule.prototype.run = function run(context, options = {}, resolve, reject) {
});
});

// Defer the rule's execution to prevent "unresponsive script" warnings.
// See https://github.com/dequelabs/axe-core/pull/1172 for discussion and details.
q.defer(res => setTimeout(res, 0));

if (options.performanceTimer) {
this._logRulePerformance();
}

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.

if (options.performanceTimer) {
this._logRulePerformance();
}
// Defer the rule's execution to prevent "unresponsive script" warnings.
// See https://github.com/dequelabs/axe-core/pull/1172 for discussion and details.
setTimeout(() => {
resolve(ruleResult);
}, 0);
}).catch(error => {
if (options.performanceTimer) {
this._logRulePerformance();
}
reject(error);
});
};

/**
Expand Down
Loading