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

Improve @tailwindcss/postcss performance for initial builds #14565

Merged
merged 19 commits into from
Oct 3, 2024

Conversation

RobinMalfait
Copy link
Member

This PR improves the performance of the @tailwindcss/postcss plugin. Before this change we created 2 compiler instances instead of a single one. On a project where a tailwindcss.config.ts file is used, this means that the timings look like this:

[@tailwindcss/postcss] Setup compiler: 137.525ms
⋮
[@tailwindcss/postcss] Setup compiler: 43.95ms

This means that with this small change, we can easily shave of ~50ms for initial PostCSS builds.

@RobinMalfait RobinMalfait force-pushed the feat/improve-postcss-performance branch from ca1390d to 931f927 Compare October 1, 2024 14:34
CHANGELOG.md Outdated Show resolved Hide resolved
@adamwathan adamwathan force-pushed the feat/improve-postcss-performance branch from 5b74a25 to b6eb194 Compare October 1, 2024 15:58
@RobinMalfait
Copy link
Member Author

Pushed a few more changes to make CI pass. Some things I noticed and that the new commits solve:

  1. The cache inside of the PostCSS plugin seems to be re-created from scratch, which means that we essentially have no cache. I hoisted it to the module-level to keep the cache around.
  2. Noticed that we call onDependency with the same file twice, so made sure it's only in the list once. (This happened because resolving module dependencies of a file x, also included the file x)

So these new changes need a second pair of eyes before we can merge this.

/cc @adamwathan @philipp-spiess

I am not 100% sure, but I believe this gets rid of at least 1 macroTask.
Otherwise it means that we:
1. Setup 2 compilers for an initial build
2. The second time we clear require caches which means that _everything_
   will be loaded from scratch.
@RobinMalfait RobinMalfait force-pushed the feat/improve-postcss-performance branch from aea94a6 to 67b2f51 Compare October 1, 2024 16:04
During debugging I noticed that some of the files were in the list
multiple times, using a Set solves that issues. While this is a little
band-aid it helps for now. Following commits will include a more proper
fix.
During debugging, I noticed that every time the PostCSS code runs, the
full plugin re-runs. This means that caches are thrown away. I'm
currently not sure if this is a test specific issue or an actual issue.

That said, the integration tests do run postcss-cli for example directly
so I think this is a real issue.

This change should be safe since the cache is unique per input file
(which it was before as well). Worst case is that we use a bit more
memory.
Removing this means that the main file (e.g.: `tailwind.config.js`) is
only in the list once. This also means that we only have to register it
as a PostCSS dependency and it also means that we will `fs.stat` the
file once.
@RobinMalfait RobinMalfait force-pushed the feat/improve-postcss-performance branch from 67b2f51 to 0c1a873 Compare October 1, 2024 16:47
Comment on lines 39 to 47
let cache = new DefaultMap(() => {
return {
mtimes: new Map<string, number>(),
compiler: null as null | Awaited<ReturnType<typeof compile>>,
css: '',
optimizedCss: '',
fullRebuildPaths: [] as string[],
}
})
Copy link
Member

Choose a reason for hiding this comment

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

The only thing I can think of here is that previously this was scoped for the opts argument to the tailwindcss function and now it's not, so now the cache is shared for the same key even if the options are different.

Interestingly the PostCSS docs explicitly say to set up caches in the place we were setting it up before:

image

Do we know with absolute certainty it was getting re-created from scratch on every save? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

If I apply this diff to our plugin:

diff --git a/packages/@tailwindcss-postcss/src/index.ts b/packages/@tailwindcss-postcss/src/index.ts
index 7d18ddc36..43b824d2f 100644
--- a/packages/@tailwindcss-postcss/src/index.ts
+++ b/packages/@tailwindcss-postcss/src/index.ts
@@ -50,6 +50,8 @@ function tailwindcss(opts: PluginOptions = {}): AcceptedPlugin {
   let base = opts.base ?? process.cwd()
   let optimize = opts.optimize ?? process.env.NODE_ENV === 'production'
 
+  console.count('Setup caches here')
+
   return {
     postcssPlugin: '@tailwindcss/postcss',
     plugins: [
@@ -65,6 +67,7 @@ function tailwindcss(opts: PluginOptions = {}): AcceptedPlugin {
           let inputFile = result.opts.from ?? ''
           let context = cache.get(inputFile)
           let inputBasePath = path.dirname(path.resolve(inputFile))
+          console.count('Run @tailwindcss/postcss')
 
           async function createCompiler() {
             env.DEBUG && console.time('[@tailwindcss/postcss] Setup compiler')

If I then use it, the output looks like this:
image

You have the initial build, then 2 builds because of watch mode and because I made 2 changes.

Some notes / observations:

  1. Not only is the Setup caches here log called every time we save, it's called twice for an unknown reason.
  2. This is using the postcss-cli package

Next steps are to verify if this behaviour also happens in other environments where the PostCSS plugin is being used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can confirm that this behaviour also exists in a vite project with the PostCSS plugin:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Some updates for completeness:

If you structure your PostCSS config like this:

module.exports = {
  plugins: {
    '@tailwindcss/postcss': {},
  },
}

or this:

module.exports = {
  plugins: [
    require('@tailwindcss/postcss')
  ],
}

Then the above behavior can be observer. If you call your function (even without options) then you see the correct behavior:

module.exports = {
  plugins: [
    require('@tailwindcss/postcss')()
  ],
}

@ZuBB
Copy link

ZuBB commented Oct 2, 2024

I might be running ahead of the train, but can this change (theoretically) be ported to v3? If so, what do maintainers think about it?

Every standalone run (read CI/CD cases) should be faster with it. IMO it deserves to be present in the latest stable branch

@adamwathan
Copy link
Member

@ZuBB This isn't really relevant in v3 — completely different codebase and all of the caching we do there is already at the module scope level, so this is just an improvement over how v4 was working prior to this.

@ZuBB
Copy link

ZuBB commented Oct 2, 2024

@ZuBB This isn't really relevant in v3 — completely different codebase and all of the caching we do there is already at the module scope level, so this is just an improvement over how v4 was working prior to this.

okay, I get it. thanks for the response

thecrypticace and others added 3 commits October 2, 2024 13:56
There’ve been some improvements in compatibility and perf in a few of them
@philipp-spiess philipp-spiess force-pushed the feat/improve-postcss-performance branch from 0b3f8e7 to 15bf21b Compare October 3, 2024 12:44
detectSources: { base },
sources: context.compiler.globs,
})
}
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 we need to recreate the scanner when doing a full rebuild too, right? Since the globs might've changed?

Copy link
Contributor

Choose a reason for hiding this comment

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

basically after line 166 we also want to re-create the scanner

CHANGELOG.md Outdated Show resolved Hide resolved
@RobinMalfait RobinMalfait enabled auto-merge (squash) October 3, 2024 14:21
@RobinMalfait RobinMalfait disabled auto-merge October 3, 2024 14:21
@RobinMalfait RobinMalfait merged commit 35b84cc into next Oct 3, 2024
1 check passed
@RobinMalfait RobinMalfait deleted the feat/improve-postcss-performance branch October 3, 2024 14:21
RobinMalfait pushed a commit that referenced this pull request Oct 3, 2024
We accidentally removed this in #14565.
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 this pull request may close these issues.

5 participants