Queuing requests is slower than sending them one by one via DebuggerClient.request

RESOLVED FIXED in Firefox 38

Status

DevTools
Framework
RESOLVED FIXED
4 years ago
a month ago

People

(Reporter: ochameau, Assigned: jryans)

Tracking

unspecified
Firefox 38

Firefox Tracking Flags

(firefox38 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
It is really not clear why/how it happens to be slower but, it happens to be faster to wait for a request to finish, before piping another one via Debuggerclient APIs.
It's not clear mostly because DebuggerClient and DebuggerTransport are already queuing requests in order to send/receive them one by one!!

I identified this slowness while working on bug 1123788, where fetching icons one by one is faster than piling up requests.

Here is a very simple xpcshell test exposing this slowness:
const run_test = Test(function*() {
  initTestDebuggerServer();
  const connection = DebuggerServer.connectPipe();
  const client = Async(new DebuggerClient(connection));

  yield client.connect();

  let N = 100;

  let a = new Date().getTime();
  for(var i = 0; i < N; i++) {
    let response = yield client.request({
      to: "root",
      type: "echo"
    });
  }
  let s = "Sequential: "+(new Date().getTime()-a)+"\n";
  dump(s);

  let promises = []
  for(var i = 0; i < N; i++) {
    promises.push(client.request({
      to: "root",
      type: "echo"
    }));
  }
  yield promise.all(promises)
         .then(() => {
          dump(s + "Parallel: "+(new Date().getTime()-a)+"\n");
         });

  yield client.close();
});

"Sequential: 170"
"Parallel: 345"


Now, I have no idea where it ends up being slower...
(Assignee)

Updated

4 years ago
Assignee: nobody → jryans
Status: NEW → ASSIGNED
(Assignee)

Comment 1

4 years ago
There's a bug in your test above.  You forgot to reset the start time after the "Sequential" run.

Regardless, the parallel version is still slower, especially as N gets much larger.

I have fix for this, attaching soon.
(Assignee)

Comment 2

4 years ago
Created attachment 8556738 [details]
MozReview Request: bz://1127435/jryans

/r/3139 - Bug 1127435 - Use separate client queues per actor. r=ochameau

Pull down this commit:

hg pull review -r fc46ca9973426e5e1398dc60e5b50046c0bb58d1
Attachment #8556738 - Flags: review?(poirot.alex)
(Assignee)

Comment 3

4 years ago
We were iterating one giant queue of all requests:

* For each new request
* After each in flight request completed

That's quite silly, since we really only queue per actor, not globally.

The parallel case is now faster (as it should be) for all values of N.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c27c9970eab
(Assignee)

Comment 4

4 years ago
Comment on attachment 8556738 [details]
MozReview Request: bz://1127435/jryans

This version is not quite right...  The debugger seems to rely on the implicit total order of requests.
Attachment #8556738 - Flags: review?(poirot.alex)
(Assignee)

Comment 5

4 years ago
Comment on attachment 8556738 [details]
MozReview Request: bz://1127435/jryans

/r/3139 - Bug 1127435 - Use separate client queues per actor. r=ochameau

Pull down this commit:

hg pull review -r d8f2c51cb8a58fd73c8d913546edee5903a6885d
Attachment #8556738 - Flags: review?(poirot.alex)
(Reporter)

Comment 7

4 years ago
https://reviewboard.mozilla.org/r/3139/#review2691

Looks good, but I'm wondering if we have an issue with protocol.js actors and Client classes.
Looking at the existing code, and the code with this patch, it seems like we have issues when we start queuing requests for these actors/clients.
I'm not sure this patch makes things worse or just alike, could you check this before landing?

::: toolkit/devtools/client/dbg-client.jsm
(Diff revision 2)
> -  _sendRequests: function () {
> +  _sendOrQueueRequest(request) {

What?\!
Yet another JS syntax :-o

::: toolkit/devtools/client/dbg-client.jsm
(Diff revision 2)
>      }

I'm wondering how do things work when we have a protocol.js or a Client class in this.\_clients (where we return early)?
It looks like we are not calling sendRequest/attemptNextRequest in these cases...
(Reporter)

Updated

4 years ago
Attachment #8556738 - Flags: review?(poirot.alex) → review+
(Assignee)

Comment 8

4 years ago
https://reviewboard.mozilla.org/r/3139/#review2791

I agree this code is hard to follow, so I can understand why you would be worried!

The changes here only impact requests that use dbg-client's own queueing system by calling it's |send| method.

protocol.js fronts don't use[1] this, they call the transport directly.

For the clients block, this only concerns clients using the "generic actor event" support (that you added! :P).  This is to handle one-off events *from* the actor, so there would not be any requests queued in the client.

[1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/protocol.js#1146
Comment hidden (typo)
(Assignee)

Updated

4 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
https://hg.mozilla.org/mozilla-central/rev/54414d565677
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Comment on attachment 8556738 [details]
MozReview Request: bz://1127435/jryans
Attachment #8556738 - Attachment is obsolete: true
Attachment #8619241 - Flags: review+
Created attachment 8619241 [details]
MozReview Request: Bug 1127435 - Use separate client queues per actor. r=ochameau

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.