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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid hangs when opening minified files #13820

Merged
merged 2 commits into from Feb 15, 2017

Conversation

Projects
None yet
@maxbrunsfeld
Contributor

maxbrunsfeld commented Feb 15, 2017

Fixes #979

Changes:

  1. We force soft wraps for lines that exceed 500 chars. This avoids the rendering bottleneck that we used to experience because we don't break lines into horizontal tiles. It's actually a pretty useful behavior, I think, because a single un-wrapped line of code is pretty hard to read.

  2. We don't pass more than 500 chars at a time to first-mate. There was already a limit to the number of tokens that we read from first-mate, but because the TextMate grammars can contain regexps with arbitrary lookahead and backtracking requirements, this token limit did not mitigate all of the performance problems of tokenizing large lines. Now we avoid passing more than 500 characters of text at a time to the tokenizer.

Benchmark results

We added a benchmark for opening large single-line files. When run agains Atom prior to this PR, this benchmark would basically not terminate 馃槵 . Now, it executes in a reasonable amount of time:

Opening a large single-line file:
512	1442.5700000000002
1024	2813.6100000000006
2048	5145.779999999995

Max time event loop was blocked after opening a large single-line file:
512	14.859999999999673
1024	8.095000000001164
2048	5.575000000004366

Clicking the editor after opening a large single-line file:
512	6.609999999999673
1024	3.9499999999970896
2048	4.955000000001746

Scrolling down after opening a large single-line file:
512	0.18499999999949068
1024	0.05000000000291038
2048	0.05000000000291038

You can see that the editor is immediately responsive after opening the file, regardless of its size. Unfortunately, the time to open a large single-line file still scales linearly with the size of the file. This is because the DisplayLayer currently indexes files on a line-wise basis, so for a single-line file, it has to index the entire file to answer any queries.

Still, a reasonably-sized minified file like jQuery 3.1.1 (85 kb, 4 lines), opens almost instantly 馃帀 .

/cc @ungb Can you give this a test, and link to any relevant issues about hangs when opening files with long lines?

Avoid hangs when opening minified files
* Force soft wraps for lines that exceed 500 chars
* Don't pass more than 500 chars to first-mate
@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Feb 15, 2017

Contributor

I'll take a look today! Thanks for getting to this. I'll look for all the issues related to performance and add them here.

Contributor

ungb commented Feb 15, 2017

I'll take a look today! Thanks for getting to this. I'll look for all the issues related to performance and add them here.

@50Wliu 50Wliu referenced this pull request Feb 15, 2017

Closed

Opening a minified js-file freezes the editor #492

1 of 1 task complete
@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Feb 15, 2017

Member

Will this fix #979?

Member

50Wliu commented Feb 15, 2017

Will this fix #979?

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Feb 15, 2017

Contributor

Will this fix #979?

Yeah, I think so.

Contributor

maxbrunsfeld commented Feb 15, 2017

Will this fix #979?

Yeah, I think so.

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Feb 15, 2017

Contributor

I was able to test this on windows x64 bit and saw that jquery.min.js loads up very quickly now where as before it causes Atom to freeze. I was able to verify other single long line files now respond (after disabling spell-check)

See atom/spell-check#190 for issue and repro on spell-check package.

Contributor

ungb commented Feb 15, 2017

I was able to test this on windows x64 bit and saw that jquery.min.js loads up very quickly now where as before it causes Atom to freeze. I was able to verify other single long line files now respond (after disabling spell-check)

See atom/spell-check#190 for issue and repro on spell-check package.

@maxbrunsfeld maxbrunsfeld merged commit 4a3c92f into master Feb 15, 2017

4 of 5 checks passed

ci/circleci Your tests failed on CircleCI
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@maxbrunsfeld maxbrunsfeld deleted the mb-ns-long-lines branch Feb 15, 2017

@benogle

This comment has been minimized.

Show comment
Hide comment
@benogle

benogle Feb 15, 2017

Contributor

Oh nice. 馃帀

Contributor

benogle commented Feb 15, 2017

Oh nice. 馃帀

@adamreisnz

This comment has been minimized.

Show comment
Hide comment
@adamreisnz

adamreisnz Feb 15, 2017

This is great, thanks for implementing

adamreisnz commented Feb 15, 2017

