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 no-descending-specificity performance #7026

Conversation

romainmenke
Copy link
Member

Which issue, if any, is this issue related to?

See #6869

Is there anything in the PR that needs further explanation?

This rule warned multiple times for the same selector.
It would emit one warning for each selector with lower specificity that preceded the current selector.

I don't think this is very useful as it just causes extra noise.

With this change this rule will only emit a single warning for each selector.
It will always reference the earliest other selector with lower specificity.

All these extra warnings came at a cost.
fewer warnings -> faster execution.

I've also added extra tests to cover this scenario.


before :

Warnings: 407860
Mean: 1452.8959879999998 ms
Deviation: 85.89908815057446 ms

after :

Warnings: 7117
Mean: 367.08543172727275 ms
Deviation: 14.07386884681592 ms

The excessive number of warnings is the result of the duplicate CSS in the test source.

Ideally we would have a large, but well formed and realistic benchmark source.

@changeset-bot
Copy link

changeset-bot bot commented Jul 2, 2023

🦋 Changeset detected

Latest commit: b81d660

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
stylelint Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@jeddy3 jeddy3 left a comment

Choose a reason for hiding this comment

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

Nice, this was the slowest rule I encountered while taking a pass at #6869. So, it's great to see this improvement.

(I saw your performance-related pull request upstream in PostCSS too. Excellent stuff.)

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

Great improvement! 👏🏼

Rule#selectors property actually has some calculation. Honestly, I feel it's hard to notice it... 😓
https://github.com/postcss/postcss/blob/8.4.24/lib/rule.js#L13-L15

@ybiquitous
Copy link
Member

Sorry for the conflict in the test file due to #7024. Could you resolve the conflict, please? 🙏🏼

…cificity-performance--considerate-avocet-12abeeb86a
@romainmenke romainmenke merged commit cf73360 into main Jul 3, 2023
18 checks passed
@romainmenke romainmenke deleted the fix-no-descending-specificity-performance--considerate-avocet-12abeeb86a branch July 3, 2023 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants