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 #565

Open
50Wliu opened this issue Mar 21, 2017 · 32 comments
Open

Slow activation time #565

50Wliu opened this issue Mar 21, 2017 · 32 comments
Labels
performance Issues with the package's speed or CPU usage, including startup time.
Milestone

Comments

@50Wliu
Copy link

50Wliu commented Mar 21, 2017

This is 4th on my list at 140ms. Dunno if anything can be improved, just wanted to let you know.

[email protected].

@Alhadis
Copy link
Member

Alhadis commented Mar 21, 2017

Glad you asked. Merge these already:

This isn't just hacky, it's also bloated. The amount of code I've written to patch loaded packages is one thing; but you have any idea what that does to V8? It forces recompilation of already compiled JavaScript, so yeah, of course it's going to slow everything down.

@Alhadis
Copy link
Member

Alhadis commented Mar 21, 2017

FYI, the more packages that're active and cross-referencing loaded code, the bigger the hit to loading time.

Just saying. This isn't hyperbole, this is a pretty realistic concern.

@50Wliu
Copy link
Author

50Wliu commented Mar 21, 2017

They're prioritized (though we do have ~30 items on the Priority list).

@Alhadis
Copy link
Member

Alhadis commented Mar 21, 2017

Relief to exhausted ears. Seriously.

Thank you.

@gtalarico
Copy link

Getting 680ms on mine
image

@UndarkAido
Copy link

For me, loading is taking 291 ms and activation is taking 3415ms. I would be really great if this could be improved.

@Alhadis
Copy link
Member

Alhadis commented Jun 7, 2017

@50Wliu Uh, if my pull-requests could be merged sooner rather than later, that'd be great...

@50Wliu
Copy link
Author

50Wliu commented Jun 7, 2017

I do not merge PRs unless I am confident I understand the change, the relevant codebase, and am willing to deal with regressions/followups. Otherwise, the extent of my participation is making sure the relevant team members are aware of the PR (they are) and sometimes giving a basic review.

@AJP4
Copy link

AJP4 commented Jun 21, 2017

I'm an Atom recent convert. file-icons is my slowest at start-up; 1551ms. Followed by Spell-check and then Project-Viewer (1395 and 1152 ms respectively). It would be good to speed this up but I like the icons so will suffer if it is necessary.

@Alhadis
Copy link
Member

Alhadis commented Jun 21, 2017

Shouldn't be necessary forever, but that's up to the Atom team. :p

@Alhadis
Copy link
Member

Alhadis commented Nov 3, 2017

PRs merged! Gonna enjoy fixing this...

@50Wliu, you mightn't see that much of a speed improvement if you're running a recent build of Atom. But the plan is to make package activation asynchronous (and therefore, non-blocking on the main thread). The current hacks are dependent on execution order - they need to run before everything else has finished activating.

So I'll need to bump the minimum required Atom version once this lands in a stable release.

@Alhadis Alhadis added the performance Issues with the package's speed or CPU usage, including startup time. label Oct 15, 2018
@Alhadis Alhadis added this to the v2.2.0 milestone Oct 15, 2018
@dead-claudia
Copy link

Just as an update, I'm getting 671ms on my latest startup time for a relatively small (10K SLoC) project. So I'd love to see a fix for this happen!

@aminya
Copy link
Contributor

aminya commented Feb 12, 2020

On my system, it takes 160ms.
Doing some profiling during loading, most of the time is spent in these lines of code:
image

The lowest level is here:

setImmediate(() => StrategyManager.query(this.resource));

Can't we improve these?

@Alhadis
Copy link
Member

Alhadis commented Feb 12, 2020

@aminya During loading, the package has to run almost 2,000 regular expressions against every filename that's visible in the workspace. It's already memoising each result to reduce overhead, and the expressions are compiled ahead of time to reduce duplication.

All things considered, 160ms is actually pretty decent. If you can think of a better way to optimise 6,636 lines of filetypes, I'd like to hear it.

@aminya
Copy link
Contributor

aminya commented Feb 12, 2020

@aminya During loading, the package has to run almost 2,000 regular expressions against every filename that's visible in the workspace. It's already memoising each result to reduce overhead, and the expressions are compiled ahead of time to reduce duplication.

