1410141 - Freezing all packets from LocalDebuggerTransport slow down the inspector

Status: NEW

(DevTools :: General, enhancement, P3)

Description

[Alexandre Poirot [:ochameau]]

If I comment this line:
http://searchfox.org/mozilla-central/source/devtools/shared/transport/transport.js#560
      this._deepFreeze(packet);

DAMP report a 6% win on complicated.inspector.open
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=9fcc71ec555dc5b5c6128ccfe7bd0cf73beaf4c8&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1&selectedTimeRange=172800

LocalDebuggerTransport is still used to connect to the main DebuggerServer instance. The one in the main process, the one with the RootActor instance.
We should mostly use the RootActor and the PreferenceActor.
All the other actors should be TabActors, going through connectToChild and using ChildDebuggerTransport.
But may be messages are still all piped throught LocalDebuggerTransport?

What is the most surprising is that DAMP report a win only against the inspector??

Comment 1

[Greg Tatum [:gregtatum]]

Freezing seems like unnecessary behavior in many cases. I dropped a dump() into the _deepFreeze method and it's freezing packets that are merely being shuffled from the content process to the main process. There is no reason to do that. Inspector is probably worse case with deeply nested dom node actors needing to be walked and frozen.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Right, the freezing first appeared when everything was still same process, pre-e10s.

Overall, there is probably not much point to freezing anymore, since it's largely IPC message passing anyway.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(We may also want to check _why_ the local transport is being hit for messages that cross processes... probably it happened out of convenience / by accident, instead of on purpose...)

Comment 4

[Alexandre Poirot [:ochameau]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> (We may also want to check _why_ the local transport is being hit for
> messages that cross processes... probably it happened out of convenience /
> by accident, instead of on purpose...)

Yes, that why I didn't just put a patch right away. It may be useful to do a *Transport classes map.

So, I imagine that's because of connectPipe:
http://searchfox.org/mozilla-central/source/devtools/server/main.js#683-685,705
    let serverTransport = new LocalDebuggerTransport();
    let clientTransport = new LocalDebuggerTransport(serverTransport);
    serverTransport.other = clientTransport;
    return clientTransport;

http://searchfox.org/mozilla-central/source/devtools/client/framework/target.js#417
  this._client = new DebuggerClient(DebuggerServer.connectPipe());

This client is a LocalDebuggerTransport.

ChildDebuggerTransport is only used within connectToChild:
http://searchfox.org/mozilla-central/source/devtools/server/main.js#1090-1097
  childTransport = new ChildDebuggerTransport(mm, prefix);
  childTransport.hooks = {
    onPacket: connection.send.bind(connection),
    onClosed() {}
  };
  childTransport.ready();
  connection.setForwarding(prefix, childTransport);

onPacket bound to connection.send is going to pipe the message back into LocalDebuggerTransport.


Having said that, that doesn't explain why inspector is more impacted by that than other tool.
At the end all the messages are being frozen.
Inspector has all these NodeActor's form messages, which are made of a couple of properties... is that it?
But there is also all these networkUpdate message, transmitted for everything panel. They don't have a lot of attributes, but there is a lot of them!

Comment 5

[Patrick Brosset <:pbro>]

Moving this inactive P2 to the backlog (P3)