Fix require time performance #2454
Comments
|
@chris-morgan Can you contrast this with other programs of similar complexity? Is there anything actionable that can be done? |
|
@davidtheclark Well, here are a bunch of other things I was trying with postcss: postcss: 109 And I consider postcss-clean and autoprefixer to be far too slow (over 200ms) as well. Seriously, 100ms is a long time. I don’t know why stylelint is so slow at present, but it is very slow and must be doing something that it shouldn’t be! |
|
@chris-morgan Can you compare it to a module of similar size? I imagine stylelint involves many, many more files than any of those modules you've listed. |
|
Putting a bit of instrumentation into |
|
@davidtheclark I compared with eslint using the method above:
eslint is pretty big. Maybe we have slow dependencies, or there are other places to improve the speed. |
|
Rolling the ~275 files of stylelint into one with rollup.js produced a saving of about one millisecond per file, halving the loading time spent in stylelint. That then leaves stylelint’s dependencies unrolled; I estimate another 150ms could be saved there if they were to roll themselves up. (Fun fact: cosmiconfig adds over 100ms of loading time, just by itself; its dependency js-yaml is around 80ms of that. A few others are almost as bad.) A 40–50% reduction in load time purely from rolling everything up is not to be sneezed at. After doing this, stylelint would still be far too slow to start for my liking, but it’s a start, at the least. |
|
@chris-morgan how did you measure dependencies loading time? Like you did with cosmicconfig. I would like to learn :) |
|
@hudochenkov A very hacked-together script which produces a kind-of-back-to-front tree. I think I’ll make a nicer version of this which does a bit of analysis, it’ll do nicely as my first package to go on npm. var orig = require.extensions['.js'];
var level = 0;
var cwd = process.cwd();
var toPrint = [];
require.extensions['.js'] = function (module, path) {
if (path.startsWith(cwd)) {
path = '.' + path.substr(cwd.length);
}
var start = Date.now();
level++;
var out = orig.apply(this, arguments);
level--;
toPrint.push('│ '.repeat(level) + '┌ ' + path + ' \x1b[32m' + (Date.now() - start) + '\x1b[m');
if (!level) {
console.log(toPrint.join('\n'));
toPrint.splice(0);
}
return out;
}
require('stylelint'); // Or ./stylelint-bundle.js after generating itThen I just worked it over a bit in Vim with regular expressions, |
|
|
@hudochenkov What tool do you use to output the time? 😄 |
|
@evilebottnawi console.time('stylelint');
const stylelint = require('./stylelint');
console.timeEnd('stylelint'); |
|
@hudochenkov Thanks) I just thought that this could be some kind of other tool, on the hooks |
|
@hudochenkov also we can try using lazy require for some modules, but it is need investigation |
hudochenkov
added
the
status: needs investigation
|
Ok, so what's actionable here? The good news is We could try rolling-up the code as part of the publishing process. What I'd like to know is will this complicate using stylelint as a pinned-github-tag dependencies. One of the reasons we target
Regarding Does any performance-orientated person want to pick this up? |
jeddy3
changed the title
require('stylelint') is very slow
jeddy3
added
status: ready to implement
|
@jeddy3 caniuse-lite is much smaller on disk, though it remains to be seen if it will improve performance. I hope that it does, but of course there is a small overhead to unpacking the data when the module is run. The module is optimised for size not performance (indeed, it's about 7 times smaller).
At the very least I hope we can start a conversation about providing alternative ways to consume caniuse data through subsets like this one. |
|
After reading https://pouchdb.com/2016/01/13/pouchdb-5.2.0-a-better-build-system-with-rollup.html and https://medium.com/@Rich_Harris/how-to-not-break-the-internet-with-this-one-weird-trick-e3e2d57fee28, I'm thinking that the only way to solve this would be to create a bundle with Rollup, and that might be just fine, or at least worth an experiment. Because we wouldn't be transpiling any code (except imports/exports), just concatenating, the built file would still be legible and debuggable (just long).
I don't know of a way to ease this — but I also don't know if it's worth worrying about too much. So, when somebody has the time and wants to take a shot at this problem, I suggest looking codemodding to ES2015 module syntax and using Rollup. |
|
I experimented with this the other day, we should be able to use rollup-plugin-commonjs to avoid having to change any existing stylelint code. This is a rollup config that worked for me on "use strict"
const commonjs = require("rollup-plugin-commonjs")
const json = require("rollup-plugin-json")
module.exports = {
entry: "lib/index.js",
dest: "dist/index.js",
moduleName: "stylelint",
format: "cjs",
plugins: [
commonjs(),
json(),
],
}Here are the before and after results of requiring the bundled and unbundled stylelint 5 times each: node -e "s=Date.now();require('./lib/index.js');console.log(Date.now()-s)"
800
749
741
762
731
===
mean 756node -e "s=Date.now();require('./dist/index.js');console.log(Date.now()-s)"
613
673
611
620
665
===
mean 636However I was unable to succesfully rollup all stylelint's dependencies into one file (using rollup-plugin-node-resolve, the resulting bundle would error when run. I assume that would bring further improvements. |
That's awesome.
100ms improvement isn't too shabby. I think we should roll with it.
I think we can definitely proceed with rolling up our files for now, and then look to this as an additionally improvement. |
|
I experience this issue as well, I'm using styelint with gulp and it's very slow. You can see my time from time-require here As you can see, Stylelint is top of the list here. |
|
@Bigdragon13th maybe your don't ignore some files (tests, examples, build directory and etc stuff)? |
|
@evilebottnawi It's not runtime that is slow, it's the loading time stylelint use when we do |
|
Has anyone successfully bundled Stylelint using rollup? I created a stylelint-bundler repo reporting my attempts, work-arounds and failure at making a stand-alone version of Stylelint. Maybe someone with more experience with rollup can help? Paging @bfred-it 😉. Also, the |
|
@Mottie I gave up on rollup for complex tasks. Try webpack or backpack https://github.com/jaredpalmer/backpack/ |
|
@Mottie Big thanks for making a start on attempting to bundle stylelint using rollup, and for documenting your progress in your repo. It looks to be a tricky one!
I don't believe any of the core team have had time to look into this, and so any progress you make would be greatly appreciated. Good performance is one of the goals of stylelint. I understand that prettier uses rollup for their build. Perhaps you'll find some inspiration there to overcome the blockers you've encountered.
Agreed. Feel free to contribute that optimisation. You can use the built-in benchmarking tools to gauge the performance impact on the rules that use this data. |
I think without bundling time is the same. Last time I checked it was around ~140ms (but some fixes for loading time wasn't merged yet) |
That's correct. The gains of bundling are only apparent when the syntaxes are bundled separately. This makes sense as the bundling replaces lazyloading.
I should have made that clearer in my original post. |
|
What do you think about marking rules as externals? Why we need to bundle them eagerly? |
For the browser (#3935), e.g. tools like JS Fiddle. |
|
Lodash might not go away, because other dependencies have it: Instead we'll have much more dependencies. One for every lodash function. |
Interesting. I wonder what It looks like:
I've been meaning to remove the dependency on I think it's worth looking into further as lodash is a significant chunk of the bundle weight at over half a meg and it impacts require time.
I should have also been clearer that reducing the require time from 120 to 80 is only half the story. It looks like a bundled stylelint (with the separate syntaxes) is twice as fast to run, which would benefit everyone but especially extension authors and people in the Makefile world, like the original poster. |
|
I tried it locally and we can cherry-pick the lodash function we use. We'd still have a single lodash dependency, but we'd cherry-pick functions from it (as table and babel already do). For example: // package.json
{
"dependencies": {
"lodash": "^4.17.15",
}
}// stringFormatter.js
const _escapeRegExp = require('lodash/escapeRegExp');
const _flatMap = require('lodash/flatMap');
const _sortBy = require('lodash/sortBy');
const _uniqBy = require('lodash/uniqBy');It knocks off a chunk of weight from the browser bundle. |
|
Can we use https://github.com/lodash/babel-plugin-lodash to automate this for bundle? |
The plugin only supports ES Modules syntax. I used the plugin to rewrite the source files (having first replaced all the lodash requires with imports) and then I converted them back commonjs when I did my local test. |
|
There is an ESLint rule that we can use to ensure method imports from lodash. See eslint-plugin-lodash/import-scope. I have written a codemod we can use to make this change across the stylelint repos. See m-allanson/codemod-lodash-requires. I'm planning to work on this once #4796 is finished. |
|
This is a useful tool for analysing require times: require-so-slow. Here's the output from the latest version of stylelint, running on my machine (requires Chrome): devtools timeline viewer (stylelint.trace)
Create your own trace with |
* Remove `postcss-reporter` package This change removes the `postcss-reporter` package and reduces the dependencies. As you see the changed `package-lock.json`, this package depends on: - `chalk` - `lodash` - `log-symbols` - `postcss` See also #2454 (comment) We've used only the tiny utility function `getLocation()` in the package. This function just returns an object with the `line` and `column` property in our project, so I think we can replace it with a simple code. https://github.com/postcss/postcss-reporter/blob/f01a601ea2cd41d626e561969d66a765b3afcb2d/lib/util.js#L4-L13 * Fix test name: `stringFormatter` -> `verboseFormatter`
|
ESBuild can bundle the I suspect we'll want to bundle:
Stylelint should remain installable from GitHub. We'll need to investigate how this is done with np, perhaps using npm lifecycle scripts and the We'll want to weigh up the performance gains against any additional maintenance burden. Investigating bundling and opening a pull request will give us the opportunity to assess that. You can give ESBuild a go with: ./node_modules/.bin/esbuild ./lib/index.js --bundle --outfile=out/index.js --format=cjs --platform=node --minify --metafile=lib.jsonWhich will result in a 814.3kb bundle with consistently sub-100ms require times (down from the 200ms for If you like, you can upload the generated metafile to https://www.bundle-buddy.com to see a breakdown. Thanks to all the hard refactoring work done (including the removal of lodash) the breakdown looks clean. |
jeddy3
changed the title
jeddy3
changed the title
jeddy3
changed the title
jeddy3
added
status: ready to implement
jeddy3
changed the title
jeddy3
added
type: bug
Loading stylelint (
require('stylelint')) is very slow. It takes over a second for me when there’s nothing else installed, and when there are various other things installed it increases (e.g. two seconds).I care about this because in a Makefile world where you spin up the tooling for each file, this is a >1 second delay per file which rapidly changes from unpleasant to awful.
Steps to reproduce:
n/a
n/a
n/a
7.9.0
n/a
n/a
require('stylelint')to be fast (like, well under 100ms).It took a long time to import stylelint, a second or two.
The text was updated successfully, but these errors were encountered: