Closed Bug 790202 Opened 8 years ago Closed 8 years ago

Protocol layer performance overhead is too much for local transports

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 18

People

(Reporter: msucan, Assigned: past)

References

Details

Attachments

(1 file, 2 obsolete files)

The Web Console is transitioning to the remote debugging protocol (bug 768096) and there's a strong performance regression.

Test case: load attachment 593077 [details] from bug 722685. Change the loop to 25000 or more and compare with the patches from bug 768096, and without them.

Profiling information with the Web Console remoting patches:

http://people.mozilla.com/~bgirard/cleopatra/?report=662a5b5b525b79eba6e9a3e81a5a81ce98167363

Without the remoting patches:

http://people.mozilla.com/~bgirard/cleopatra/?report=8b638c0e76ac07af6bbc98319ef0456d91be4adb

One can notice that DT_onOutputStreamReady() from dbg-transport.js takes the majority of execution time.

We probably need to avoid the input/output streams in local mode, just fake the transport of packets.
Assignee: nobody → past
Status: NEW → ASSIGNED
Priority: -- → P2
Summary: Protocol layer performance overhead is too much in some cases → Protocol layer performance overhead is too much for local transports
Attached patch Patch v1 (obsolete) — Splinter Review
This patch adds a "fake" transport that simply calls the onPacket handler of the other endpoint. No XPCOM boundary crossing, no copy to an nsIPipe buffer, no JSON.stringify & parse, no convertToUnicode, nada. All tests pass locally and I'll do a try run next, after we get some perf measurements.

I'll probably move this transport to dbg-transport.js as well, and I wonder if it makes better sense to keep the previous connectPipe and just change all call sites to a connectLocal.
Attachment #661223 - Flags: feedback?(mihai.sucan)
Attachment #661223 - Flags: feedback?(dcamp)
It is kind of scary to me that both the client and the server have a reference to the same packet and can both modify it. I know that the code shouldn't be doing that anywhere, but it still kind of scares me. Would it be too much overhead to just do a quick deep copy of the packet before we call the onPacket hook?
Some quick benchmarking with the test case mentioned in comment 0 shows that the remote protocol overhead should be negligible:

https://docs.google.com/spreadsheet/pub?key=0As9genFGqpMNdFA0LWFCOUxvdXBZRVA2RGtwZ3RhZmc&output=html

I'll wait for Mihai's own tests to be sure.

