1397341 - Try loading React in the parent process while debugger server and tab ator are created in the content process

Status: NEW

(DevTools :: Framework, enhancement, P3)

Description

[Alexandre Poirot [:ochameau]]

Loading react.js is a significant step in the overall toolbox load.
For now, we load it here, implicitely, by accessing React getter:
http://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox.js#1106

And we do it only *after*:
* the target is remoted
  yield this._target.makeRemote();
* the thread is attached
  this._threadClient = yield attachThread(this);
* the toolbox document is ready:
  yield domReady.promise;

http://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox.js#419-432

So it happens pretty late and there is a clear opportunity to make it load during the call to target.makeRemote, which triggers a lot of computation on the content process. This computation is mostly about:
* initializing the debugger server
* load tab actor
* attach the tab actor
No matter how we optimize this part in the content process, it should take more time than loading React in the parent.

Compared to bug 1393086, this is a super focused parallelization, which should be much safer to land.

Comment 1

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba4c976ba1bf943dde6cc50cfcdb8a9af09e8af6

Comment 2

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=09ff8f40ecfd913032338e48f35bfd18a1fa4763

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1397341 - Try loading React while debugger server initialize. ?

Review commit: https://reviewboard.mozilla.org/r/177286/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/177286/

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8905476 [details]
Bug 1397341 - Try loading React while debugger server initialize. ?

https://reviewboard.mozilla.org/r/177286/#review182298

::: devtools/client/framework/toolbox.js:430
(Diff revision 1)
> +              Services.tm.dispatchToMainThread({
> +                run: () => {
> +                  Services.tm.dispatchToMainThread({
> +                    run: () => {
> +                      this.React;
> +                    }

So... I need 4 calls to dispatchToMainThread to have it called while `makeRemote` computes in the content process. (this is equivalent to DevToolsUtils.executeSoon)

It looks like it isn't that easy to ensure code runs in parallel...

But with this patch we clearly see react.js is loaded while debugger server and tab actor is loaded in the content process!

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8905476 [details]
Bug 1397341 - Try loading React while debugger server initialize. ?

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177286/diff/1-2/

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8905476 [details]
Bug 1397341 - Try loading React while debugger server initialize. ?

Tom, I have some doubts about all these "paralellization" patches.

It seems hard to ensure to run code that is processed at the same time in both processes. I think here, the client/transport code isn't necessarely synchronous. So in order to make it right, you have to go down and get a precise event of exactly when we hand over to the client process.
Here, I would like to load React while the getTab request is being processed (and requires significant computation in content process).

The current patch introduce yet another promise for all request, which doesn't look reasonable. But I was able to reduce the number of dispatchToMainThread and improve parallelization. Note that it doesn't always work, even with this...

Comment 7

[Alexandre Poirot [:ochameau]]

DAMP reports between 2 to 6% win on tools opening:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=fc434e3cc56755879de7f0b10cf68b7a6c84e707&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyImportant=1&showOnlyConfident=1&framework=1&selectedTimeRange=172800
But to be clear, I still have limited trust against DAMP results...
It used to report no win when there is some and also reports win when there is none :/

Comment 8

[Tom Tromey :tromey]

Comment on attachment 8905476 [details]
Bug 1397341 - Try loading React while debugger server initialize. ?

https://reviewboard.mozilla.org/r/177286/#review184000

Thank you for the patch.

I think this kind of thing is worth doing.  There are two main issues to my mind.

One is that it makes the code more subtle.  This, I think, can usually be counteracted by comments explaining the intent of the code, to help future developers try to know what is required about it and what is happenstance.

The second is that due to its obscurity, it seems relatively likely to regress.  But that, I suppose, must be addressed elsewhere, as you discussed at the meeting today.

So on the whole I am in favor.

::: devtools/client/framework/target.js:397
(Diff revision 2)
>    /**
>     * Adds remote protocol capabilities to the target, so that it can be used
>     * for tools that support the Remote Debugging Protocol even for local
>     * connections.
>     */
> -  makeRemote: async function () {
> +  makeRemote: async function (go) {

If I understand correctly, what's happening here is that this code starts some tasks running on the server, and then in the middle of that, provides a callback so that client code can be started in parallel.

Continuing to hopefully understand, we don't want to unconditionally start these in parallel because the connection may fail.  So we want to be careful about when to start the client-side work.

Anyway, if that's true, it seems subtle enough that there should be some jsdoc describing the "go" parameter; and also a comment in the body explaining why this is done, even linking back to this bug.

::: devtools/client/framework/toolbox.js:420
(Diff revision 2)
>  
>        // Optimization: fire up a few other things before waiting on
>        // the iframe being ready (makes startup faster)
>  
>        // Load the toolbox-level actor fronts and utilities now
> -      yield this._target.makeRemote();
> +      let onRemote = this._target.makeRemote(() => {

I don't think the variable is needed here.

::: devtools/shared/client/main.js:1318
(Diff revision 2)
>  
>  class Request extends EventEmitter {
>    constructor(request) {
>      super();
>      this.request = request;
> +    this.sent = new Promise(done => {

It seems maybe mildly strange that this class already extends EventEmitter but then also has a separate promise-based API.  Is there a reason not to use `once`?  Not an issue for me if you'd rather not.

Comment 9

[Tom Tromey :tromey]

(In reply to Alexandre Poirot [:ochameau] from comment #6)

> Here, I would like to load React while the getTab request is being processed
> (and requires significant computation in content process).

Well it seems like perhaps I didn't fully understand after all.
Sorry I didn't read this more closely the first N times.
I'm not sure this affects the conclusions though.

Comment 10

[Alexandre Poirot [:ochameau]]

(In reply to Tom Tromey :tromey from comment #8)
> If I understand correctly, what's happening here is that this code starts
> some tasks running on the server, and then in the middle of that, provides a
> callback so that client code can be started in parallel.
> 
> Continuing to hopefully understand, we don't want to unconditionally start
> these in parallel because the connection may fail.  So we want to be careful
> about when to start the client-side work.

This is where I try to hook getTab RDP request.
I have to dig into makeRemote to be able to trigger something in the parent just after asking the content to process the getTab request.

But calling go() just after getTab() call isn't enough, I have to also dig into Transport/Request to know exactly when we just sent the request to the content process. But it is still not enough, I have to queue on the event loop once again (dispatchToMainThread).
And unfortunately, it doesn't seem to be enough to ensure it always works.

If can fail for 3 reasons:
* React getter is accessed before getTab RDP request reach the content process, in which case, loading React delay the request. We win nothing.
* React getter is access during the getTab is processed, we succeed.
* React getter is access between getTab response and some other steps of makeRemote and delay makeRemote. We win nothing.
* React getter is accessed after makeRemote, we delay things again and win nothing again.

So it is still pretty weak, even by adding a serious amount of complexity to makeRemote and Request.
I'm still not convinced it is worth it.
I would be more confident if we find a reliable way to run code in parallel between processes, which isn't the case of this patch.

> 
> ::: devtools/shared/client/main.js:1318
> (Diff revision 2)
> >  
> >  class Request extends EventEmitter {
> >    constructor(request) {
> >      super();
> >      this.request = request;
> > +    this.sent = new Promise(done => {
> 
> It seems maybe mildly strange that this class already extends EventEmitter
> but then also has a separate promise-based API.  Is there a reason not to
> use `once`?  Not an issue for me if you'd rather not.

Oh, yes, I didn't realized it was an EventEmitter...

Comment 11

[Tom Tromey :tromey]

Comment on attachment 8905476 [details]
Bug 1397341 - Try loading React while debugger server initialize. ?

Clearing the f? since I was reading in mozreview and gave it an r+ there.