Closed aminya closed 4 years ago
And you've completely missed the point...
We need either nuclide, atom-ide-ui, or linter & friends. We don't need all of them. That's also the motivation for wrapping the installer like this, since it only installs atom-ide-ui (also, side note, probably a good idea to bump the version on that dependency)
I suspect the difference you're seeing primarily comes from available packages vs loaded packages. The looping code generally shouldn't be particularly slow, but filesystem access in getAvailablePackages
probably is.
Also, how are you measuring startup time? Because I don't see what you do (Atom 1.46.0, current master of atom-typescrpt)
FWIW,
atom.packages.getAvailablePackageNames().length === 191
on my system
Nevermind the question about testing, I've overlooked the webhooks part.
@lierdakil I cannot reproduce those results anymore (Atom timecope always acts funky), but now when I do the following it is 13.5ms faster:
let t1 = window.performance.now()
// code....
console.log(window.performance.now()-t1)
Before
vs
After
Huh. Well, apparently I was wrong about the looping thing, it is indeed rather inefficient. Good to know.
With performance.now() I'm seeing at most 2ms difference though, but okay, it's still a large relative difference.
Anyway, could you make the code a little less noisy by using something like
const packagesProvidingUIServices = ["atom-ide-ui", "linter", "nuclide"]
if (packagesProvidingUIServices.some(p => atom.packages.isPackageLoaded(p)))
? This shouldn't affect performance by much, and I would argue is more concise if nothing else.
Also, for the sake of consistency, could you leave the atom-package-deps
loading code as require
there, or change other requires
to await import
? I would prefer the require
variant as synchronous, but I don't really mind either option.
Probably you mean
const packagesProvidingUIServices = ["atom-ide-ui", "linter", "nuclide"]
if (!packagesProvidingUIServices.some(p => atom.packages.isPackageLoaded(p))) {
I tested it, and window.performance gives:
So not much difference.
About import
, let it be as it is. TypeScript will convert it based on the target you choose. This will also allow using bundlers like rollup in the future to further speedup loading.
I want to also fix some of the redundant dynamic requires in the activation function.
;(require("etch") as typeof import("etch")).setScheduler(atom.views)
These do not improve the performance when used in the activation function, and it is better to just write them regularly using import
.
If you want to lazy load them, you should have a condition or command that loads them later, not just used them in the activation function. This is one of the best methods to improve performance.
See this for example in package-deps 6. We don't import ./promt
and ./install
until actually we have checked the deps:
I am going to make a separate PR for that.
About import, let it be as it is. TypeScript will convert it based on the target you choose. This will also allow using bundlers like rollup in the future to further speedup loading.
I doubt bundlers are relevant for Atom pacakges, all things considered. And I know how await import
works in TypeScript over node, basically it's await Promise.resolve().then(() => require(x))
, which is a lot of fluff and defers execution until the next tick. But whatever. My issue here is stylistic inconsistency. Why use await import
in one place and require
literally two lines down? I suggest you either change all of those or none of those.
These do not improve the performance when used in the activation function, and it is better to just write them regularly using import.
Those do improve Atom loading time when used in conjunction with activation hooks. If those are imported, literally everything is loaded on Atom startup, and by then there's no real point in deferred activation.
Actually bundlers speedup things a lot!
Check these for example:
https://github.com/JunoLab/atom-ink/pull/277
https://github.com/steelbrain/package-deps/pull/270
https://github.com/JunoLab/language-weave/pull/44
https://github.com/steelbrain/linter/pull/1695
This is a technique that I use a lot. I can give you more examples if you like.
Those do improve Atom loading time when used in conjunction with activation hooks. If those are imported, literally everything is loaded on Atom startup, and by then there's no real point in deferred activation.
Ah yes. Good to know. I was thinking about the no activation hook situation.
I doubt bundlers are relevant for Atom pacakges, all things considered. And I know how
await import
works in TypeScript over node, basically it'sawait Promise.resolve().then(() => require(x))
, which is a lot of fluff and defers execution until the next tick. But whatever. My issue here is stylistic inconsistency. Why useawait import
in one place andrequire
literally two lines down? I suggest you either change all of those or none of those.
Most of the time, bundlers don't bundle/tree-shake something that is require
ed. This is one of the biggest cons of require
. The awesome thing about TypeScript
and Bundlers is that we can use import
and they bundle/tree-shake or convert them for us. But they don't touch require.
Actually, I've realized we can't have activate
be async, or at least we can't have pluginManager
be created asyncronously, because providers/consumers depend on it, and Atom doesn't await
on package activation. So yeah, that was a bug actually. For the same reason, can't use await import
there.
Anyway, I've corrected that and pushed out a patch release. Thanks for your work!
You're welcome. You can use the old-style import('').then(()=>{})
if you can't use await import
.
Can't if need the result of the import right away.
This improves the loading time by at least 150ms (X8.5 faster)
I disabled the activation hooks to measure the loading time correctly:
After:![image](https://user-images.githubusercontent.com/16418197/83997308-babb2d80-a923-11ea-85ca-ccdfc366f4c3.png)
Before:![image](https://user-images.githubusercontent.com/16418197/83997245-92cbca00-a923-11ea-9fa6-3afbcf98d5ec.png)
why this improves the loading time so much:
getAvailablePackages
is very slow. I have submitted a PR https://github.com/atom/atom/pull/20899, but even with that it does not makes sense to get the list of all packages just for checking 3 among them.cc: @lierdakil