Generating HttpChannelBase::mChannelId and serializing it to a string shows up in profiles

RESOLVED FIXED in Firefox 55

Status

()

Core
Networking: HTTP
RESOLVED FIXED
a month ago
21 days ago

People

(Reporter: Ehsan, Assigned: nwgh)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [necko-active])

MozReview Requests

()

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

Attachments

(1 attachment)

This is hard to demonstrate in the Gecko Profiler but easy to see in Instruments.  <http://searchfox.org/mozilla-central/rev/c2a60adfc7b16761cbbfcefa2093fa402ba1aa69/netwerk/protocol/http/nsHttpHandler.cpp#2461> shows up as spending quite a bit of time under profiles.  So does <http://searchfox.org/mozilla-central/rev/c2a60adfc7b16761cbbfcefa2093fa402ba1aa69/netwerk/protocol/http/HttpChannelChild.cpp#2251>.

Is there any reason why this has to be a UUID?  Can't we use something simpler and faster, like a uint64_t?
Flags: needinfo?(mcmanus)
this was added over last summer to support dev tools in e10s.

https://bugzilla.mozilla.org/show_bug.cgi?id=1274556

it looks like from the discussion the id started out as an int but was changed to a uuid over some concern about thread synchronization.. istm that channels are always created on the same thread so it should be fine to use an unlocked int64 for this as the original design did..
Blocks: 1274566
Flags: needinfo?(mcmanus)
Flags: needinfo?(jsnajdr)
Flags: needinfo?(hurley)
Assignee: nobody → hurley
Whiteboard: [necko-active]
Flags: needinfo?(jsnajdr)
Flags: needinfo?(hurley)
Comment hidden (mozreview-request)

Comment 3

27 days ago
mozreview-review
Comment on attachment 8852646 [details]
Bug 1348461 - Use a process-unique uint64 instead of a uuid for channel ids

https://reviewboard.mozilla.org/r/124832/#review127794

::: netwerk/protocol/http/nsHttpHandler.cpp:2461
(Diff revision 1)
>  
>  nsresult
> -nsHttpHandler::NewChannelId(nsID *channelId)
> +nsHttpHandler::NewChannelId(uint64_t& channelId)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> -  if (!mUUIDGen) {
> +  channelId = ((mProcessId << 32) & 0xFFFFFFFF00000000LL) | mNextChannelId++;

pretty sure you don't need to mask that after you shift it - but its fine if you like it. 

I'm more concerned that getuniqueprocessid is opaque and we're throwing away 32 bits of it.. it seems that some implementation of that might put all the interesting information in the 32 bits being discarded.

can we just use the 32 bit process ID provided by the same IDL?

or maybe make the key a uint96_t :)
Attachment #8852646 - Flags: review?(mcmanus)
Extending from @ehsan's profiling result, converting RequestContextID to string will also be a bottleneck.
http://searchfox.org/mozilla-central/rev/72fe012899a1b27d34838ab463ad1ae5b116d76b/netwerk/protocol/http/HttpChannelChild.cpp#2423

We should pass nsID in IPDL directly without converting it to string, since nsID is a serializable type and used in many places.
https://searchfox.org/mozilla-central/search?q=nsID&case=true&regexp=false&path=ipdl

Serializing nsID should be much faster than converting it to string + serializing the string. Parsing string back to nsID is heavy as well.

Here is the nsID serialization code:
https://searchfox.org/mozilla-central/rev/72fe012899a1b27d34838ab463ad1ae5b116d76b/ipc/glue/IPCMessageUtils.h#609
Ah, just found bug 1348462 already covers the RequestContextID part.
Comment hidden (mozreview-request)

Comment 7

23 days ago
mozreview-review
Comment on attachment 8852646 [details]
Bug 1348461 - Use a process-unique uint64 instead of a uuid for channel ids

https://reviewboard.mozilla.org/r/124832/#review128704
Attachment #8852646 - Flags: review?(mcmanus) → review+

Comment 8

23 days ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 499e7c7c7862 -d f469eec0a987: rebasing 386542:499e7c7c7862 "Bug 1348461 - Use a process-unique uint64 instead of a uuid for channel ids r=mcmanus" (tip)
merging netwerk/protocol/http/HttpBaseChannel.cpp
merging netwerk/protocol/http/HttpBaseChannel.h
merging netwerk/protocol/http/HttpChannelChild.cpp
merging netwerk/protocol/http/HttpChannelChild.h
merging netwerk/protocol/http/HttpChannelParent.cpp
merging netwerk/protocol/http/NullHttpChannel.cpp
merging netwerk/protocol/http/nsHttpChannel.cpp
merging netwerk/protocol/http/nsHttpChannel.h
merging netwerk/protocol/http/nsHttpHandler.cpp
merging netwerk/protocol/http/nsHttpHandler.h
merging netwerk/protocol/http/nsIHttpChannel.idl
merging netwerk/protocol/viewsource/nsViewSourceChannel.cpp
warning: conflicts while merging netwerk/protocol/http/nsHttpHandler.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 9

23 days ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 499e7c7c7862 -d c2363a9e7d2f: rebasing 386542:499e7c7c7862 "Bug 1348461 - Use a process-unique uint64 instead of a uuid for channel ids r=mcmanus"
merging netwerk/protocol/http/HttpBaseChannel.cpp
merging netwerk/protocol/http/HttpBaseChannel.h
merging netwerk/protocol/http/HttpChannelChild.cpp
merging netwerk/protocol/http/HttpChannelChild.h
merging netwerk/protocol/http/HttpChannelParent.cpp
merging netwerk/protocol/http/NullHttpChannel.cpp
merging netwerk/protocol/http/nsHttpChannel.cpp
merging netwerk/protocol/http/nsHttpChannel.h
merging netwerk/protocol/http/nsHttpHandler.cpp
merging netwerk/protocol/http/nsHttpHandler.h
merging netwerk/protocol/http/nsIHttpChannel.idl
merging netwerk/protocol/viewsource/nsViewSourceChannel.cpp
warning: conflicts while merging netwerk/protocol/http/nsHttpHandler.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Ugh. Must be a corner case in autoland - can't handle 2 commits from separate bugs that depend on each other, even though I attempted to land them in the correct order. This one will land after the first makes it to central (to make the rebase easier on my git-using self).
Flags: needinfo?(hurley)

Comment 11

22 days ago
Pushed by hurley@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3df2617892b
Use a process-unique uint64 instead of a uuid for channel ids r=mcmanus
Flags: needinfo?(hurley)

Comment 12

21 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c3df2617892b
Status: NEW → RESOLVED
Last Resolved: 21 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.