CodeMirror has this API: http://codemirror.net/doc/manual.html#api_doc It looks like it allows you to dump a large file into it (like 50MB) and then get back an object that has all the internal state already set up, and you can simply swap it back in later. Doing so should make switching to large files almost instant, even if they originally took a long time to load.

Use CodeMirror document management API to cache internal state of large files

Here's a first draft. The patch changes the way that source files are shown in the editor. The old way simply replaced the editor document's source text and mode. This was pretty inefficient, when it came to jumping between large files. It also messes up the idea of change history between files. The new way maintains a dictionary of source editor documents that have been shown. That way, when it comes time to showing a previously opened source we can re-use the existing document. Notes: 1. there are two cases where we still set editor text (loading, errors) which i think would break a document, which should be fixed. 2. once those two cases are fixed there won't be any touch points for setEditorText and setEditorMode. I'm tempted to remove those functions. Does that make sense?

This patch simplifies the other patch and fixes the issue with loading and error text. The workflow looks like this: User selects a new source: 1. The debugger view creates an editor document, sets the "loading" text, then applies source text when it is available. The User re-selects a source: 1. The debugger view gets that source's document, checks the text and sees nothing has changed.

I forgot to add r=jlong to the commit message

Comment on attachment 8732233 [details] [diff] [review] large-files.2.patch Review of attachment 8732233 [details] [diff] [review]: ----------------------------------------------------------------- r- for now because I think there's a better approach that makes `_renderSourceText` more straight-forward. That code is already kind of complicated so I'd like to keep it as straight-forward as possible. Sorry I didn't get to this sooner; we can talk more next week about this. Thanks! ::: devtools/client/debugger/debugger-view.js @@ +489,5 @@ > ); > }, > _renderSourceText: function(source, textInfo, opts = {}) { > const selectedSource = queries.getSelectedSource(this.controller.getState()); > + const doc = this._setEditorDocument(source); It feels a little strange to do anything here. The `if` block below is supposed to make this function a noop if we shouldn't do anything. @@ +546,5 @@ > this._editorSource.actor = source.actor; > this._editorSource.prettyPrinted = source.isPrettyPrinted; > this._editorSource.blackboxed = source.isBlackBoxed; > let { text, contentType } = textInfo; > + if (doc.getValue() != text) { Can we avoid doing this, and just making `_setEditorText` do the actual `_setEditorDocument`? We shouldn't need to compare this, if we are at this point we know that we should render the source text because there are checks above that make sure we haven't already rendered it. I'm also worried about comparing strings; we're trying to make this behave fast and comparing a 10mb string may not be fast. I saw a previous version of this patch which I think I liked better: it seemed to replace `_setEditorText` but I could be remember wrongly. What there something awkard about the CodeMirror API that led you to this? ::: devtools/client/sourceeditor/editor.js @@ +490,5 @@ > /** > + * Creates a CodeMirror Document > + * @returns CodeMirror.Doc > + */ > + createDocument: function() { +1 I'm almost wondering if we should just drop our pretend Editor abstraction. It's supposed to make it feel like we're not using CodeMirror directly but its so leaky that it just doesn't make sense. I suppose we will basically do that with a React Editor component actually. It would replace this.

(In reply to James Long (:jlongster) from comment #6) > ::: devtools/client/sourceeditor/editor.js > @@ +490,5 @@ > > /** > > + * Creates a CodeMirror Document > > + * @returns CodeMirror.Doc > > + */ > > + createDocument: function() { > > +1 I'm almost wondering if we should just drop our pretend Editor > abstraction. It's supposed to make it feel like we're not using CodeMirror > directly but its so leaky that it just doesn't make sense. I don't think we should drop the Editor abstraction. * There's a lot of setup code needed that needs to be shared somewhere anyway * It also provides a lot of helpers that are more compact for us than the CM API (see things like canUndo, canRedo, moveLineUp, jumpToLine) * It's integrating with our prefs system and providing panel-based autocompletion which wouldn't be there when working with CM directly (see setOption and reloadPreferences) It is leaky for sure, but my impression is it mostly leaks with APIs like Markers (and potentially now Documents depending on how this is implemented).. things where we return CodeMirror objects directly like getMarker. We also use the CM API directly for a number of functions in CM_MAPPING, but I believe they all return primitives. > I suppose we will basically do that with a React Editor component actually. > It would replace this. Separate discussion from this bug, but I don't see what benefit we will get by switching to a React component for Editor, assuming we still load CM in an iframe (which we need to still do at least whenever it's loaded in a XUL doc, but I think is not a bad idea anyway for style containment at least). How about we have a call sometime next week for anyone interested - I think it'll be easier to talk this one out.

Thanks for the comments. Let's talk Monday about other approaches. I agree, the text comparison is less than ideal. Also, I wonder if we can get rid of the editor_doc hash in favor of more closely associating the source object and doc? Happy to also discuss the Editor API if others are interested.

Made a couple changes based on our conversation yesterday.

Comment on attachment 8733545 [details] [diff] [review] large-files.3.patch Review of attachment 8733545 [details] [diff] [review]: ----------------------------------------------------------------- This looks great! Exactly what was I was thinking. A few tweaks needed to make it work in various cases but generally it's good. Have you run the mochitests locally? They should expose these issues, and if you get it passing locally it'll probably run fine on try (can't think of any race conditions/leaks/etc that might happen) r+ this because it's close but make these tweaks and get tests passing! ::: devtools/client/debugger/debugger-view.js @@ +423,5 @@ > * The source text content. > */ > + _setEditorText: function(documentKey, aTextContent = "") { > + const isNew = !this._editorDocuments[documentKey]; > + this._setEditorDocument(documentKey); Instead of exposing `this._editorDocuments` here, can you make `_setEditorDocument` return a boolean value indicating whether or not a new document was created? Right now it returns the document but we don't use that. If we ever need the document we could create a `getDocument` but no need for that now. @@ +429,5 @@ > + // Only set editor's text and mode if it is a new document > + if (isNew) { > + this.editor.setMode(Editor.modes.text); > + this.editor.setText(aTextContent); > + this.editor.clearDebugLocation(); I believe you'll still need to call `clearDebugLocation` and `clearHistory` no matter what. If the document was swapped out but you didn't have to set the text, you still want to clear the debug line and other state. Try setting breakpoints and swapping back and forth. @@ +561,5 @@ > this._editorSource.prettyPrinted = source.isPrettyPrinted; > this._editorSource.blackboxed = source.isBlackBoxed; > > let { text, contentType } = textInfo; > + this._setEditorText(source.url, text); We can't use the URL as the key unfortunately. Welcome to the world of debugger sources :) Some sources do not have a URL, like eval scripts. Right now we hide these scripts (we show them if they have been given a URL with `//#sourceURL=foo.js`, but otherwise hidden). However, if the debugger stops in an unnamed eval script, because of a `debugger` statement or something else, we actually dynamically add it to source list and show it with a random name like SCRIPT0. But it has no URL. The `key` that we use to identify sources is the actor ID. So use `source.actor` instead. We'll need to revisit this when we want to support "updating" a source, but we don't support that right now anyway.

Notes: This patch is very similar to the previous approved patch. The changes come from some corner cases found by the tests: 1. reloading should clear the document cache so that changes to sources are shown 2. currently jumping between files resets the cursor to the top left. We'd probably prefer the cursor stay put, but we can easily implement that in another bug. 3. enabling / disabling pretty printing should update the editor. 4. jumping between files should reset the temporary breakpoint state kept in the debugger controller's dbginfo cache and codemirror's gutter class names

Comment on attachment 8734871 [details] [diff] [review] large-files.5.patch Review of attachment 8734871 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! Thanks for figuring out all the edge cases! ::: devtools/client/debugger/debugger-view.js @@ +431,4 @@ > this.editor.clearDebugLocation(); > this.editor.clearHistory(); > + this.editor.setCursor({ line: 0, ch: 0}); > + this.editor.removeBreakpoints(); Just curious: why is this needed? The call to `updateEditorBreakpoints` at the bottom of `_renderSourceText` should do this.

I'm not a fan of how breakpoints are currently managed. Under the hood, updateEditorBreakpoints uses hasBreakpoint to determine if a breakpoint should be added. hasBreakpoint checks the codemirror document, using the dom as a source of truth. Previously, when a new source was shown, the document's text was reset. Now, that's not the case and that's causing these issues.

(In reply to Jason Laster [:jlast] from comment #16) > I'm not a fan of how breakpoints are currently managed. Under the hood, > updateEditorBreakpoints uses hasBreakpoint to determine if a breakpoint > should be added. hasBreakpoint checks the codemirror document, using the dom > as a source of truth. Previously, when a new source was shown, the > document's text was reset. Now, that's not the case and that's causing these > issues. I'm not either. It was out-of-scope to clean up the CodeMirror instance in my refactor, but in the next pass we'll definitely make breakpoints have a single source of truth. Does that mean that the Document instance stores DOM?

The Document keeps the DOM associated with the lines including line info. Breakpoints are line wrap classes "breakpoint" "conditional" that were not being cleared. The debugger.hasBreakpoint function was checking for the presence of these classes on the lines to see if there was a breakpoint.

Yeah, I knew that it checks the DOM but I didn't realize that the Document object stores DOM as well. These Document instances must be pretty heavy. We can probably get rid of these checks now: https://github.com/mozilla/gecko-dev/blob/master/devtools/client/debugger/debugger-view.js#L524-L526 The editor tracked which source was being shown so that it wouldn't do unnecessary work, but we can safely dump in the same text twice now because it's so fast. We can simplify that code to make sure to set the cursor according to `opts` but always load the text. I filed bug 1260129 for that and assigned it to you, Jason, I hope that's ok. Hopefully it's pretty straight-forward, especially since you're familiar with that code now.