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

Inline the PostCSS config file into the webpack config #45030

Merged
merged 4 commits into from
Aug 24, 2020

Conversation

jsnajdr
Copy link
Member

@jsnajdr jsnajdr commented Aug 19, 2020

This is a follow up to @alshakero's postcss-custom-properties optimization in #42829.

Here I'm trying to speed up the webpack build by inlining the PostCSS config file into the webpack config. The postcss-loader doesn't need to load the config file 800+ times, for each *.scss compilation, but it's created only once, as an in-memory object, when creating the webpack config.

I needed to add a new option, postCssOptions, to the calypso-build/webpack/sass loader, enhancing the existing postCssConfig option. The existing option only lets us specify a path and context for the config file, but the new options lets up specify the plugins directly, skipping the config-file-loading step completely.

This speeds up mainly the fallback production build, the only one that uses postcss-custom-properties plugin. Testing locally, the timings improved from 167 seconds before to 94 seconds after (yarn run build-client-fallback with production env variables, skipping the linting check).

I'm not sure if these timings improvements are for real -- maybe the cache-loader and other similar things skew the result? It's worth trying to reproduce the results by the reviewer.

How to test:
Verify that the fallback build output still contains fallback definitions for CSS theme colors, and that the autoprefixer did its work, too.

@matticbot
Copy link
Contributor

@matticbot matticbot added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Aug 19, 2020
@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Member

@alshakero alshakero left a comment

Choose a reason for hiding this comment

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

Small change, big win, thank you! Code looks good to me.

This is one of the times I wish we had implemented this.

Comment on lines 227 to 234
plugins: () =>
[
autoprefixerPlugin(),
browserslistEnv === 'defaults' &&
postcssCustomPropertiesPlugin( {
importFrom: [ calypsoColorSchemes ],
} ),
].filter( Boolean ),
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to making plugins a function instead of a plain array? I might be mistaken but PostCSS API seems to accept an array.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really, it can be a plain array just fine 🙂 All the variables that are used to compute the plugin list are global constants. Fixed in f42a554

…el dependencies

They were specified only in `calypso-build/package.json` until now, which is not entirely
correct. The packages are used a lot outside the `calypso-build` context.
Allows for more powerful configuration of the PostCSS pipeline, e.g., inlining
the config completely instead of loading it from file.
…config.js

The config file doesn't need to be loaded 800+ times for every .scss compilation.
It's created only once when loading the webpack config.
@jsnajdr jsnajdr force-pushed the update/postcss-inline-config branch from 8afbb60 to f42a554 Compare August 21, 2020 06:01
@jsnajdr jsnajdr merged commit 7986ad7 into master Aug 24, 2020
@jsnajdr jsnajdr deleted the update/postcss-inline-config branch August 24, 2020 05:37
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 24, 2020
@scinos
Copy link
Contributor

scinos commented Aug 24, 2020

For completeness, I don't get the performance benefits in my system: the fallback build tooks about 2 minutes in all cases:

# Before

Commit: 594a8badc9 with `postbuild-client-fallback` disabled

## Cold caches

Command: rm -fr .cache/ .build/ public/fallback && time NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback
Average: 1:56.29
Raw data:
	NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback  154.81s user 14.33s system 143% cpu 1:58.07 total
	NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback  151.49s user 13.56s system 144% cpu 1:54.51 total
	NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback  151.26s user 13.93s system 142% cpu 1:55.66 total


## Warm caches

Command: time NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback
Average: 2:04.99
Raw data:
	NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback  154.96s user 15.02s system 142% cpu 1:59.66 total
	NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback  173.44s user 18.74s system 143% cpu 2:13.74 total
	NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback  153.43s user 14.80s system 138% cpu 2:01.57 total

---

# After

Commit: 7986ad73d0 with `postbuild-client-fallback` disabled

## Cold caches

Command: rm -fr .cache/ .build/ public/fallback && time NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback
Average: 2:06.74
Raw data:
	NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback  166.93s user 19.75s system 143% cpu 2:09.66 total
	NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback  175.55s user 17.54s system 143% cpu 2:14.49 total
	NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback  152.63s user 14.64s system 144% cpu 1:56.08 total

## Warm caches

Command: time NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback
Average: 1:57.70
Raw data:
	NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback  151.40s user 13.24s system 144% cpu 1:54.24 total
	NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback  151.96s user 12.99s system 144% cpu 1:54.06 total
	NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback  165.02s user 16.86s system 145% cpu 2:04.79 total

@jsnajdr
Copy link
Member Author

jsnajdr commented Aug 24, 2020

@scinos I see that when testing with "cold caches", you use the following command to delete the caches: rm -fr .cache/ .build/ public/fallback

On my machine, I also have a ~/.cache folder in my home directory, with a webpack/css subfolder. The results of the CSS compilations seem to be cached there, and they use the PostCSS pipeline that is being optimized in this PR. The subfolder is regenerated on the next build after I delete it, so it's not something stale. It seems that your "cold caches" build was using that cache, as it was not delete?

Is it expected that the ~/.cache folder is used, and used only by the CSS compilations? It seems that something in the mini-css-extract-plugin configuration was omitted when you recently worked on the cache-loader and the .cache directory.

@scinos
Copy link
Contributor

scinos commented Aug 24, 2020

Let me rerun the tests with deleting ~./cache too, give me a few mins.

@scinos
Copy link
Contributor

scinos commented Aug 24, 2020

Data with a colder cache:

# Before

Commit: 594a8badc9 with `postbuild-client-fallback` disabled

## Colder caches

Command: rm -fr ~/.cache/webpack .cache/ .build/ public/fallback && time NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback
Average: 3:06.13
Raw data:
	NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback  252.58s user 22.27s system 140% cpu 3:15.05 total
	NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback  232.14s user 18.25s system 142% cpu 2:55.69 total
	NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback  238.04s user 22.88s system 139% cpu 3:07.30 total

---

# After

Commit: 7986ad73d0 with `postbuild-client-fallback` disabled

## Colder caches

Command: rm -fr ~/.cache/webpack .cache/ .build/ public/fallback && time NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback
Average: 2:59.43
Raw data:
	NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback  231.28s user 21.88s system 131% cpu 3:12.93 total
	NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback  217.50s user 19.49s system 140% cpu 2:48.96 total
	NODE_ENV=production CALYPSO_ENV=production yarn run build-client-fallback  228.40s user 21.66s system 141% cpu 2:56.40 total

The diff is ~7 seconds. Not sure if it is statistically relevant, given that I got runs with a difference of >20 seconds.

@jsnajdr
Copy link
Member Author

jsnajdr commented Aug 24, 2020

The diff is ~7 seconds. Not sure if it is statistically relevant, given that I got runs with a difference of >20 seconds.

Thanks for testing! The 7 seconds diff is not that great, but still nice if it proves to be real.

The patch removes a code path that loads a small JS file from filesystem and executes it, repeated 800 times. 7 seconds looks like a plausible timing for that.

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