1232292 - Hang when setting sliding breakpoint on a large file in debugger

Status: RESOLVED DUPLICATE of bug 1230345

(DevTools :: Debugger, defect, P1)

Description

[Brian Grinstead [:bgrins]]

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

Another issue with large files in the debugger we noticed when looking at Bug 1224726 is that it is slow/hangs when setting a breakpoint that gets slid.

STR:
1. Open https://bgrins.github.io/devtools-demos/debugger/scripts.html
2. Switch to debugger panel
3. In 1mb.js, set a breakpoint on line 2 (warning, this might lock up the browser for a few seconds).  Eventually the bp moves to line 3.

Expected:
This happens quickly

Actual:
Not so much

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: use-line-no-for-bp-sliding.patch

I don't know if this is an OK thing to do.  In my smoke tests, bp sliding works fine and try agrees: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc14b7859875&selectedJob=14570687.

Here's what I've been able to figure out:

getAllOffsets from the Debugger API returns a fairly large sparse array that has very few elements in it.  In script.js, _setBreakpoint is very slow when iterating over all of these arrays, and most of the iterations do nothing since the offsets object isn't set.  For example:

* In my simple testcase at: https://bgrins.github.io/devtools-demos/debugger/scripts.html with a 1MB script there are 1768 script arrays and ~3000-25000 elements per array, of which < 10 are filled.
* In the Unity example attached to Bug 1224726 with a 50MB script there are 39213 script arrays with ~3000-25000 elements per array, of which < 200 are generally filled

This patch makes it so that in both cases only 2 script arrays are returned (still with a ton of empty elements in each sparse array).  I have more thoughts about how to optimize what's returned from the debugger API, but this at least stops the hang and seems relatively simple.  What I'm not sure is if there is some case that is missing from test coverage where we would need all of the scripts to be gathered.  The comments in the function are clear about gathering all scripts in the source but I don't know enough about it to be confident this isn't going to break anything.

The patch also updated another usages of getScriptsBySourceActor that appears it was intending to call getScriptsBySourceActorAndLine since it was passing a line number in.

Comment 2

[James Long (:jlongster)]

Comment on attachment 8698060 [details] [diff] [review]
use-line-no-for-bp-sliding.patch

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

::: devtools/server/actors/script.js
@@ +2793,5 @@
>            // entry points for the corresponding line.
>            let lineToEntryPointsMap = [];
>  
>            // Iterate over all scripts that correspond to this source actor.
> +          let scripts = this.scripts.getScriptsBySourceActorAndLine(this, originalLine);

I don't think you can change this one. Sourcemaps are very complicated: you can't just search the single line in the generated source, because a sourcemapped line may correspond to multiple generated lines. In fact, I just noticed your passing `originalLine` here which seems plain wrong; you're asking for a line from the generated source, no?

The other call looks like a mistake, and you're change is correct (it was already passing in `originalLine` but it wasn't calling the more optimal method).

If you want to land this, I say remove the change to this line and we can fix the other call which will fix it for non-sourcemapped sources. Bug 1230345 will remove the need for most of this which we can land soon.

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8698060 [details] [diff] [review]
use-line-no-for-bp-sliding.patch

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

::: devtools/server/actors/script.js
@@ +2793,5 @@
>            // entry points for the corresponding line.
>            let lineToEntryPointsMap = [];
>  
>            // Iterate over all scripts that correspond to this source actor.
> +          let scripts = this.scripts.getScriptsBySourceActorAndLine(this, originalLine);

This is the change that fixes the hang, the other one was just something I noticed in the process.  But I'm find with landing that part.

Note that both of these are wrapped in a !isSourceMapped condition so we are dealing with non-sourcemapped sources at this point: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/script.js#2730

Regarding originalLine, does that problem go away given the fact that we know this isn't source mapped?  It's used later on in this block: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/script.js#2832

Comment 4

[James Long (:jlongster)]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> 
> Note that both of these are wrapped in a !isSourceMapped condition so we are
> dealing with non-sourcemapped sources at this point:
> https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/script.
> js#2730

Oh you're right, I thought the `else` between those two lines was part of the "is sourcemapped" checked but it's actually part of the check for column numbers. I suppose your original patch is probably fine then, but Eddy should actually review this patch to make sure.

> 
> Regarding originalLine, does that problem go away given the fact that we
> know this isn't source mapped?  It's used later on in this block:
> https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/script.
> js#2832


Yeah, if it's not sourcemapped original line and generated line are the same.

I think we should probably just work on bug 1230345 though. I'd probably want Eddy to review this if we want to land it because he wrote that code, but I don't know if it's worth spending time doing that if we know we are going to remove most of this code. I think we'll probably remove all code for breakpoint sliding and re-introduce it in the future if we want to do it in some basic scenarios.

Comment 5

[Brian Grinstead [:bgrins]]

Alright, let's wait and see how Bug 1230345 pans out before proceeding here.  The only argument I'd make in favor of doing something now is that all existing tests seem to pass, but if this code is likely to be removed in the next release we'll just wait.

Comment 6

[James Long (:jlongster)]

Brian, can you try to reproduce this now in nightly (tomorrow)? bug 1230345 landed so this should be fixed.

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to James Long (:jlongster) from comment #6)
> Brian, can you try to reproduce this now in nightly (tomorrow)? bug 1230345
> landed so this should be fixed.

Nice, testing on a 1mb script and 50mb script I'm not able to reproduce the hang!  I still think we could optimize getAllOffsets as explained in Comment 1, but this problem is fixed so I'm going to dup this to Bug 1230345.

Comment 8

[James Long (:jlongster)]

You mean optimize the Debugger API itself to take in a line number? The only place we call `getAllOffsets` now is in `_breakOnEnter` and it's necessary there. What else needs to be optimized? (feel free to file a new bug though!)

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to James Long (:jlongster) from comment #8)
> You mean optimize the Debugger API itself to take in a line number? The only
> place we call `getAllOffsets` now is in `_breakOnEnter` and it's necessary
> there. What else needs to be optimized? (feel free to file a new bug though!)

The return value from getAllOffsets is a large sparse array where the index is used as data (line number I believe).  It looks like in the case of  _breakOnEnter, we never access the value at that index except to see if it exists.  In this case we could instead return an array of line numbers and save a lot of work in the actor.