Reduce package activation time #875
Conversation
This saves about 13 ms, depending on your machine.
We don't need the dependencies to be installed to finish activation of the package, so push those off to an idle callback, this way the (currently) ~110ms time it takes to load `atom-package-deps` and run the install() method isn't cut out of our activation time.
We don't need it active immediately, spawn the worker during idle time.
Make the export of isConfigAtHomeRoot a default export and change the usage in main.js to a lazy require so we don't incur a ~30ms penalty for importing it from an external file on activate().
Move to lazily requiring all dependencies of the main file so "activation" time is kept down, as TimeCop includes this in the activation instead of loading time. Note that idsToIgnoredRules was moved into main.js so we aren't forced to require it to observe settings. Although this requires a re-implementation in the specs, the specs already are containing some implementation in there in the first place so I don't think this is a big deal for such a small bit of logic.
|
After rebasing on the fixed specs locally, some issues with the worker not being initialized quick enough have cropped up, looking into fixing this. |
Since idle callbacks are handled in a FIFO queue if the worker has not been initialized yet we can wait on another idle callback to ensure that the idle callback that initializes the worker has been called. Fixes a semi-rare case where a lint call happens before the worker has been initialized.
|
Fixed 😉 |
There was a problem hiding this comment.
I found one problem with fix-on-save, but other than that this looks good. My startup time was reduced to 5ms.
Of course the downside to this is more complicated code. It might be nice if there were a few more comments scattered around explaining to future travelers why these shenanigans are being played, as it might not be immediately obvious to a casual observer new to the codebase.
| path = require('path') | ||
| } | ||
| if (!isConfigAtHomeRoot) { | ||
| isConfigAtHomeRoot = require('./is-config-at-home-root') |
There was a problem hiding this comment.
Fix-on-save is currently broken, with an error message that isConfigAtHomeRoot isn't a function. It seems that you need to use isConfigAtHomeRoot = require('./is-config-at-home-root').default, although to be honest, I'm not entirely clear on the "right" way to interop require statements with export. Too bad we don't have import() yet.
|
Whoops, that's the problem with having all of these branches running off of each other. Already found that and fixed it, but apparently that was in a different branch, and since the fixing of the specs that would expose that is over in #878 I missed it here. Will update in the morning! |
|
@IanVS |
Add a comment describing the reasoning behind lazy loading the dependencies.
Reduce activation time of the package by deferring some actions for a later idle time and lazily requiring imports where possible. On my system this reduces package activation from an average of 177 ms down to 7 ms. The remaining 7 ms is largely taken up with Atom internals, with only 1.3 ms being under our control in any manner.
Fixes #871.