All things considered, 160ms is actually pretty decent. If you can think of a better way to optimise 6,636 lines of filetypes, I'd like to hear it.

Thank you for your answer!

May I ask which algorithm checks the regular expressions?
I don't see any C++, C, etc source code or binary (like re2). Is it in JavaScript?

@Alhadis
Copy link
Member

Alhadis commented Feb 12, 2020

Yes, it's pure JavaScript.

@aminya
Copy link
Contributor

aminya commented Feb 12, 2020

Yes, it's pure JavaScript.

Doing a quick check with a simple algorithm, it takes 15ms for my Julia code to check 100 words against a 2000 regex list (word length of 7 for example).
https://gist.github.com/aminya/4b6e6db1147e63727cffc781633a8e0b

@Alhadis
Copy link
Member

Alhadis commented Feb 12, 2020

What's your point, exactly?

@aminya
Copy link
Contributor

aminya commented Feb 12, 2020

What's your point, exactly?

I am saying that the time can be improved by using a better algorithm unless the issue is the slowness of the frontend API (creation of the icons shapes).

@Alhadis
Copy link
Member

Alhadis commented Feb 12, 2020

I don't know what algorithms V8 uses for its regular expression matching. It's probably a hybrid of several. Regardless, it's not something I have control over.

150ms versus 15ms is seriously too slow for you?

@Alhadis
Copy link
Member

Alhadis commented Feb 12, 2020

FYI, the execution speed is tangential to the problem reported in this thread. Matching doesn't happen until after file-icons has loaded and activated, and package activation happens on a blocking (synchronous) thread. That is, one package can't begin activating until the one loaded before it has finished its activation. This has a very noticeable impact on responsiveness, especially when the user has many packages installed.

Execution speed is less of a concern because it typically happens in response to user activity, such as clicking a folder in the tree-view. Any sluggishness felt here is likely to be caused by synchronous system calls (which are still an unresolved problem).

@dead-claudia
Copy link

I do want to note that my slow startup time is on an older computer in need of repair - I/O is dog slow and the onboard hardware diagnostics report nothing except an unrelated battery issue that long pre-dated the I/O issues.

So my 671ms is probably equivalent to about 1/4 that for an equivalent working laptop.


Separately, why on earth did Atom not make package initialization async? </rant>

@Alhadis
Copy link
Member

Alhadis commented Feb 12, 2020

@isiahmeadows I'm in the same boat with an ancient MacBook and an equally ancient Dell laptop with almost non-existent battery life (both have low-density screens too, which seriously sucks when you're preparing vector images for icon display). Unfortunately I've no money to afford a new one...

Separately, why on earth did Atom not make package initialization async?

Heh. Wouldn't be the first time I've asked that myself. 😉

@aminya
Copy link
Contributor

aminya commented Feb 13, 2020

I don't know what algorithms V8 uses for its regular expression matching. It's probably a hybrid of several. Regardless, it's not something I have control over.

150ms versus 15ms is seriously too slow for you?

Well, it is 10X speed up. This is a huge improvement for me.
I believe by just using Node-re2, we can get at least 7X speedup compared to V8 (ref.). More speed is achievable by using Hyperscan which makes a database of regex patterns.

FYI, the execution speed is tangential to the problem reported in this thread. Matching doesn't happen until after file-icons has loaded and activated, and package activation happens on a blocking (synchronous) thread. That is, one package can't begin activating until the one loaded before it has finished its activation. This has a very noticeable impact on responsiveness, especially when the user has many packages installed.

Execution speed is less of a concern because it typically happens in response to user activity, such as clicking a folder in the tree-view. Any sluggishness felt here is likely to be caused by synchronous system calls (which are still an unresolved problem).

If loading of the icondb.js is a problem, we should use a binary format for storing and loading the data.

Again, my suggestions is based on a quick glance at the repo. I don't know the codebase.

@Alhadis
Copy link
Member

Alhadis commented Feb 13, 2020

Well, it is 10X speed up. This is a huge improvement for me.

And you're suggesting we add a hard dependency on a runtime library that we actually don't need? Just for a negligible performance improvement? 10× sounds like a lot, but you're talking about pre-cache execution at startup (when every icon visible in the workspace causes a simultaneous filename/icon lookup, causing ostensibly heavy CPU usage).