This is great, thanks for implementing

nathansobo added a commit that referenced this pull request Feb 17, 2017

Merge pull request #13820 from atom/mb-ns-long-lines
Avoid hangs when opening minified files
@timdp

This comment has been minimized.

Show comment
Hide comment
@timdp

timdp Feb 17, 2017

First of all, I love that there's finally a fix for Atom hanging on long lines. 馃憤

So, question: shouldn't the MAX_LINE_LENGTH_TO_TOKENIZE const be a setting of some sort? Its value 500 seems pretty arbitrary.

timdp commented Feb 17, 2017

First of all, I love that there's finally a fix for Atom hanging on long lines. 馃憤

So, question: shouldn't the MAX_LINE_LENGTH_TO_TOKENIZE const be a setting of some sort? Its value 500 seems pretty arbitrary.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Feb 17, 2017

Contributor

@timdp We considered making it a setting but it wasn't clear that anybody would want to have control over it since it's just circumventing a deficiency in our tokenizer until we have a chance to replace it with an improved parser. Are you interested in changing this value? If so, what's your rationale?

Contributor

nathansobo commented Feb 17, 2017

@timdp We considered making it a setting but it wasn't clear that anybody would want to have control over it since it's just circumventing a deficiency in our tokenizer until we have a chance to replace it with an improved parser. Are you interested in changing this value? If so, what's your rationale?

@timdp

This comment has been minimized.

Show comment
Hide comment
@timdp

timdp Feb 17, 2017

@nathansobo I probably wouldn't change it myself, but as a rule of thumb, I try to avoid magic constants like these in my own code. I can see how this feature is a stopgap though, so the simpler, the better, I suppose.

timdp commented Feb 17, 2017

@nathansobo I probably wouldn't change it myself, but as a rule of thumb, I try to avoid magic constants like these in my own code. I can see how this feature is a stopgap though, so the simpler, the better, I suppose.

@mbrancato

This comment has been minimized.

Show comment
Hide comment
@mbrancato

mbrancato Feb 18, 2017

I tested 1.15-beta3 with this patch and it does not seem to solve #979 (which has been closed). My example is a file which is either a single long line of base64 encoded data, of a file which has a line with a large blob of base64 encoded data.

An example here would be an HTML file with embedded images represented as base64 on a single line.

Atom 1.15-beta3 actually crashed while attempting to open the file.

mbrancato commented Feb 18, 2017

I tested 1.15-beta3 with this patch and it does not seem to solve #979 (which has been closed). My example is a file which is either a single long line of base64 encoded data, of a file which has a line with a large blob of base64 encoded data.

An example here would be an HTML file with embedded images represented as base64 on a single line.

Atom 1.15-beta3 actually crashed while attempting to open the file.

@maxbrunsfeld

This comment has been minimized.

Show comment
Hide comment
@maxbrunsfeld

maxbrunsfeld Feb 19, 2017

Contributor

Can you provide an example file @mbrancato?

Contributor

maxbrunsfeld commented Feb 19, 2017

Can you provide an example file @mbrancato?

@mbrancato

This comment has been minimized.

Show comment
Hide comment
@mbrancato

mbrancato Feb 19, 2017

Here's a file about the size I'm working with. I'm not sure what caused the crash on the file I'm opening. It happened only the first time I tried with the 1.15-beta3. Subsequent attempts open the file, but syntax highlighting is broken.

Opening a large PowerShell file with an embedded binary is what I was attempting. The version 1.15-beta3 appears to be slower at opening files, and handling edits of a file where a large line exists. The stable 1.14.3 appears to be much faster at inserting lines, and even rendering typing. I'm running the Mac version, if that matters.

Attached is a 9MB file of random data, then base64 encoded. It takes about 40 seconds to open on 1.15-beta3 for me.

9MB-random-base64.txt

I'll keep trying to run down the differences on my end to isolate anything useful.

mbrancato commented Feb 19, 2017

Here's a file about the size I'm working with. I'm not sure what caused the crash on the file I'm opening. It happened only the first time I tried with the 1.15-beta3. Subsequent attempts open the file, but syntax highlighting is broken.

Opening a large PowerShell file with an embedded binary is what I was attempting. The version 1.15-beta3 appears to be slower at opening files, and handling edits of a file where a large line exists. The stable 1.14.3 appears to be much faster at inserting lines, and even rendering typing. I'm running the Mac version, if that matters.

Attached is a 9MB file of random data, then base64 encoded. It takes about 40 seconds to open on 1.15-beta3 for me.

9MB-random-base64.txt

I'll keep trying to run down the differences on my end to isolate anything useful.

@andrewstart

This comment has been minimized.

Show comment
Hide comment
@andrewstart

andrewstart Feb 28, 2017

Possibly related, the mere 149 bytes here causes the JS syntax highlighter to hang - shortening the long line a little more allows it to finish, but it takes long enough to bring up the 'keep waiting/close' dialog. This happens on a Macbook Pro & a Windows 7 box.
hang.txt

andrewstart commented Feb 28, 2017

Possibly related, the mere 149 bytes here causes the JS syntax highlighter to hang - shortening the long line a little more allows it to finish, but it takes long enough to bring up the 'keep waiting/close' dialog. This happens on a Macbook Pro & a Windows 7 box.
hang.txt

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Feb 28, 2017

Member

Ok yeah, this is really bad. @andrewstart can you please create a new issue on atom/language-javascript so that we can prioritize it? Thanks!

Edit: atom/language-javascript#495

Member

50Wliu commented Feb 28, 2017

Ok yeah, this is really bad. @andrewstart can you please create a new issue on atom/language-javascript so that we can prioritize it? Thanks!

Edit: atom/language-javascript#495

@Oblik-Design

This comment has been minimized.

Show comment
Hide comment
@Oblik-Design

Oblik-Design Mar 12, 2017

it is getting ridiculus, 500 lines in html and hitting return to get a new line, takes 4 seconds on a strong computer. I love all the rest, but this is insane.

Oblik-Design commented Mar 12, 2017

it is getting ridiculus, 500 lines in html and hitting return to get a new line, takes 4 seconds on a strong computer. I love all the rest, but this is insane.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Mar 12, 2017

Member

@Oblik-Design please open a new issue.

Member

50Wliu commented Mar 12, 2017

@Oblik-Design please open a new issue.

@Oblik-Design

This comment has been minimized.

Show comment
Hide comment
@Oblik-Design

Oblik-Design Mar 12, 2017

there are hundreds of these, performance is a serious issues, a simple website will hit slowdowns ... atom ranks very last in performance, it is the only reason why it is loosing users by the secondes. Else it would be the only one on the map.

Oblik-Design commented Mar 12, 2017

there are hundreds of these, performance is a serious issues, a simple website will hit slowdowns ... atom ranks very last in performance, it is the only reason why it is loosing users by the secondes. Else it would be the only one on the map.

@adamreisnz

This comment has been minimized.

Show comment
Hide comment
@adamreisnz

adamreisnz Mar 12, 2017

I must agree with @Oblik-Design that raising individual issues for each performance related item is not really workable. I have now also reached the point that developing with Atom on my normally quite performant Macbook Air is becoming less and less productive because of intermittent lagging and performance related problems.

Sometimes it's just typing code that causes intermittent temporary freezes. Needless to say that's not really reproducible in a consistent manner, not will it affect people with a faster computer, so it's hard to debug. On my iMac the issues are less noticeable because there's more memory and processing power, but to be honest, the editor should run smoothly on a decent laptop as well.

Is there anything else we can do to help analyse these performance problems other than opening more new issues?

adamreisnz commented Mar 12, 2017

I must agree with @Oblik-Design that raising individual issues for each performance related item is not really workable. I have now also reached the point that developing with Atom on my normally quite performant Macbook Air is becoming less and less productive because of intermittent lagging and performance related problems.

Sometimes it's just typing code that causes intermittent temporary freezes. Needless to say that's not really reproducible in a consistent manner, not will it affect people with a faster computer, so it's hard to debug. On my iMac the issues are less noticeable because there's more memory and processing power, but to be honest, the editor should run smoothly on a decent laptop as well.

Is there anything else we can do to help analyse these performance problems other than opening more new issues?

@Oblik-Design

This comment has been minimized.

Show comment
Hide comment
@Oblik-Design

Oblik-Design Mar 12, 2017

I'm on a 4 core i7 , coud not imagine how bad it must be on an air... but again, Atom is I guess, single threaded ?

Oblik-Design commented Mar 12, 2017

I'm on a 4 core i7 , coud not imagine how bad it must be on an air... but again, Atom is I guess, single threaded ?

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Mar 12, 2017

Member

Opening a new issue allows us to triage it more effectively if you go through the prerequisites. For example, does it happen in safe mode? Is there already an open issue describing your performance issue? Are you on the latest version? Can you collect a profile of the slow performance? etc. That gives us much more information to work with than "500 lines in html and hitting return to get a new line, takes 4 seconds on a strong computer". Creating an issue also ensures that your performance woes aren't lost in the comments of an already-merged PR (that frankly doesn't seem to be that related to what you're seeing @Oblik-Design...unless those 500 lines are also very long).

As some examples, atom/language-javascript#493 and atom/language-javascript#495 were both addressed quickly once the issues were opened.

Member

50Wliu commented Mar 12, 2017

Opening a new issue allows us to triage it more effectively if you go through the prerequisites. For example, does it happen in safe mode? Is there already an open issue describing your performance issue? Are you on the latest version? Can you collect a profile of the slow performance? etc. That gives us much more information to work with than "500 lines in html and hitting return to get a new line, takes 4 seconds on a strong computer". Creating an issue also ensures that your performance woes aren't lost in the comments of an already-merged PR (that frankly doesn't seem to be that related to what you're seeing @Oblik-Design...unless those 500 lines are also very long).

As some examples, atom/language-javascript#493 and atom/language-javascript#495 were both addressed quickly once the issues were opened.

@Oblik-Design

This comment has been minimized.

Show comment
Hide comment
@Oblik-Design

Oblik-Design Mar 13, 2017

it is ok in safe mode, but i'm running only a few packages, terns , linter, emmet , highlight selected, and the autocompletes, you know, the ones you need for JS development , and no typescript or even react stuff. I only do es6 , I do babel, scss, prefixes, everything through webpack, so not using any packages to do the heavy lifting. I had issues at first , could not use atom, because it did not run on 4k monitors well, this got "fixed", js is fast if small , scss to, but html is a joke. Simple sites, no long lines.

Oblik-Design commented Mar 13, 2017

it is ok in safe mode, but i'm running only a few packages, terns , linter, emmet , highlight selected, and the autocompletes, you know, the ones you need for JS development , and no typescript or even react stuff. I only do es6 , I do babel, scss, prefixes, everything through webpack, so not using any packages to do the heavy lifting. I had issues at first , could not use atom, because it did not run on 4k monitors well, this got "fixed", js is fast if small , scss to, but html is a joke. Simple sites, no long lines.

@Oblik-Design

This comment has been minimized.

Show comment
Hide comment
@Oblik-Design

Oblik-Design Mar 13, 2017

Still I have to use Brackets if I want to have a look at what webpack is doing. It craches EVERY time I want to open the js file that webpack creates... Brackets is made with the same tools, so this is a serious issue. Every problems I have with atom, is performance related. Like I said earlier, other then that, it is incredible. sorry for my poor english.

Oblik-Design commented Mar 13, 2017

Still I have to use Brackets if I want to have a look at what webpack is doing. It craches EVERY time I want to open the js file that webpack creates... Brackets is made with the same tools, so this is a serious issue. Every problems I have with atom, is performance related. Like I said earlier, other then that, it is incredible. sorry for my poor english.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Mar 13, 2017

Member

And like I said earlier, it would be better for the both of us if you opened a new issue. But since you said the performance issues do not happen in safe mode, I would encourage you to go through your packages and disable individual packages until there are no performance issues left, then file issues on the appropriate package repositories. (You can also use the package-cop package to help with that)

Member

50Wliu commented Mar 13, 2017

And like I said earlier, it would be better for the both of us if you opened a new issue. But since you said the performance issues do not happen in safe mode, I would encourage you to go through your packages and disable individual packages until there are no performance issues left, then file issues on the appropriate package repositories. (You can also use the package-cop package to help with that)

@Oblik-Design

This comment has been minimized.

Show comment
Hide comment
@Oblik-Design

Oblik-Design Mar 13, 2017

well I can saffely say that the problem is with https://atom.io/packages/linter , ouchhh. this is needed

Oblik-Design commented Mar 13, 2017

well I can saffely say that the problem is with https://atom.io/packages/linter , ouchhh. this is needed

@ungb

This comment has been minimized.

Show comment
Hide comment
@ungb

ungb Mar 13, 2017

Contributor

@Oblik-Design Thanks for your report on performance issue you are seeing. As @50Wliu said, it's tough for us to triage issue that is within a closed PR as it's easy to get lost. When creating another issue and following the template it helps us triage and identify the underlying issue and get a fix out. Since you were able to pin point the issue to the linter package, please file an issue for them here: https://github.com/steelbrain/linter/issues/new.

Contributor

ungb commented Mar 13, 2017

@Oblik-Design Thanks for your report on performance issue you are seeing. As @50Wliu said, it's tough for us to triage issue that is within a closed PR as it's easy to get lost. When creating another issue and following the template it helps us triage and identify the underlying issue and get a fix out. Since you were able to pin point the issue to the linter package, please file an issue for them here: https://github.com/steelbrain/linter/issues/new.

@Oblik-Design

This comment has been minimized.

Show comment
Hide comment
@Oblik-Design

Oblik-Design Mar 13, 2017

well there is another one ... Emmet. I just cant loose both of these.

Oblik-Design commented Mar 13, 2017

well there is another one ... Emmet. I just cant loose both of these.

@stopdropandrew

This comment has been minimized.

Show comment
Hide comment
@stopdropandrew

stopdropandrew Mar 13, 2017

This caused me some trouble this morning. While I think having soft-wrap @ 500 characters on by default is totally fine, it's pretty strange to View->Toggle Soft Wrap on and off and just switch between a 200 and 500 character soft-wrap.

For reference, I was trying to view a csv roughly 3000 characters per line. Is there a middle ground?

stopdropandrew commented Mar 13, 2017

This caused me some trouble this morning. While I think having soft-wrap @ 500 characters on by default is totally fine, it's pretty strange to View->Toggle Soft Wrap on and off and just switch between a 200 and 500 character soft-wrap.

For reference, I was trying to view a csv roughly 3000 characters per line. Is there a middle ground?

@Oblik-Design

This comment has been minimized.

Show comment
Hide comment
@Oblik-Design

Oblik-Design Mar 13, 2017

I was completely wrong, it is only EMMET. I got confused.... my bad, linter is ok.
it at least makes sens, linter screwing html made no sense. but it's Emmet playground.

Oblik-Design commented Mar 13, 2017

I was completely wrong, it is only EMMET. I got confused.... my bad, linter is ok.
it at least makes sens, linter screwing html made no sense. but it's Emmet playground.

@lee-dohm

This comment has been minimized.

Show comment
Hide comment
@lee-dohm

lee-dohm Mar 13, 2017

Member

I'm going to lock this conversation. We prefer not to lock conversations but this has gotten out of hand and I feel that valuable information is getting lost. Here's what we need people to do:

Thanks everyone for your understanding!

Member

lee-dohm commented Mar 13, 2017

I'm going to lock this conversation. We prefer not to lock conversations but this has gotten out of hand and I feel that valuable information is getting lost. Here's what we need people to do:

Thanks everyone for your understanding!

@Oblik-Design

This comment has been minimized.

Show comment
Hide comment
@Oblik-Design

Oblik-Design Mar 13, 2017

I can also now open the webpack build file without issues. It was emmet fault, sorry guys. My bad. But I use EMMET so much ...

Oblik-Design commented Mar 13, 2017

I can also now open the webpack build file without issues. It was emmet fault, sorry guys. My bad. But I use EMMET so much ...

@atom atom locked and limited conversation to collaborators Mar 13, 2017

@lee-dohm

This comment has been minimized.

Show comment
Hide comment
@lee-dohm

lee-dohm Mar 13, 2017

Member

I've locked this conversation. We prefer not to lock conversations but this has gotten out of hand and I feel that valuable information is getting lost. Here's what we need people to do:

Thanks everyone for your understanding!

Member

lee-dohm commented Mar 13, 2017

I've locked this conversation. We prefer not to lock conversations but this has gotten out of hand and I feel that valuable information is getting lost. Here's what we need people to do:

Thanks everyone for your understanding!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.