Open Bug 1410141 Opened 2 years ago Updated 7 months ago

Freezing all packets from LocalDebuggerTransport slow down the inspector

Categories

(DevTools :: General, enhancement, P3)

enhancement

Tracking

(Not tracked)

People

(Reporter: ochameau, Unassigned)

References

(Blocks 1 open bug)

Details

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??
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.
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.
(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...)
(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!
Priority: -- → P2
Product: Firefox → DevTools
Moving this inactive P2 to the backlog (P3)
Priority: P2 → P3
You need to log in before you can comment on or make changes to this bug.