Furthermore, the time saved by using node-re2 is probably lost loading and linking the bindings at startup, which also involves a working C++ compiler. Ergo, users with misconfigured environments or botched LLVM installations are shit-out-of-luck. Just to save a few cycles when using an already performance-challenged editor. That sounds logical to you?

If loading of the .icondb.js is a problem, we should use a binary format for storing and loading the data.

I already have one in mind, but I haven't started work on it yet. But I'd like to hear your thoughts on how you'd implement it.

@aminya
Copy link
Contributor

aminya commented Feb 13, 2020

Well, it is 10X speed up. This is a huge improvement for me.

And you're suggesting we add a hard dependency on a runtime library that we actually don't need? Just for a negligible performance improvement? 10× sounds like a lot, but you're talking about pre-cache execution at startup (when every icon visible in the workspace causes a simultaneous filename/icon lookup, causing ostensibly heavy CPU usage).

Furthermore, the time saved by using node-re2 is probably lost loading and linking the bindings at startup, which also involves a working C++ compiler. Ergo, users with misconfigured environments or botched LLVM installations are shit-out-of-luck. Just to save a few cycles when using an already performance-challenged editor. That sounds logical to you?

If loading of the .icondb.js is a problem, we should use a binary format for storing and loading the data.

I already have one in mind, but I haven't started work on it yet. But I'd like to hear your thoughts on how you'd implement it.

If Atom doesn't provide an easy way to use C, C++ code, that explains all of its performance issues

@Alhadis
Copy link
Member

Alhadis commented Feb 13, 2020

Slight correction: it only explains some of its performance issues. Package code can use native add-ons, but doing so requires extra tooling that users may or may not have available. In addition, new versions of Atom will usually require each native add-on to be recompiled, and all-in-all, the payoff simply isn't worth it.

In conclusion, the performance issues aren't to do with V8's perceived inability to match strings efficiently (in fact, its string-handling is quite performant). It's more to do with I/O and blocking startup than anything else.

@aminya
Copy link
Contributor

aminya commented Feb 13, 2020

Added an issue describing the situation and the needs for a better user experience.

@Alhadis
Copy link
Member

Alhadis commented Feb 13, 2020

You mentioned WASM. WebAssembly is different to C/C++ bindings; it can be precompiled and checked into this package like ordinary source code, eliminating the potential edge-cases I described earlier. node-re2 uses native bindings, which != WASM.

However, I'm still not about to turn this project on its head to eek out a few drops of performance that may or may not benefit every user. I'm still confused as to what the real problem you're trying to solve is.

@aminya
Copy link
Contributor

aminya commented Feb 13, 2020

You mentioned WASM. WebAssembly is different to C/C++ bindings; it can be precompiled and checked into this package like ordinary source code, eliminating the potential edge-cases I described earlier. node-re2 uses native bindings, which != WASM.

Well, the native addons are just a wrapper around the actual code. For example:
https://github.com/google/re2

However, I'm still not about to turn this project on its head to eek out a few drops of performance that may or may not benefit every user. I'm still confused as to what the real problem you're trying to solve is.

https://discuss.atom.io/t/why-is-atom-so-slow/11376

@Alhadis
Copy link
Member

Alhadis commented Feb 13, 2020

Well, the native addons are just a wrapper around the actual code.

I know what the re2 library is. There's still no compelling reason to use it. Adding complexity for the sake of trivial performance gain is micro-optimising.

Moreover, if I were to take the WASM route, I'd just rewrite the entire damn library in ISO C so it could be called from Atom and any other program. In fact, that's actually what I already plan to do (eventually). Until then, I'd rather limit changes to this codebase to only those which are important, like making I/O calls async (which has proven to be difficult enough already).

Getting this package to a point of stability took a lot of work and hair-pulling, and that's not something I'm eager to repeat when the payoff is infinitesimal.

@aminya
Copy link
Contributor

aminya commented May 7, 2020

I added a couple of patches to solve this issue. #814, #815, #805 are among those. These reduced the loading time from about 450 ms to 60 ms. This is like 8 times faster!

More PRs are coming. I have made it 50 ms locally.

If we optimize the JavaScript code itself, we can gain a lot even without touching WASM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Issues with the package's speed or CPU usage, including startup time.
Projects
None yet
Development

No branches or pull requests

7 participants