1224726 - High memory consumption when opening and searching a large Javascript file in debugger.

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P1)

Description

[Jukka Jylänki]

+++ This bug was initially created as a clone of Bug #1158098 +++

Somewhat similar to bug #1158098, but reporting separately since a different STR (long lines vs short lines) and slightly different behavior (crash vs no crash), which makes it possible that the fixes could end up being something different.

STR:

1. Download https://dl.dropboxusercontent.com/u/40949268/emcc/bugs/2015-08-28-emunittest_0.4-AngryBots-u5.1.3f1_hg-e1.34.6-release-devbuild-exceptions.zip and unzip.
2. Open index.html
3. Check Firefox memory usage, e.g. open about:memory and Measure, which shows something like

175.25 MB (100.0%) -- explicit

4. Open JS debugger on the file Development/2015-08-28-emunittest_0.4-AngryBots-u5.1.3f1_hg-e1.34.6-release-devbuild-exceptions.js
5. Check memory usage again. This time it is around

640.98 MB (100.0%) -- explicit

which is an increase of +465.73 MB. Not ideal, and perhaps a bit odd, since the big JS file itself was only 56.6MB, so debugger consumes ~10x the memory of the JS file on disk. Still probably bearable.

6. Tap Ctrl-F in the text field containing the JS code to search. Firefox will hang for ~5 minutes or so.

7. Check memory usage again. Now it states:

