Fix performance of constructor parsing rules #6869
Comments
|
If you gotta have a shortlist of rules that you wanna review pick in priority the ones included in stylelint-config-recommended. |
ybiquitous
added
status: wip
|
@romainmenke Thank you so much for your effort! 👏🏼 |
|
Edit: Todo list moved to the top post. edit: added config-recommended sub list |
|
🤔 selector parsing seems to be particularly slow. With a modified benchmark source that includes :
these three are only chosen because they are readily available and large |
|
Benchmarking for CSS code including more modern syntax sounds like a good idea. The current CSS (Bootstrap 4.4) might be a bit outdated. 🤔 stylelint/scripts/benchmark-rule.mjs Line 25 in 1c44f18 |
Seems to be caused by the There are two things affecting performance here : scenario A : Seemingly simply checks are done against scenari B : The selectors need to be parsed but parsing isn't done with the selector list as a whole. Just passing everything to the selector parser would be much faster because of multiple reasons : Multiple short/small loops are a lot slower than one longer loop:
By first splitting with Refactoring rules to always use Other note : Including TailwindCSS in my temporary benchmark skewed the results too much. TailwindCSS isn't representative of typical CSS because it's ratio of selectors vs. properties is off and it's selectors have too much escaping. Optimizing specifically for TailwindCSS isn't that interesting in my opinion. |
|
Consideration about |
jeddy3
changed the title
jeddy3
added
type: bug
|
This is a great initiative. It reminds me of this series of articles that look into improving JavaScript tool performance. I think it makes sense to limit this issue to:
I've:
|
|
Since performance is a never ending endeavour I think we should state the minimal outcome for this issue to be closed. |
That's correct. It was implied, but I should've made it explicit for clarity.
We can close once all the tasks in the list are done, i.e. each rule is checked and any necessary changes made. The list is in priority order, but people are free to choose which rule(s) they want to look at. As an aside, I've unassigned @romainmenke from the issue. We don't typically assign other people to issues because we're an open-source project where everyone can pick up or ignore any issue they want. However, people are welcome to assign themselves if they find it useful for their workflow. I'll rustle up a quick PR to add this to the managing issues part of our maintainer guide, where there's guidance on other things like how we use labels. |
|
This is starting to have noticeable impact :) A commit from before this effort was started : On the branch for Benchmark script : /* eslint-disable no-console */
import Benchmark from 'benchmark';
import picocolors from 'picocolors';
import postcss from 'postcss';
import stylelint from '../lib/index.js';
const { bold, yellow } = picocolors;
const CSS_URL = 'https://cdn.jsdelivr.net/npm/bootstrap@5.3.0/dist/css/bootstrap.css';
const processor = postcss().use(stylelint({ config: { "extends": "../stylelint-config-standard" } }));
fetch(CSS_URL)
.then((response) => response.text())
.then(async (response) => {
let css = response;
console.profile(`profile__config-standard`);
await processor.process(css, { from: undefined }).then((result) => {
result.messages
.filter((m) => m.stylelintType === 'invalidOption')
.forEach((m) => {
console.log(bold(yellow(`>> ${m.text}`)));
});
console.log(`${bold('Warnings')}: ${result.warnings().length}`);
}).catch((err) => {
console.error(err.stack);
});
console.profileEnd(`profile__config-standard`);
let lazyResult;
const bench = new Benchmark('rule test', {
defer: true,
setup: () => {
lazyResult = processor.process(css, { from: undefined });
},
onCycle: () => {
lazyResult = processor.process(css, { from: undefined });
},
fn: (deferred) => {
lazyResult.finally(() => {
deferred.resolve();
});
},
});
bench.on('complete', () => {
console.log(`${bold('Mean')}: ${bench.stats.mean * 1000} ms`);
console.log(`${bold('Deviation')}: ${bench.stats.deviation * 1000} ms`);
});
bench.run();
})
.catch((error) => console.error('error:', error));
/* eslint-enable no-console */ |
|
Context :
|
|
Nice to see your upstream changes in PostCSS making a dent in those timings! |
Recommended config:
Standard config:
Selector rules:
Other rules:
List created with :
find ./lib/rules -type f -print0 | xargs -0 grep -l "parse"Some rules may not have any issues. Maybe they just contain the word "parse" in a comment.
In stylelint/css-parser#2 performance was discussed in the context of CSS parsers.
This made me curious about the current state of Stylelint.
Is it fast, slow, ... ?
This issue doesn't say anything about stylelint/css-parser#2.
The other issue was merely inspiration for this one.
Especially for Stylelint performance is an interesting subject because by it's very nature Stylelint runs should gravitate towards becoming a
noop.In a codebase with many linter errors it's logical to do a lot of work to compute what the issues are and where they are.
However in a codebase that is already "healthy", Stylelint should be able to skip a lot of work.
Optimizing for codebases that are already free of issues means that users will have a fast experience most of the time.
Even in a codebase that is full of errors, there is a lot parsing and processing done that can be eliminated. Most parts of a CSS codebase do not apply to most Stylelint rules.
A low effort and low complexity way to make Stylelint faster is to add one or two types of fast aborts :
Example of 1 :
color-function-notationwhere the CSS source doesn't containrgb|rgba|hsl|hslaExample of 2 :
color-function-notationwith optionmodernwhere the CSS source doesn't contain any comma's.These checks will typically be regexp's that are to broad to know if the issue is real and where it is, but are specific enough to know that there can not be an issue.
i.e. they can have false negatives, but not false positives.
I've already explored some rules for low hanging fruit and I think reviewing all (non deprecated) rules and making improvements to them will be good for users of Stylelint.
alpha-value-notationperformance with improved benchmark script #6864at-rule-property-required-listperformance #6865color-*performance #6868The text was updated successfully, but these errors were encountered: