1106695 - Hold strong references to all scripts to alleviate findScripts' GC-sensitivity + index scripts for faster querying

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: WIP

We (Jim, James, and I) talked about this on IRC the other day, and said we'd like to hack on it in PDX. I got an early start and hacked on it a bit over my thanksgiving vacation -- oops!

Description of ScriptStore copied from the patch:

"""
A `ScriptStore` is a cache of `Debugger.Script` instances. It indexes scripts
by `Debugger.Source`, URL, debuggee global, and by spanning line range for
quick queries. It holds strong references to the cached scripts to alleviate
the GC-sensitivity issues that plague `Debugger.prototype.findScripts`, but
this means that its lifetime must be managed carefully.
"""

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 0: Define require in toolkit/devtools/tests/unit/head_devtools.js. r=trivial

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Add toolkit/devtools/RedBlackTree.js. r=jimb

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Add toolkit/devtools/IntervalTree.js. r=jimb

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 3: Add toolkit/devtools/server/actors/utils/ScriptStore.js. r=jimb

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 4: Use ScriptStore in the script actors. WIP

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 4: Use ScriptStore in the script actors. r=jimb

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Note to self (and reviewer): need to also update max for rotated nodes in the interval tree.

Comment 8

[James Long (:jlongster)]

Comment on attachment 8531317 [details] [diff] [review]
Part 1: Add toolkit/devtools/RedBlackTree.js. r=jimb

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

Looks generally good. I got familiar enough with RB trees to understand it, but not enough to completely check the implementation. Seems to work as expected when I throw some data at it though. r+

::: toolkit/devtools/RedBlackTree.js
@@ +55,5 @@
> +   *
> +   * @param RedBlackTree.Node z
> +   */
> +  _insertNode(z) {
> +    let y = null;

What do you think about changing the `x` and `y` variables here to something a little more descriptive? At first I thought they indicated something in particular, but all you're doing here is traversing down the tree until you find a leaf. `z` is fine, but the x/y was a little confusing to me. What about `node` and `next` or for y & x or something?

@@ +91,5 @@
> +   *
> +   * @param RedBlackTree.Node z
> +   */
> +  _insertFixup(z) {
> +    while (z && z[parent] && z[parent][color] === red) {

Using symbols sure makes this a big more dense/hard to read. Are you sure it's worth it? I like symbols for things like protocols and stuff that need to apply across many objects, but for data structure-specific fields I'm not entirely convinced there's much win. I'm not going to r- because of it though.

@@ +224,5 @@
> +      return;
> +    }
> +    yield *this.inOrderWalk(node[left]);
> +    yield node;
> +    yield *this.inOrderWalk(node[right]);

Yay for generator-based iteration!

Comment 9

[James Long (:jlongster)]

Comment on attachment 8531318 [details] [diff] [review]
Part 2: Add toolkit/devtools/IntervalTree.js. r=jimb

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

This looks awesome!

Comment 10

[James Long (:jlongster)]

Comment on attachment 8531319 [details] [diff] [review]
Part 3: Add toolkit/devtools/server/actors/utils/ScriptStore.js. r=jimb

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

I'm impressed that this didn't take a whole lot of code (besides the RB tree stuff); I had the impression we'd have to do some crazy C++ shenanigans. This a much better approach! r+ with just a few comments

We should test a huge JS file with this just to prove the performance of the interval tree, and make sure it's not too costly to construct the data structure. I bet will be fine.

::: toolkit/devtools/DevToolsUtils.js
@@ +29,5 @@
> + * @param Map map
> + * @param any key
> + * @param Function makeDefault
> + */
> +exports.getOrCreate = function (map, key, makeDefault) {

Why is `makeDefault` a function, and not just let the user pass a value? (I guess it could be for optimization purposes, but you could check `typeof default === 'function'` and call it, otherwise just use the value)

@@ +41,5 @@
> +
> +/**
> + * No operation.
> + */
> +exports.noop = function noop() {};

Might be nice to generalize this into this:

function identity(x) { return x };

Should have the exact same semantics as `noop`, but also allows you to use it in places that need to take an argument (useful in certain functional situations)

::: toolkit/devtools/server/actors/utils/ScriptStore.js
@@ +25,5 @@
> +function ScriptStore(populate) {
> +  this._populate = populate;
> +
> +  // Set of every Debugger.Script in the cache.
> +  this._scripts = new Set();

Every property here is prefixed with an underscore. Can we just document that the public API are all the methods? It's pretty clear to me that that's the case, but removing underscores is just personal preference (and it may be the case that underscores follows current coding patterns)

@@ +201,5 @@
> +   * The first time this method is called for a given ScriptStore instance,
> +   * force population of the cache using the `populate` function provided at
> +   * construction time. Subsequent calls have no effect.
> +   */
> +  _ensurePopulated() {

Is it worth making this lazy? The ScriptStore will only be instantiated when the debugger is created, and will always need to use it immediately. I guess it doesn't really add much complexity making it lazy though.

Comment 11

[James Long (:jlongster)]

Comment on attachment 8531375 [details] [diff] [review]
Part 4: Use ScriptStore in the script actors. r=jimb

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

This last patch looks like it's still in progress

::: toolkit/devtools/server/actors/script.js
@@ +702,5 @@
>            + JSON.stringify(data, null, 2) + "\n");
>    },
>  
>    /**
> +   * TODO FITZGEN

Still in progress?

@@ +704,5 @@
>  
>    /**
> +   * TODO FITZGEN
> +   */
> +  _getAllScripts: function () {

May be worth renaming to make it clear that this is just for populating the script store initially

@@ +2119,5 @@
> +    // inside _addScript, we can accidentally set a breakpoint in a top level
> +    // script as a "closest match" because we wouldn't have added the child
> +    // scripts to the ScriptStore yet.
> +    this.scripts.addScript(aScript);
> +    this.scripts.addScripts(aScript.getChildScripts());

is `onNewScript` only called for top-level scripts?

@@ +3117,5 @@
> +      let i = 0;
> +      dumpn("FITZGEN: scripts =");
> +      for (let s of scripts) {
> +        dumpn("FITZGEN:   script " + i++);
> +        dumpn("FITZGEN:     " + s);

Still in progress?

Comment 12

[James Long (:jlongster)]

Whoops, just noticed that you hadn't assigned me to review the last patch yet!

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to James Long (:jlongster) from comment #8)
> ::: toolkit/devtools/RedBlackTree.js
> @@ +55,5 @@
> > +   *
> > +   * @param RedBlackTree.Node z
> > +   */
> > +  _insertNode(z) {
> > +    let y = null;
> 
> What do you think about changing the `x` and `y` variables here to something
> a little more descriptive? At first I thought they indicated something in
> particular, but all you're doing here is traversing down the tree until you
> find a leaf. `z` is fine, but the x/y was a little confusing to me. What
> about `node` and `next` or for y & x or something?

I was using the same variables used in CLRS, since I have the comment at the top
saying that this is based on CLRS and the ascii diagram for the rotation
stuff. Not worth it? Better to use more descriptive names in this case?

> @@ +91,5 @@
> > +   *
> > +   * @param RedBlackTree.Node z
> > +   */
> > +  _insertFixup(z) {
> > +    while (z && z[parent] && z[parent][color] === red) {
> 
> Using symbols sure makes this a big more dense/hard to read. Are you sure
> it's worth it? I like symbols for things like protocols and stuff that need
> to apply across many objects, but for data structure-specific fields I'm not
> entirely convinced there's much win. I'm not going to r- because of it
> though.

I think it is worth it whenever you expect sub-classing (as in the next
patch). I can't count the number of times I have accidentally used the same
`_commonPropertyName` as the super-class in my own sub-class by accident,
resulting in really weird, difficult to track down bugs.

(In reply to James Long (:jlongster) from comment #10)
> We should test a huge JS file with this just to prove the performance of the
> interval tree, and make sure it's not too costly to construct the data
> structure. I bet will be fine.

Definitely. On my TODO list.

> ::: toolkit/devtools/DevToolsUtils.js
> @@ +29,5 @@
> > + * @param Map map
> > + * @param any key
> > + * @param Function makeDefault
> > + */
> > +exports.getOrCreate = function (map, key, makeDefault) {
> 
> Why is `makeDefault` a function, and not just let the user pass a value? (I
> guess it could be for optimization purposes, but you could check `typeof
> default === 'function'` and call it, otherwise just use the value)

I don't want to have to instantiate the value if I'm not going to use it.

I've also grown to loath APIs with various signatures to do the "same" thing in
every way possible so you never have to think about it, but the reality tends to
be that it is more confusing than if you just only had one type signature. I
wouldn't r- a patch that did the `typeof defValue === "function"` stuff, but I
don't feel the need to add it just for the convenience of some theoretical
future consumer of the function.

> @@ +41,5 @@
> > +
> > +/**
> > + * No operation.
> > + */
> > +exports.noop = function noop() {};
> 
> Might be nice to generalize this into this:
> 
> function identity(x) { return x };
> 
> Should have the exact same semantics as `noop`, but also allows you to use
> it in places that need to take an argument (useful in certain functional
> situations)

Will wait till the time comes when we find it useful to do this.

> ::: toolkit/devtools/server/actors/utils/ScriptStore.js
> @@ +25,5 @@
> > +function ScriptStore(populate) {
> > +  this._populate = populate;
> > +
> > +  // Set of every Debugger.Script in the cache.
> > +  this._scripts = new Set();
> 
> Every property here is prefixed with an underscore. Can we just document
> that the public API are all the methods? It's pretty clear to me that that's
> the case, but removing underscores is just personal preference (and it may
> be the case that underscores follows current coding patterns)

Our coding style uses _ as a prefix to "private" properties really heavily,
don't think special casing this is worth it.

> @@ +201,5 @@
> > +   * The first time this method is called for a given ScriptStore instance,
> > +   * force population of the cache using the `populate` function provided at
> > +   * construction time. Subsequent calls have no effect.
> > +   */
> > +  _ensurePopulated() {
> 
> Is it worth making this lazy? The ScriptStore will only be instantiated when
> the debugger is created, and will always need to use it immediately. I guess
> it doesn't really add much complexity making it lazy though.

Yeah, perhaps not; will do the measurements and see.

(In reply to James Long (:jlongster) from comment #11)
> @@ +2119,5 @@
> > +    // inside _addScript, we can accidentally set a breakpoint in a top level
> > +    // script as a "closest match" because we wouldn't have added the child
> > +    // scripts to the ScriptStore yet.
> > +    this.scripts.addScript(aScript);
> > +    this.scripts.addScripts(aScript.getChildScripts());
> 
> is `onNewScript` only called for top-level scripts?

Yes.

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Add toolkit/devtools/server/actors/utils/ScriptStore.js. r=jlongster

Reality trumps theory and fun.

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Use ScriptStore in the script actors. r=jlongster

Comment 16

[James Long (:jlongster)]

Comment on attachment 8536858 [details] [diff] [review]
Part 1: Add toolkit/devtools/server/actors/utils/ScriptStore.js. r=jlongster

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

Looks good. It's very nice to have this in JS. Should we file a follow up bug to remove the C++ code in `findScripts` that we don't need anymore? We basically just need a function to get all the scripts in the debugger now, without any of the querying.

::: toolkit/devtools/server/actors/utils/ScriptStore.js
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +const { noop } = require("devtools/toolkit/DevToolsUtils");

I don't see this used anywhere in the code now.

Comment 17

[James Long (:jlongster)]

Comment on attachment 8536860 [details] [diff] [review]
Part 2: Use ScriptStore in the script actors. r=jlongster

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

Nice!

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/232c05049237
remote:   https://hg.mozilla.org/integration/fx-team/rev/30226ebe3c5e

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

James, the ScriptStore.prototype.getAllSources should be useful for bug 1111058. Would be interested in seeing if it speeds up the RDP "sources" request.

Jeff, can you watch this[0] over the next couple days and see what the difference is. I'd expect it to drop on avg, but I'm feeling rather paranoid!

[0] http://telemetry.mozilla.org/#filter=nightly%2F37%2FDEVTOOLS_DEBUGGER_RDP_LOCAL_SOURCES_MS&aggregates=multiselect-all!Submissions!Mean!5th%20percentile!25th%20percentile!median!75th%20percentile!95th%20percentile&evoOver=Builds&locked=true&sanitize=true&renderhistogram=Graph

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/232c05049237
https://hg.mozilla.org/mozilla-central/rev/30226ebe3c5e

Comment 21

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Nick Fitzgerald [:fitzgen] from comment #19)
> James, the ScriptStore.prototype.getAllSources should be useful for bug
> 1111058. Would be interested in seeing if it speeds up the RDP "sources"
> request.
> 
> Jeff, can you watch this[0] over the next couple days and see what the
> difference is. I'd expect it to drop on avg, but I'm feeling rather paranoid!
> 
> [0]
> http://telemetry.mozilla.org/
> #filter=nightly%2F37%2FDEVTOOLS_DEBUGGER_RDP_LOCAL_SOURCES_MS&aggregates=mult
> iselect-all!Submissions!Mean!5th%20percentile!25th%20percentile!median!
> 75th%20percentile!
> 95th%20percentile&evoOver=Builds&locked=true&sanitize=true&renderhistogram=Gr
> aph

My guess at interpreting that graph is that over the weekend 2 things happened:

1. submissions went way down as you would expect over the weekend prior to christmas holidays
2. the 95th percentile of the histogram spiked hard, so for lots of people this is taking 600ms ( but not that many people because it's the holidays )

It kinda looks like we should back this out before we forget to do it later. James?

Comment 22

[Jeff Griffiths (:canuckistani) (:⚡︎)]

the re-need-info so I don't forget this.

Comment 23

[James Long (:jlongster)]

(In reply to Jeff Griffiths (:canuckistani) from comment #21)
> 1. submissions went way down as you would expect over the weekend prior to
> christmas holidays
> 2. the 95th percentile of the histogram spiked hard, so for lots of people
> this is taking 600ms ( but not that many people because it's the holidays )
> 
> It kinda looks like we should back this out before we forget to do it later.
> James?

Those graphs confuse me. It spikes on the 18th, I think, but this landed on the 19th, and on the 19th the spike isn't there anymore. Maybe I just don't know how to read the graphs? I also can't figure out how to change the graph to show up to today; fitzgen's link just shows up to the 20th.

Comment 24

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Yeah, default telemetry graphs are a bit opaque. I think you're right though - if the build people used on the 18th didn't have his patch in, then there is nothing to do here ( yet ) - let's let a few more days of data come in.

Comment 25

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Update, after a few days the graph looks wonky and the submission rate is way down as one would expect over christmas. I don't think you can draw definitive conclusions from this measure right now, the data just isn't that good. Curious to know what fitzgen thinks once he gets back on the job.

Comment 26

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I think this should stick:

                | w/out patch | w/ patch
----------------+-------------+---------
5th Percentile  |   19        |  17
25th Percentile |   52        |  44
Median          |  103        |  95
75th Percentile |  228        | 200
95th Percentile | 1020        | 711

(All measurements are milliseconds.)

The change in the 95th percentile seems like a pretty big impact, even if there wasn't much of an effect elsewhere.

Comment 27

[James Long (:jlongster)]

Awesome!