929225 - Don't fallback to text syntax highlighting mode for large files

Status: VERIFIED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Anton Kovalyov (:anton)]

In the debugger-view.js we have the following code:

  if (aTextContent.length >= SOURCE_SYNTAX_HIGHLIGHTING_MAX_FILE_SIZE)
    return void this.editor.setMode(Editor.mode.text);

I'm not sure this is still necessary with bug 919709.

Comment 1

[Victor Porof [:vporof][:vp]]

I removed it and loading the asm.js code for http://www.unrealengine.com/html5/ is instant now! But that's on my mac. We need to test this on old hardware too.

Comment 2

[Girish Sharma [:Optimizer]]

Note that very long lines are still an issue in CM (https://github.com/marijnh/CodeMirror/issues/1022)

Comment 3

[Anton Kovalyov (:anton)]

Maybe increase the value of SOURCE_SYNTAX_HIGHLIGHTING_MAX_FILE_SIZE?

Comment 4

[Girish Sharma [:Optimizer]]

I think we should go with a better heuristics to determine the threshold instead of just file size . Even if CM can now load even bigger files faster, it is slow on minified single lined files which are very long. Maybe have a heuristics depending on the max (or median) line size.

Comment 5

[James Long (:jlongster)]

Attachment: 929225.patch

This is very doable now. I tested a 50MB JS file with a line that that 40K chars on it and it was fine. I think we should enable and see if anyone finds problems with it in the wild.

Comment 6

[James Long (:jlongster)]

Comment on attachment 8697736 [details] [diff] [review]
929225.patch

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

Brian, I tested this on a huge file and CodeMirror has definitely optimized for this. It probably only highlights the code in the viewport, and doesn't need to parse the code, just uses regexs to match keywords.

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8697736 [details] [diff] [review]
929225.patch

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

I'm worried about adding anything that might slow down opening large files since it can still be really janky with huge files, especially on lower memory devices.  However I've profiled this and and most of time spent janking on load has nothing to do with highlighting.  In my measurements with a 50MB file setting the mode to js took around 73ms while setting it to text took around 55ms.  In both cases setting the text in the editor took around 8 seconds total so this is a drop in the bucket.  Also, the highlighting runs after the load is done and once the framerate is back up to normal.

::: devtools/client/debugger/debugger-view.js
@@ -409,5 @@
>     */
>    _setEditorMode: function(aUrl, aContentType = "", aTextContent = "") {
> -    // Avoid setting the editor mode for very large files.
> -    // Is this still necessary? See bug 929225.
> -    if (aTextContent.length >= SOURCE_SYNTAX_HIGHLIGHT_MAX_FILE_SIZE) {

Please also remove the const at the top of the file

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/09d738f5f284

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09d738f5f284

Comment 10

[Mayur Patil]

[bugday-20160323]

STR:
Please mention "steps to reproduce" clearly for verification during testdays.

Status: RESOLVED -> UNVERIFIED

Component: 
Name 	        Firefox
Version 	46.0b2
Build ID 	20160314144540
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

Actual Results: 
Ambiguous steps to reproduce problem. So from whatever data is there:
1. On https://bugzilla.mozilla.org/show_bug.cgi?id=929225
2. Open Debugger, there is no file in sources window named as view.js
3. Test fails. 

Expected Results: 
Please give STR for testing.

Comment 11

[James Long (:jlongster)]

Please file a new bug.

Comment 12

[arni2033]

Tested on Win7_64, with 3 scripts ( >1Mb each one):
> [1]  data:text/html,<script src="https://s.ytimg.com/yts/jsbin/player-ru_RU-vfl_2nEnm/base.js"></script>
> [2]  data:text/html,<script src="https://plus.google.com/_/scs/apps-static/_/js/k=oz.home.ru.FDWJAtZQrp4.O/m=b,prc/am=YE2ChoYAAFA/rt=j/d=1/rs=AGLTcCMFp2P8QX4-zVMpXQ_998diS9a19Q"></script>
> [3]  data:text/html,<script src="https://accounts.firefox.com//scripts/main.js"></script>

Fix range:
> https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=d5fd77678f2cef6ca58f2c4b9901e22d375876c5&tochange=063c2822231032a310ad6508daf7a52486ec2e5c