Closed Bug 1238173 Opened 4 years ago Closed 4 years ago

Investigate removing ScriptStore

Categories

(DevTools :: Debugger, defect, P1)

45 Branch
defect

Tracking

(firefox50 fixed)

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: bgrins, Assigned: jlong)

References

Details

User Story

Bug 1230345 made breakpoint sliding a lot more tolerant to scripts being GCed. Since scripts can still be GCed before the toolbox opens, we never fully solved that problem anyway. Now, GCed scripts aren't really a big deal (our new sliding algorithm handles it well), so the ScriptStore can probably be removed. This will also potentially help memory usage of the user's page because we aren't keeping everything alive.

Attachments

(1 file, 1 obsolete file)

It doesn't appear to be used anymore as of Bug 1230345, so seems like it could be removed
Unless if there's some reason to keep it around..
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
I'd say it should be there just to have a complete API. It might be used in the future.

However, I think what this bug should morph into is an investigation about removing the ScriptStore entirely. We built it to help with breakpoint sliding by keeping all scripts in memory (never letting them be GCed). But the problem is that behavior still exists if you don't open the toolbox until after a bunch of GCs have happened. And we fixed breakpoint sliding with bug 1230345. It's now OK if a script gets GCed, we handle it pretty well.

It shouldn't be that much work; the few places that use the script store need to go back to using the Debugger API to query for scripts.

It might potentially speed up debugger load time as well, because right now we call `findScripts` on a source to get *all* scripts when it's introduced. For sources that have tons of scripts (huge sources) there's likely to be a speedup here.

Anyway, I'm fine if you want to just land this bug and I can investigate in a separate bug. What do you think?
(In reply to James Long (:jlongster) from comment #3)
> I'd say it should be there just to have a complete API. It might be used in
> the future.
> 
> However, I think what this bug should morph into is an investigation about
> removing the ScriptStore entirely. We built it to help with breakpoint
> sliding by keeping all scripts in memory (never letting them be GCed). But
> the problem is that behavior still exists if you don't open the toolbox
> until after a bunch of GCs have happened. And we fixed breakpoint sliding
> with bug 1230345. It's now OK if a script gets GCed, we handle it pretty
> well.
> 
> ...
> 
> Anyway, I'm fine if you want to just land this bug and I can investigate in
> a separate bug. What do you think?

Let's just repurpose the bug as you propose
Assignee: bgrinstead → nobody
Status: ASSIGNED → NEW
Summary: Remove getScriptsBySourceActor from ScriptStore → Investigate removing ScriptStore
Attachment #8705878 - Attachment is obsolete: true
Attachment #8705878 - Flags: review?(jlong)
I can take it, I was planning on doing that work after bug 1230345. Probably won't to it until next week. Need to do careful benchmarking to make sure we don't regress on anything (DAMP will help with that, but I want to local preliminary numbers as well)
Assignee: nobody → jlong
User Story: (updated)
Assignee: jlong → jlaster
Priority: -- → P3
This is potentially blocking bug 670002, which sourcemaps the console. In my opinion, that feature is higher priority than almost anything else, and this bug should not take too long, so I'm going to take it on. We want to do this to clean up the debugger server code, and we would probably do it when moving sourcemaps to the client anyway.

I'll research this today and come up with an estimate for how hard it will be.
Assignee: jlaster → jlong
Priority: P3 → P1
Attached patch 1238173.patchSplinter Review
This was surprisingly easy. Now that our sliding is much more defensive about GC'ed scripts, we can remove the script store and allow things to query the engine directly. This makes it a lot easier for other tools to use the sources because they can get SourceActors on demand without having the debugger initialized yet (we don't want to load in all the sources if just opening the console).

My one question is `_restoreBreakpoints`. No tests failed when I removed it, and I can't figure out what it does. Shouldn't the breakpoint store always be empty in the `onAttach` method, because we never detach and re-attach the same debugger server right? When the devtools closes, the breakpoint store is destroyed so it'll be empty when the devtools is opened again.
Attachment #8765568 - Flags: review?(nfitzgerald)
Attachment #8765568 - Flags: feedback?(ejpbruel)
Comment on attachment 8765568 [details] [diff] [review]
1238173.patch

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

I see nothing obviously wrong with this patch, so f+ for me.

::: devtools/server/actors/source.js
@@ +719,5 @@
> +        const query = this.source ?
> +            { source: this.source } :
> +            { url: this.url };
> +        query.line = originalLine;
> +        const scripts = this.dbg.findScripts(query);

I find this a bit obtuse. How about this instead?

const query = { line: originalLine });
if (this.source) {
  query.source = this.source;
} else {
  query.url = this.url;
}
const scripts = this.dbg.findScripts(query);

@@ +845,1 @@
>  

Same comment as above.
Attachment #8765568 - Flags: feedback?(ejpbruel) → feedback+
I think that _restoreBreakpoints has been obsolete for a bit now.