5,794.72 MB (100.0%) -- explicit
├──5,220.05 MB (90.08%) -- js-non-window
│  ├──5,165.29 MB (89.14%) -- zones
│  │  ├──5,157.86 MB (89.01%) -- zone(0x3c4576f800)
│  │  │  ├──5,092.49 MB (87.88%) -- compartment([System Principal], resource://gre/modules/reflect.jsm)
│  │  │  │  ├──5,092.48 MB (87.88%) -- classes
│  │  │  │  │  ├──5,027.82 MB (86.77%) -- class(Object)
│  │  │  │  │  │  ├──5,027.81 MB (86.77%) -- objects
│  │  │  │  │  │  │  ├──3,352.67 MB (57.86%) ── malloc-heap/slots
│  │  │  │  │  │  │  └──1,675.14 MB (28.91%) ── gc-heap

This time it reads about 6GB of memory used. Interestingly closing the debugger, or even the page in question does not (immediately?) release the memory. Clicking on "Minimize memory usage" in about:memory after closing the page does free it.

Comment 1

[James Long (:jlongster)]

Thanks for filing this. As mentioned in other bugs, we are somewhat restricted to what CodeMirror can handle. But we probably can make searching a lot better, by not sending huge amounts of text between the client & server.  I'm not sure how far we'll get here but we definitely want to work on it in the next few months.

Comment 2

[James Long (:jlongster)]

I suggest that this bug be merged back with bug 1158098. Although the STR is different, they are both symptoms of the same issue: we load in all the sources in the frontend and search them there, when we really should just do the searching in the backend.

We're going to work on moving searching to the backend in Q1 of next year, and I'm filing some bugs for that. I'll mark that as blocking bug 1158098. Is it ok if we close this one?

Comment 3

[James Long (:jlongster)]

Actually, let's keep this open. I just realized that it may not be the issue of searching code in the frontend, since you already opened the file it's already loaded on the frontend. We should investigate this and the other bug separately.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to James Long (:jlongster) from comment #2)
> I suggest that this bug be merged back with bug 1158098. Although the STR is
> different, they are both symptoms of the same issue

(I know you already change your mind, but for future reference...)  In general, I've found it helps to keep bugs with different STRs open until the assumed fix actually lands, so you can verify that they actually are for real the same.  Then you can go through each STR and verify the new state after the fix (helps to have them all depend on the bug with the fix).  Basically, I'd suggest avoiding closing as a dupe unless you have extremely high confidence they are identical.

In particular, I know Jukka has ranted about the tendency to "early resolve as dupe" only to end up with an assumed fix that doesn't actually fix all variants of the problem.

Anyway, no process is perfect, just a suggestion.

Comment 5

[James Long (:jlongster)]

Yeah, that's a good point. In this case I thought it was something where the feature itself was going to be completely re-done, so it's unlikely the STR would produce the same results. But it could produce different erroneous results, so that's true.

Comment 6

[Brian Grinstead [:bgrins]]

This can be resolved by not using the parser when starting a search on a large file: https://dxr.mozilla.org/mozilla-central/source/devtools/client/debugger/views/filter-view.js#450.  We should bail out and not try to figure out the current function when the file is too big.

Comment 7

[Brian Grinstead [:bgrins]]

Attachment: debugger-search-fix-WIP.patch

WIP - needs a test.  Using the same const as we do to prevent syntax highlighting (1MB file size).

Comment 8

[James Long (:jlongster)]

Comment on attachment 8697769 [details] [diff] [review]
debugger-search-fix-WIP.patch

Review of attachment 8697769 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to have a test for this too, but I'm honestly not sure how to do that. Loading a 50MB file in the debugger takes several seconds, which is going to be terrible for our try servers. We've already disabled all debugger tests on linux32 debug because the whole suite takes too long to run, and they can't split it out into any more chunks because it's all in a single directory.

I'd love to have a way to "mock" a large file and run it through the system and see if the features are turned off. But I don't think the debugger code is quite there yet. After I clean up more of the code we should be able to do things like that.

I don't know what to do because we really should have a test.

::: devtools/client/debugger/debugger-view.js
@@ +5,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  "use strict";
>  
>  const SOURCE_SYNTAX_HIGHLIGHT_MAX_FILE_SIZE = 1048576; // 1 MB in bytes
> +const AUTOFILL_SEARCH_MAX_FILE_SIZE = 1048576; // 1 MB in bytes

I'd prefer to just define something like `LARGE_FILE_SIZE` that everything uses to turn off features. Most of these features will probably depend on parsing the script so they all have a similar limit.

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to James Long (:jlongster) from comment #8)
> Comment on attachment 8697769 [details] [diff] [review]
> debugger-search-fix-WIP.patch
> 
> Review of attachment 8697769 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to have a test for this too, but I'm honestly not sure how to do
> that. Loading a 50MB file in the debugger takes several seconds, which is
> going to be terrible for our try servers. We've already disabled all
> debugger tests on linux32 debug because the whole suite takes too long to
> run, and they can't split it out into any more chunks because it's all in a
> single directory.
> 
> I'd love to have a way to "mock" a large file and run it through the system
> and see if the features are turned off. But I don't think the debugger code
> is quite there yet. After I clean up more of the code we should be able to
> do things like that.
> 
> I don't know what to do because we really should have a test.

We've done something like this with the console or the markup view, where we attach the number onto an object that the test can modify or make it a pref value and then the test makes it smaller before starting.  So that way we could make it only 1KB in a single test.

> ::: devtools/client/debugger/debugger-view.js
> @@ +5,5 @@
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  "use strict";
> >  
> >  const SOURCE_SYNTAX_HIGHLIGHT_MAX_FILE_SIZE = 1048576; // 1 MB in bytes
> > +const AUTOFILL_SEARCH_MAX_FILE_SIZE = 1048576; // 1 MB in bytes
> 
> I'd prefer to just define something like `LARGE_FILE_SIZE` that everything
> uses to turn off features. Most of these features will probably depend on
> parsing the script so they all have a similar limit.

Yeah, makes sense.  We could then also have a single test that uses some method as above to make sure that both features are disabled.

Comment 10

[Brian Grinstead [:bgrins]]

Just a note about the root cause of the slowness / memory usage.  Parser.jsm eventually uses Reflect.jsm, which causes the hang when trying to parse the source text.  As an example, running this in the browser console is extremely slow:

var {Reflect} = Cu.import("resource://gre/modules/reflect.jsm", {});
Reflect.parse(new Array(1048576).join("function foo() {}\n"))

Comment 11

[Brian Grinstead [:bgrins]]

Going to add a test for this and send a patch

Comment 12

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 1224726 - Do not attempt to parse source file when finding if text > 1MB;r=jlongster

Bug 1224726 - Do not attempt to parse source file when finding if text > 1MB;r=jlongster

Comment 13

[James Long (:jlongster)]

Comment on attachment 8698108 [details]
MozReview Request: Bug 1224726 - Do not attempt to parse source file when finding if text > 1MB;r=jlongster

https://reviewboard.mozilla.org/r/27803/#review25007

We need the test to check that the source isn't getting parsed, not that syntax highlighting is disabled. Sorry for another r- but can you change that? :) Let me know if you're busy and I can update it as well.

::: devtools/client/debugger/test/mochitest/browser_dbg_sources-large.js:27
(Diff revision 1)
> +    gDebugger.DebuggerView.LARGE_FILE_SIZE = 1;

How is this... possible. It's a `const`. I think `const` declarations weren't available outside the scope of the file anymore? This actually works?

::: devtools/client/debugger/test/mochitest/browser_dbg_sources-large.js:32
(Diff revision 1)
> +      is(gEditor.getMode().name, "text",

Hm, this tests mainly seems to make sure that it isn't syntax highlighted. But we actually can syntax highlight large sources and I'm going to turn it on in bug 929225. We need to somehow check that the contents aren't getting parsed. I'm not sure how to check that though. Can you possibly check the Parser cache and make sure that it's not in there, but the smaller file is in there?

Comment 14

[Brian Grinstead [:bgrins]]

https://reviewboard.mozilla.org/r/27803/#review25007

> How is this... possible. It's a `const`. I think `const` declarations weren't available outside the scope of the file anymore? This actually works?

We discussed this on IRC - right now the const is copied onto the debuggerview object. Insetad we will just put the value straight onto that object.

> Hm, this tests mainly seems to make sure that it isn't syntax highlighted. But we actually can syntax highlight large sources and I'm going to turn it on in bug 929225. We need to somehow check that the contents aren't getting parsed. I'm not sure how to check that though. Can you possibly check the Parser cache and make sure that it's not in there, but the smaller file is in there?

The check below about ctrl+f is what does the parser stuff.  How it works right now is if your cursor is over a function name for instance and you press ctrl+f it pre-populates the search box with that name.  But in a large file the search box will just have '#'

Comment 15

[James Long (:jlongster)]

Ah you're right, I forgot that has that different behavior. Can you remove the syntax highlighting test then? I'd like to land that fix this week since it really does seem to work fine.

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8698108 [details]
MozReview Request: Bug 1224726 - Do not attempt to parse source file when finding if text > 1MB;r=jlongster

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27803/diff/1-2/

Comment 17

[Brian Grinstead [:bgrins]]

Try push with both this and the patch from Bug 929225: https://treeherder.mozilla.org/#/jobs?repo=try&revision=35b8b1c43d25

Comment 18

[James Long (:jlongster)]

Comment on attachment 8698108 [details]
MozReview Request: Bug 1224726 - Do not attempt to parse source file when finding if text > 1MB;r=jlongster

https://reviewboard.mozilla.org/r/27803/#review25021

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/063c28222310

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/063c28222310

Comment 21

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: 
File is not present on Dropbox.

Component: 
Name 			Firefox
Version 		46.0b9
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Expected Results: 
Memory based test depends on index.html

Actual Results: 
No zip file has found on dropbox.