(In reply to Nick Fitzgerald [:fitzgen] from comment #2)
> It is kind of scary to me that both the client and the server have a
> reference to the same packet and can both modify it. I know that the code
> shouldn't be doing that anywhere, but it still kind of scares me. Would it
> be too much overhead to just do a quick deep copy of the packet before we
> call the onPacket hook?

To be honest I'm rather comfortable with this and I'd prefer it if we could somehow write tests that verify packet immutability. That said, it will be trivial to add back a JSON.parse(JSON.stringify(packet)) as a cloning mechanism, if Mihai can confirm that our performance is still acceptable.
We could also just call Object.freeze(packet)
(In reply to Nick Fitzgerald [:fitzgen] from comment #4)
> We could also just call Object.freeze(packet)

I was thinking about proxies, but this is so much better!
(In reply to Nick Fitzgerald [:fitzgen] from comment #4)
> We could also just call Object.freeze(packet)

<3
I wonder how much that slows down property access, but I bet it's negligible.
(In reply to Victor Porof [:vp] from comment #6)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #4)
> > We could also just call Object.freeze(packet)
> 
> <3
> I wonder how much that slows down property access, but I bet it's negligible.

I'm not seeing any significant difference in property access: http://jsperf.com/freeze-vs-seal-vs-normal/3
Status: ASSIGNED → NEW
Priority: P2 → --
Comment on attachment 661223 [details] [diff] [review]
Patch v1

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

This is now much faster. Thank you Panos!

I also suggest cloning the packet or Object.freeze(), to avoid potential issues.
Attachment #661223 - Flags: feedback?(mihai.sucan) → feedback+
Status: NEW → ASSIGNED
Priority: -- → P2
Attached patch Patch v2 (obsolete) — Splinter Review
Here is the updated version which freezes the packet before transport and with LocalDebuggerTransport moved into dbg-transport.js. I decided not to keep a separate connectPipe since I couldn't think of any use for it, but I could be convinced otherwise. Try run:

https://tbpl.mozilla.org/?tree=Try&rev=7fe6eb835f6a
Attachment #661223 - Attachment is obsolete: true
Attachment #661223 - Flags: feedback?(dcamp)
Attachment #662443 - Flags: review?(rcampbell)
Attachment #662443 - Flags: feedback?
Comment on attachment 662443 [details] [diff] [review]
Patch v2

you have a blank requestee field in your feedback request. Who were you looking for feedback from?

in +LocalDebuggerTransport.prototype = {
LDT_send()...

+      this._deepFreeze(aPacket);

why do we need to do this? Is the other transport going to modify this packet object?

+      let thr = Cc["@mozilla.org/thread-manager;1"].getService().currentThread;

Use your words, Panos. let thread = ...

+      // Remove the reference to the other endpoint before calling close(), to
+      // avoid infinite recursion.

aw.

in _deepFreeze:

+      if (aObject.hasOwnProperty(prop) && typeof aObject === "object" &&
+          !Object.isFrozen(aObject)) {
+        deepFreeze(o[prop]);
+      }

is deepFreeze() a thing? should that be this._deepFreeze(o[prop])?

connectPipe looks good.
(In reply to Rob Campbell [:rc] (:robcee) from comment #10)
> Comment on attachment 662443 [details] [diff] [review]
> Patch v2
> 
> you have a blank requestee field in your feedback request. Who were you
> looking for feedback from?

Duh, probably dcamp, but I'm not sure if he has the cycles.

> in +LocalDebuggerTransport.prototype = {
> LDT_send()...
> 
> +      this._deepFreeze(aPacket);
> 
> why do we need to do this? Is the other transport going to modify this
> packet object?

See comment 2 and comment 8. The kids nowadays are more easily scared than us old-timers :-)

More seriously though, the immutability guarantees imposed by a socket or a pipe no longer hold with this patch. It is not inconceivable that we might introduce some weird bugs in the future, or maybe less careful add-on writers will. I expect we should catch such bugs quickly due to the testing we will get from B2G & Fennec, but freezing the objects seems like a cheap solution to avoid worrying about it altogether.

> +      let thr =
> Cc["@mozilla.org/thread-manager;1"].getService().currentThread;
> 
> Use your words, Panos. let thread = ...

Fine, as long as Dave doesn't hold it against me.

> in _deepFreeze:
> 
> +      if (aObject.hasOwnProperty(prop) && typeof aObject === "object" &&
> +          !Object.isFrozen(aObject)) {
> +        deepFreeze(o[prop]);
> +      }
> 
> is deepFreeze() a thing? should that be this._deepFreeze(o[prop])?

Ugh, refactoring typo! Will fix.
(In reply to Panos Astithas [:past] from comment #11)
> (In reply to Rob Campbell [:rc] (:robcee) from comment #10)
> > Comment on attachment 662443 [details] [diff] [review]
> > Patch v2
...
> > in +LocalDebuggerTransport.prototype = {
> > LDT_send()...
> > 
> > +      this._deepFreeze(aPacket);
> > 
> > why do we need to do this? Is the other transport going to modify this
> > packet object?
> 
> See comment 2 and comment 8. The kids nowadays are more easily scared than
> us old-timers :-)

Our greater wisdom should trump their fears. ;)

> More seriously though, the immutability guarantees imposed by a socket or a
> pipe no longer hold with this patch. It is not inconceivable that we might
> introduce some weird bugs in the future, or maybe less careful add-on
> writers will. I expect we should catch such bugs quickly due to the testing
> we will get from B2G & Fennec, but freezing the objects seems like a cheap
> solution to avoid worrying about it altogether.

Cheap enough, I suppose. Feels a little bit like pre-emptive engineering, but whatevs.
Attached patch Patch v3Splinter Review
Addressed review comments.
Attachment #662443 - Attachment is obsolete: true
Attachment #662443 - Flags: review?(rcampbell)
Attachment #662443 - Flags: feedback?
Attachment #663309 - Flags: review?(rcampbell)
Comment on attachment 663309 [details] [diff] [review]
Patch v3

this looks like it'll do it!
Attachment #663309 - Flags: review?(rcampbell) → review+
https://hg.mozilla.org/mozilla-central/rev/4900ae292315
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 18
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.