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

Skip ignore comment checks for files with no ignore comments #730

Conversation

marvinhagemeister
Copy link
Contributor

@marvinhagemeister marvinhagemeister commented Nov 26, 2022

Previously we'd cast every rule to a string and then check if an ignore comment is present. That constant casting and repeatedly calling the regex costs a lot of time. We can avoid that work if we know that the input source doesn't contain any ignore comments at all.

This speeds up postcss compilation time in one of my projects by about 4.6s. In that project there are no css files with postcss comments anywhere.

Before:

postcss

Previously we'd cast every rule to a string and then check if an ignore
comment is present. That constant casting and repeatedly calling the
regex costs a lot of time. We can avoid that work if we know that the
input source doesn't contain any ignore comments at all.
@marvinhagemeister marvinhagemeister marked this pull request as ready for review November 26, 2022 18:41
@romainmenke
Copy link
Member

romainmenke commented Nov 26, 2022

@marvinhagemeister Thank you this!
I wasn't aware that this feature was causing performance issues.


@Antonio-Laguna Maybe we should reconsider removing this feature?
We've discussed this in the past but we decided to keep it because it wasn't causing any harm and it wasn't adding that much complexity.

Given that we are doing breaking changes for postcss-preset-env v8 anyway I would prefer removing it entirely. That would make this plugin significantly faster even when users have one or more comments.

I've also looked over this feature and I don't see a way to make it not a performance issue.
We could "shield" it behind a plugin option but I am not sure how many users would opt-in to this.

Thoughts?

@marvinhagemeister
Copy link
Contributor Author

Dropping support for ignore comments all together works for me 👍

@Arborescent
Copy link

Another optimization is reusing saving the RegExp instance for later use.
Most of the cost of this code is compiling the regular expression while initializing the instance. Once it's compiled you can reuse it for a lower cost.

@elijahrdorman
Copy link

elijahrdorman commented Nov 30, 2022

Another optimization is reusing saving the RegExp instance for later use. Most of the cost of this code is compiling the regular expression while initializing the instance. Once it's compiled you can reuse it for a lower cost.

Because regex literals don't change during runtime (like RegExp() might), JITs can and do choose to cache them if heuristics indicate they should (I may be wrong, but I believe this is mostly a factor of how many times it was used). This wouldn't be hard to try out, but may not do much.

I'd certainly rather see it behind a plugin for the few people who want it and are okay with the performance hit.

@romainmenke
Copy link
Member

see : #737

The check for the comment is impossible to make really fast.
But we can make sure we do it as few times as possible.

@romainmenke
Copy link
Member

romainmenke commented Nov 30, 2022

@marvinhagemeister Thank you again for starting this.

We have merged #737 as a fix for the underlying issue and added you as a co-author because you did all the hard work to pinpoint the issue!

We will ship this as soon as possible

@marvinhagemeister marvinhagemeister deleted the custom-properties-perf branch December 1, 2022 09:28
@marvinhagemeister
Copy link
Contributor Author

@romainmenke Awesome sounds good to me! Thanks again for taking a look 👍

@romainmenke
Copy link
Member

@marvinhagemeister This was just released.
Curious what your benchmark results will be with the latest changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants