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

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[(no longer active)]

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?

Comment 1

[Patrick McManus [: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..

Comment 2

[u408661]

Attachment: Bug 1348461 - Use a process-unique uint64 instead of a uuid for channel ids

Review commit: https://reviewboard.mozilla.org/r/124832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124832/

Comment 3

[Patrick McManus [:mcmanus]]

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 :)

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Ah, just found bug 1348462 already covers the RequestContextID part.

Comment 6

[u408661]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124832/diff/1-2/

Comment 7

[Patrick McManus [:mcmanus]]

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

Comment 8

[Mozilla Autoland]

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

[Mozilla Autoland]

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)

Comment 10

[u408661]

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).

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3df2617892b
Use a process-unique uint64 instead of a uuid for channel ids r=mcmanus

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c3df2617892b