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

Performance degradation in v11 when using "mixinsFiles" #167

Open
RubenStoesser opened this issue Sep 25, 2024 · 17 comments · Fixed by #168
Open

Performance degradation in v11 when using "mixinsFiles" #167

RubenStoesser opened this issue Sep 25, 2024 · 17 comments · Fixed by #168

Comments

@RubenStoesser
Copy link

RubenStoesser commented Sep 25, 2024

Hi,

we are using PostCSS with this plugin and vite and have noticed a pretty significant performance degradation starting with v11 when using the mixinsFiles option.

As soon as either multiple files are referenced or a glob is used responses from vite dev server are stuck in pending state for a long time when first starting the dev server.

Examples:

    "postcss-mixins": {
      "silent": true,
      "mixinsFiles": ["src/Global.mixins.css"]
    },

using 11.0.1
Screenshot 2024-09-25 at 11 40 23

using 10.0.1
Screenshot 2024-09-25 at 11 55 37

As long as only one file (without any glob patterns) is referenced everything seems to work as before. However, when referencing multiple files this changes:

    "postcss-mixins": {
      "silent": true,
      "mixinsFiles": ["src/Shared.mixins.css", "src/Global.mixins.css"]
    },

using 11.0.1
Screenshot 2024-09-25 at 11 52 16

using 10.0.1
Screenshot 2024-09-25 at 11 56 46

As shown in the screenshots the first load of localhost takes nearly 23 seconds when using v11.0.1 in comparison to 56ms in v10.0.1. Nothing else changed between the two runs. It also doesn't matter whether the referenced files exist or not.

The only real difference I can see in the two versions is the change from fast-glob to tinyglobby so it seems like this has made some kind of impact.

Any help on how to fix this issue would be much appreciated.

@RubenStoesser RubenStoesser changed the title Performance degradation in v11 when using "mixinFiles" Performance degradation in v11 when using "mixinsFiles" Sep 25, 2024
@ai
Copy link
Member

ai commented Sep 25, 2024

сс @ziebam

@SuperchupuDev
Copy link
Contributor

this particular pattern seems to avoid all of tinyglobby's perf optimizations, performance should greatly improve once something like SuperchupuDev/tinyglobby#23 gets implemented, will take a look at it

@SuperchupuDev
Copy link
Contributor

@RubenStoesser i think i've managed to solve the perf issue in a simpler way, can you add the following line to your package.json and check if it works after reinstalling dependencies? i'll try to release a new tinyglobby version with the fix if it does

"resolutions": {
  "tinyglobby": "https://pkg.pr.new/tinyglobby@f819a2a"
},

@RubenStoesser
Copy link
Author

Hey @SuperchupuDev, thank you for the fast response!

Unfortunately, the change does not seem to have fixed the issue :/

Just an FYI the same degradation also occurs with a single entry in mixinsFiles using *.

e.g.

"mixinsFiles": ["src/**/*.mixins.css"]

@SuperchupuDev
Copy link
Contributor

interesting, will investigate more. meanwhile can you go to tinyglobby inside your node modules, and add a console.log(properties.root) right before the const api = new fdir(fdirOptions).crawl(properties.root); line? (also make sure you installed the pkg.pr.new resolution correctly)

@SuperchupuDev
Copy link
Contributor

oh, another thing I missed, @RubenStoesser if you are using npm the resolutions field won't be recognized, if that's the case try this instead:

"overrides": {
  "tinyglobby": "https://pkg.pr.new/tinyglobby@f819a2a"
},

@RubenStoesser
Copy link
Author

RubenStoesser commented Sep 26, 2024

interesting, will investigate more. meanwhile can you go to tinyglobby inside your node modules, and add a console.log(properties.root) right before the const api = new fdir(fdirOptions).crawl(properties.root); line? (also make sure you installed the pkg.pr.new resolution correctly)

With this config:

      "mixinsFiles": ["src/Shared.mixins.css", "src/Global.mixins.css"]

it logs /<path-to-repo>/src around 170 times before loading in the browser is first completed (count related to amount of css files?).

With this:

"mixinsFiles": ["src/**/*.mixins.css"]

it logs /<path-to-repo> without the /src and the time between each log is longer than in the other case.

I also added a console.time in the postcss-mixins loadGlobalMixin function:

  console.time("glob time");

  let files = globSync(globs, {
    caseSensitiveMatch: false,
    expandDirectories: false
  })

  console.timeEnd("glob time");

"mixinsFiles": ["src/**/*.mixins.css"] -> each call on average glob time: 449.083ms
"mixinsFiles": ["src/Shared.mixins.css", "src/Global.mixins.css"] -> each call on average glob time: 11.179ms

As a comparison using 10.0.1 (version with fast-glob):

"mixinsFiles": ["src/**/*.mixins.css"] -> each call on average glob time: 9.179ms

And with 11.0.1 but WITHOUT your optimisation:

"mixinsFiles": ["src/Shared.mixins.css", "src/Global.mixins.css"] -> each call on average glob time: 415.981ms

So it looks like the optimisation has definitely improved performance for cases explicitly referencing single files but not when using the wildcard * pattern

@SuperchupuDev
Copy link
Contributor

SuperchupuDev commented Sep 26, 2024

it logs /<path-to-repo>/src around 170 times before loading in the browser is first completed (count related to amount of css files?).

that doesn't sound right, it should only log once. unless this library calls the glob function more than once which i don't think is necessary and even less 170 times

So it looks like the optimisation has definitely improved performance for cases explicitly referencing single files but not when using the wildcard * pattern

i don't think this optimization can be expanded to ** patterns, as path/** can match path even if it's a file

@SuperchupuDev
Copy link
Contributor

i've opened #168 to speed up globbing in this library and potentially solve the performance issue you're seeing, i couldn't reproduce the glob function getting called more than once though which sounds like this library's issue and not tinyglobby's

@ai ai closed this as completed in #168 Sep 26, 2024
@ai
Copy link
Member

ai commented Sep 26, 2024

@RubenStoesser try postcss-mixins 11.0.2 with that fix.

@RubenStoesser
Copy link
Author

Hi @ai, 11.0.2 does seem to improve performance noticeably.

Unfortunately, completely ignoring **/node_modules/** is not a feasible solution for us, as we are actually consuming postcss mixin files from libraries in node_modules.

@ai ai reopened this Sep 27, 2024
@ai
Copy link
Member

ai commented Sep 27, 2024

Agree. It will be cool if @SuperchupuDev will find a better improvement.

We can also use native glob for Node.js 22+ if it will fix the issue.

@SuperchupuDev
Copy link
Contributor

SuperchupuDev commented Sep 27, 2024

i've pushed another small improvement that can speed up more cases, you can test it by having this in your package.json:

"resolutions": {
  "tinyglobby": "https://pkg.pr.new/tinyglobby@0fcb1b3"
},

(use overrides instead of resolutions if using npm)

this optimizes usage of ** patterns but only when they're not followed by /. this means you could use src/**.mixins.css as a workaround and it should apply the optimization

@SuperchupuDev
Copy link
Contributor

hi @RubenStoesser, the performance issues you reported around the two patterns in the initial comment should have been fixed in today's tinyglobby release, 0.2.7. i can't make tinyglobby apply those optimizations to "src/**/*.mixins.css" but you can use "src/**.mixins.css" instead which does the same and is able to get optimized. can you confirm if the perf is better now?

@RubenStoesser
Copy link
Author

Hi @SuperchupuDev,

the src/**.mixins.css pattern does not seem to do exactly the same as src/**/*.mixins.css.

For example a file located at src/utils/theming/Global.mixins.css is not found by the pattern and this plugin complains that the mixins defined in that file are unrecognized causing compilation to fail.

However, even using that pattern I unfortunately still do not see a significant performance boost (in version v11.0.1 but with tinyglobby v0.2.7). The main difference in v11.0.2 seems to be the ignoring of node_modules. This also still applies when * wildcards are not used anywhere in the list of mixinsFiles.

@SuperchupuDev
Copy link
Contributor

SuperchupuDev commented Sep 30, 2024

the src/**.mixins.css pattern does not seem to do exactly the same as src/**/*.mixins.css.

For example a file located at src/utils/theming/Global.mixins.css is not found by the pattern and this plugin complains that the mixins defined in that file are unrecognized causing compilation to fail.

right, sorry about that. i think i've finally figured it out and just managed to optimize ** patterns as long as they're followed by something else, including a / (so your original src/**/*.mixins.css should get optimized). @RubenStoesser can you test it using tinyglobby 0.2.8 (that i've just released)?

However, even using that pattern I unfortunately still do not see a significant performance boost (in version v11.0.1 but with tinyglobby v0.2.7). The main difference in v11.0.2 seems to be the ignoring of node_modules. This also still applies when * wildcards are not used anywhere in the list of mixinsFiles.

didn't you say here #167 (comment) that performance did increase for patterns without *? did it change? tinyglobby 0.2.7 and up should improve performance for both

also, try version 11.0.3 of this package which reverted node_modules being ignored

@RubenStoesser
Copy link
Author

didn't you say here #167 (comment) that performance did increase for patterns without *? did it change? tinyglobby 0.2.7 and up should improve performance for both

Sorry, should have clarified here. The performance is slow when referencing multiple specific files and one of those files is located in the node_modules folder.

e.g. the same performance test as used in previous comments with one file in node_modules:

"mixinsFiles": ["node_modules/some-pkg/dist/PostCSS.css"]

glob: 0.115ms

and multiple files:

"mixinsFiles": ["node_modules/some-pkg/dist/PostCSS.css", "src/Global.mixins.css"]

glob: 415.154ms

This was now using v11.0.3 of this package and v0.2.8 of tinyglobby.

Interestingly, referencing a single glob pattern is now definitely faster with v0.2.8:

"mixinsFiles": ["src/**/*.mixins.css"]

glob: 10.46ms

However, adding a second entry to the mixinsFiles array referencing a file in node_modules slows performance down again significantly.

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 a pull request may close this issue.

3 participants