Can you test the performance of getting the source list for pages with huge numbers of sources and scripts? http://lampwww.epfl.ch/~doeraene/presentations/scala-js-scaladays2013/#/ is a test case that I often used. IIRC, the ScriptStore provided a performance boost for such pages because Debugger#findScripts is by nature very slow.

If the ScriptStore does not provide a performance boost, then off with its head.
Comment on attachment 8765568 [details] [diff] [review]
1238173.patch

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

r=me iff the ScriptStore doesn't provide any performance boost anymore.

::: devtools/server/actors/source.js
@@ +719,5 @@
> +        const query = this.source ?
> +            { source: this.source } :
> +            { url: this.url };
> +        query.line = originalLine;
> +        const scripts = this.dbg.findScripts(query);

I also prefer the way that Eddy rewrote this.
Attachment #8765568 - Flags: review?(nfitzgerald) → review+
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #9)
> I think that _restoreBreakpoints has been obsolete for a bit now.
> 
> Can you test the performance of getting the source list for pages with huge
> numbers of sources and scripts?
> http://lampwww.epfl.ch/~doeraene/presentations/scala-js-scaladays2013/#/ is
> a test case that I often used. IIRC, the ScriptStore provided a performance
> boost for such pages because Debugger#findScripts is by nature very slow.
> 
> If the ScriptStore does not provide a performance boost, then off with its
> head.

Also: maybe try and make your own synthetic test where there are tens of thousands of sources and scripts to test before/after this patch. A page that does something like this:

  for (let i = 0; i < 100000; i++) {
    eval(`function f${i}() { return ${i}; } //# sourceURL=${i}.js`);
  }

(Yes, there really are pages in the wild with tens of thousands of JSScripts!)
** RESULTS **

These were recorded with the following code inside `_discoverSources` which is called when the debugger requests a lists of all of the sources. For the ScriptStore case, the `scripts` object will be constructed by a getter at this point in time, which calls `findScripts`. The ScriptStore is slower because it's doing additional work to index all of the scripts within the store, whereas the other case uses `findScripts` directly.

    dump('JWL getting\n');
    // either `this.scripts.getAllScripts();` or `this.dbg.findScripts()`
    dump('JWL ' + (Date.now() - start).toString() + '\n');

All results are in milliseconds.

URL: http://lampwww.epfl.ch/~doeraene/presentations/scala-js-scaladays2013/#/
Test Case: _discoverSources
with ScriptStore: 378 + 170 + 407 + 171 + 163 = ~257
w/out ScriptStore: 162 + 99 + 87 + 243 + 90 + 133 = = ~135

The ScriptStore is a little slower because not only is it doing `findScripts()`, but it's indexing them as well. The question is if there is ever a time when we do subsequent calls to `_discoverSources`, where the ScriptStore will certainly win out. I don't think there is. We call it in `onSources` which is only called for the initial debugger opening.

Now let's test setting a breakpoint, where ScriptStore potentially is faster because it does not have to call `findScripts` at all. This was done with the following code in `_setBreakpointAtGeneratedLocation`:

    const start = Date.now();
    let scripts = this.dbg.findScripts(query);
    dump('JWL ' + (Date.now() - start).toString() + '\n');

and

    const start = Date.now();
    let scripts = this.scripts.getScriptsBySourceActorAndLine(
      generatedSourceActor,
      generatedLine
    );
    dump('JWL ' + (Date.now() - start).toString() + '\n');


URL: http://lampwww.epfl.ch/~doeraene/presentations/scala-js-scaladays2013/#/
Test Case: `setBreakpoint` on a source
with ScriptStore: 36 + 38 + 32 + 33 + 31 = ~34
w/out ScriptStore: 3 + 2 + 3 + 2 + 2 = ~2

This was a bit surprising to me, I don't know why the ScriptStore appears actually slower in this case...

Nick, I haven't tried your suggested test case of dynamically creating a ton of scripts, but given that your suggested URL has 81832 scripts it does seem like a decent test case. Is there any other situation you want me to test?
Flags: needinfo?(nfitzgerald)
Great results!

No, I don't think we need more testing -- as you point out, we've got some Real World results here and don't need to introduce artificial test cases into the mix when our Real World test case is already a bit of a stress test.

Looks like we can safely dead the ScriptStore! Thanks for investigating!
Flags: needinfo?(nfitzgerald)
Ok, great! Another thing I forgot to mention: now we don't have to call `findScripts` at all in `_addSource`. Previously it had to call it for scripts for a source every time, so reloading the page was probably a little slower too.

The store made sense given what we were trying to do before, but a lot has changed since then. I'll work on landing this soon.
None of those errors seem related to this, so I'll push this.
Pushed by jlong@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/f272791a6d75
remove ScriptStore in use findScripts in debugger r=fitzgen
https://hg.mozilla.org/mozilla-central/rev/f272791a6d75
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Depends on: 1302222
Depends on: 1302460
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.