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

Slow Activation Time #330

Open
haroldtreen opened this issue Dec 28, 2017 · 25 comments
Open

Slow Activation Time #330

haroldtreen opened this issue Dec 28, 2017 · 25 comments

Comments

@haroldtreen
Copy link
Contributor

@haroldtreen haroldtreen commented Dec 28, 2017

Current Behavior

prettier-atom is showing up as my slowest package, with an activation time nearly 2x as long as the subsequent package.

image

Expected Behavior

I would expect prettier-atom to boot closer to 100 ms? It seems to be an outlier, and I would like to understand why.

Debug Info

Atom version: 1.23.1
prettier-atom version: 0.43.1
prettier version: 1.9.2
prettier-eslint version: 8.6.0
prettier-atom configuration: {
  "useEslint": false,
  "useStylelint": false,
  "useEditorConfig": true,
  "formatOnSaveOptions": {
    "enabled": false,
    "respectEslintignore": true,
    "showInStatusBar": false,
    "javascriptScopes": [
      "source.js",
      "source.jsx",
      "source.js.jsx",
      "source.babel",
      "source.js-semantic",
      "text.html.basic",
      "text.html.vue"
    ],
    "typescriptScopes": [
      "source.ts",
      "source.tsx",
      "source.ts.tsx"
    ],
    "cssScopes": [
      "source.css",
      "source.less",
      "source.css.less",
      "source.scss",
      "source.css.scss",
      "source.css.postcss"
    ],
    "jsonScopes": [
      "source.json"
    ],
    "graphQlScopes": [
      "source.graphql"
    ],
    "markdownScopes": [
      "source.md",
      "source.gfm",
      "text.md"
    ],
    "excludedGlobs": [],
    "whitelistedGlobs": [],
    "isDisabledIfNotInPackageJson": false,
    "isDisabledIfNoConfigFile": false
  },
  "prettierOptions": {
    "singleQuote": false,
    "bracketSpacing": true,
    "semi": true,
    "useTabs": false,
    "jsxBracketSameLine": false,
    "printWidth": 80,
    "tabWidth": "auto",
    "trailingComma": "none",
    "parser": "babylon"
  },
  "prettierEslintOptions": {
    "prettierLast": false
  }
}

OS: MacOS 10.13.2

I also experimented with

atom --profile-startup .

I've attached that profile:

CPU-20171228T150504.cpuprofile.zip

@robwise
Copy link
Collaborator

@robwise robwise commented Dec 31, 2017

We've done what we can to try and reduce startup time by lazy-loading as much as possible. There's going to be some overhead though because we do register a status tile in the lower left that lets you disable/enable format-on-save. If you or anyone else have any ideas on how to further reduce startup time, I'm happy to give it a shot. I also want to try implementing https://github.com/sindresorhus/import-lazy this instead of our own solution.

@haroldtreen
Copy link
Contributor Author

@haroldtreen haroldtreen commented Dec 31, 2017

Given that context I explored around a bit. As a caveat, I've never worked with Atom plugins so please forgive any naive assumptions I may make 😅 .

  1. That lazy import looks really interesting. Could definitely be a way to clean up the existing lazy import code.
  2. I wonder if the status tile could be made even more lazy. It seems currently createStatusTile creates the element but also computes the status and imports all of atomInterface simply to get the toggleFormatOnSave callback. Instead, you could have createLazyStatusTile that injects a disabled tile and adds the status + listener after the package activates.
  3. Speaking of atomInterface, that takes 85ms to load. Actually, only 0.1ms of that is self-time. What's really taking time is babel-runtime/helpers/toConsumableArray.

image

image

Looking at atomInterface there's no imports but there is this fancy ES6 syntax going on.

const getAllScopes = () => [
  ...getJavascriptScopes(),
  ...getTypescriptScopes(),
  ...getCssScopes(),
  ...getJsonScopes(),
  ...getGraphQlScopes(),
  ...getMarkdownScopes(),
];

It looks like babel handles the spread operator by loading all kinds of ES6 polyfills which is very time consuming for this one usage. Removing the spread syntax might stop that import and give savings.

So those are some ideas :).

Any context I'm missing that might make these futile? #2 might make #3 irrelevant.
I'll see if anyone has any thoughts before taking a stab at these changes...

@haroldtreen
Copy link
Contributor Author

@haroldtreen haroldtreen commented Dec 31, 2017

Just got rid of the array spreads in atomInterface and it seems to have reduced the activation time.

Here's the results from the profile:

image

And the timecop result:

image

248ms - 178ms = 70ms

Which seems in line with the time previously taken by babel-runtime import (which is no longer being required).

@haroldtreen
Copy link
Contributor Author

@haroldtreen haroldtreen commented Dec 31, 2017

Next thing causing long load times - lodash.

editorInterface is requiring all of lodash/fp so that it can use _.flow.

updateStatusTileScope and linterInterface are requiring all of editorInterface in order to access some helper methods (getCurrentFilePath and getCurrentScope). Neither of those helpers use lodash, but they are still blocked on it loading.

Inlining those helper functions gives significant savings.

Got the activation time down to 55ms 🎉

image

Inlining was my hack way of getting those functions out, so I'll see if I can rework things in a more elegant way...

haroldtreen added a commit to haroldtreen/prettier-atom that referenced this issue Dec 31, 2017
Remove need for ES6 array polyfills and only import lodash once required

re prettier#330
@robwise
Copy link
Collaborator

@robwise robwise commented Dec 31, 2017

@haroldtreen This is awesome. Maybe we can try babel-plugin-lodash for the lodash problem?: https://github.com/lodash/babel-plugin-lodash

@haroldtreen
Copy link
Contributor Author

@haroldtreen haroldtreen commented Dec 31, 2017

Could do. I've actually opened a PR that solves the lodash problem with lazy loading and importing flow directly.

#335

lodash doesn't appear to be a part of activation anymore, so the gains from that plugin would need to be measured elsewhere.

Still haven't looked at lazy loading the tile callback, so maybe could trim this down more. But I can live with a 55ms activation time 😊 .

haroldtreen added a commit to haroldtreen/prettier-atom that referenced this issue Dec 31, 2017
Remove need for ES6 array polyfills and only import lodash once required

re prettier#330
haroldtreen added a commit to haroldtreen/prettier-atom that referenced this issue Dec 31, 2017
Remove need for ES6 array polyfills and only import lodash once required

re prettier#330
@haroldtreen
Copy link
Contributor Author

@haroldtreen haroldtreen commented Jan 2, 2018

Been digging around a tiny bit more. Some thoughts:

  • import-lazy is nicer syntactically then having many lazy callbacks, but adding it everywhere doesn't necessarily uncover big savings.
  • setTimeout seems to be a promising way of deferring setup work until later.
    • In main.js#consumeIndie there's a call to linterInterface.set. By setting the linter interface in a setTimeout, that module no longer needs to be required for activate and that method doesn't need to be run. Saves a bit of time.
    • I noticed that atom.commands.add seemed to be executing the commands? Or at least displayDebugInfo was being run. That module has some external requires which eat a bit of time. By adding commands after activate using setTimeout, there's also some savings.
    • In createStatusTile you can use setTimeout to set the getFormatOnSaveStatus and attach the listener. That combined with the lazyImport saves those two functions from being required right away.

Overall, I've shaved another 15ms off the activation time with these techniques:

image

But maybe there's race conditions that this would introduce. I'm not super familiar with atom best practices...

Maybe also scratching the bottom of the barrel 😅 .

@haroldtreen
Copy link
Contributor Author

@haroldtreen haroldtreen commented Jan 2, 2018

Looks like other packages use requestIdleCallback (which is slightly better than using setTimeout). It looks like that can also be used for installing dependencies. I bet that would shave off a bunch of time.

https://github.com/AtomLinter/linter-eslint/blob/master/src/main.js#L53-L60

@haroldtreen
Copy link
Contributor Author

@haroldtreen haroldtreen commented Jan 21, 2018

I've been trying to blog about these random open source patches. Show libraries I like. Show how welcoming they are to contributions. Show open source isn't scary.

So my last one was about this fix: https://haroldtreen.com/tech/open-source/contributing-to/2018/01/15/contributing-to-prettier-atom/

Thanks again for the awesome project!

@robwise
Copy link
Collaborator

@robwise robwise commented Feb 5, 2018

@haroldtreen I read your blog post, was super cool! I think all of these ideas sound cool:

  • import-lazy: I didn't expect would get us any additional perf over our current from-scratch implementation, but I figured it would clean up the code a bit so I think it's worth doing
  • requestIdleCallback sounds like the way to go for those things you mentioned, and I don't see why in any of the cases there would be a race condition we'd need to worry about
@robwise
Copy link
Collaborator

@robwise robwise commented Feb 15, 2018

I'm going to close this for now because I think our objectives have been accomplished and activation time has been significantly reduced.

@robwise robwise removed the help wanted label Feb 15, 2018
@robwise robwise closed this Feb 15, 2018
@haroldtreen
Copy link
Contributor Author

@haroldtreen haroldtreen commented Apr 14, 2018

Seeing a regression on this one - async / await being used requires a polyfill that adds ~65ms to the startup profile:

image

@robwise
Copy link
Collaborator

@robwise robwise commented Apr 16, 2018

@haroldtreen thanks for reporting the regression! I wonder what the proper fix should be? It might be the case that Atom supports this stuff without a polyfill, but I'd have to check that.

I wonder if we could get away with just totally stopping using the Babel precompilation step entirely and we'd avoid these types of problems.

@haroldtreen
Copy link
Contributor Author

@haroldtreen haroldtreen commented Apr 16, 2018

While looking at the .bablerc env I noticed that electron supports a lot of the features that were triggering polyfill issues in the past (eg. ...). So hopefully fixing the env will reduce the need for so many babel transforms.

Maybe the best thing to do would remove the stage-2 preset? It's only the new features that trigger heavy polyfills to be added. I think babel will always be needed so flow types get stripped.

The real solution to the above issue will require removing the async/awaits.

@robwise
Copy link
Collaborator

@robwise robwise commented Apr 17, 2018

I think babel will always be needed so flow types get stripped.

Oh, duh! Good point!

I noticed that electron supports a lot of the features that were triggering polyfill issues in the past... Maybe the best thing to do would remove the stage-2 preset?

Another good point, I think you're right. I will try this out now in a new PR.

@robwise
Copy link
Collaborator

@robwise robwise commented Apr 17, 2018

Ok done! ^

@haroldtreen
Copy link
Contributor Author

@haroldtreen haroldtreen commented Sep 23, 2018

Looks like this has become a thing again 😅

image

@robwise
Copy link
Collaborator

@robwise robwise commented Sep 24, 2018

Oh my gosh, I wish there was some way to lock this down in some kind of automated performance test or something, but I don't think there's an easy way to do that. I'm reopening for now.

@robwise robwise reopened this Sep 24, 2018
@haroldtreen
Copy link
Contributor Author

@haroldtreen haroldtreen commented Sep 24, 2018

Looking at the diff I suspect...

  • require('lodash/fp') is back in the critical path of loading (In this PR it reduced the activation time by 120ms - #335)
  • There's new modules getting required (eg. src/helpers/isFileFormattable.js and src/helpers/isPrettierProperVersion.js).
@robwise
Copy link
Collaborator

@robwise robwise commented Sep 24, 2018

Okay I guess that means I can just go into those new modules and explicitly require or avoid using all of lodash/fp

@dcalhoun
Copy link

@dcalhoun dcalhoun commented Dec 8, 2018

I currently experience nearly a half second activitation time currently. 😞

image

Are you open to a PR replacing all require('lodash/fp') with specific requires? Do you believe that will mitigate the issue?

@robwise
Copy link
Collaborator

@robwise robwise commented Dec 8, 2018

Are you open to a PR replacing all require('lodash/fp') with specific requires?

Yes, but just for the files that are requested at activation time, what haroldteen refers to as the "critical path" (most of the code base is only required when a format on save or manual save is invoked). I think doing it everywhere would just get kind of annoying to work with, but is worth it if done just on the critical path files?

Do you believe that will mitigate the issue?

I do think that was the culprit last time, so I'd expect to see at least some performance gain from that, yes.

@j-f1
Copy link
Member

@j-f1 j-f1 commented Dec 8, 2018

You could also use something like Rollup to turn the package into a single file with minimal I/O.

@robwise
Copy link
Collaborator

@robwise robwise commented Dec 8, 2018

That's definitely a possibility, but I'm worried that may actually have the opposite effect. Right now, Atom is lazy-loading most of our codebase so we're avoiding a lot of code being on the critical path of Atom's startup routine.

If we put the entire codebase into a single file, we may end up increasing the load time, unless I'm miscalculating?

@j-f1
Copy link
Member

@j-f1 j-f1 commented Dec 8, 2018

@robwise You could use code splitting to split the code into what has to be run at startup and what is dynamically loaded later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.