1348462 - RequestContextService's uuid generation also shows up in profiles

Status: RESOLVED FIXED

(Core :: Networking, enhancement)

Description

[(no longer active)]

Similar to bug 1348461.  Can this one be an integer or something also?

Comment 1

[Patrick McManus [:mcmanus]]

ehsan - thanks so much for spending time with a profiler.

this one is nicks.. and I think an int (atomic if necessary) would work..

Comment 2

[u408661]

So with this (and 1348461) the concern was more about multiple processes in a multi-e10s world - much harder to synchronize between processes than threads :) (The IDs are created on the child, and communicated to the parent, so we don't want to accidentally clobber one child's ID with another's.)

That said, we could likely do some trickery with an uint64 where the high n bits are some per-process unique value, and the low (64-n) bits are a per-process serial.

Now that I type that, though, I vaguely remember (perhaps erroneously) some reason we couldn't do that. I'll have to go back to the original bug to try to refresh my memory. Leaving the ni? so I can circle back around once I do the archaeology.

Comment 3

[(no longer active)]

We have the content child ID that is unique per content process, if needed.  Would that work?

Comment 4

[u408661]

Attachment: Bug 1348462 - Use a process-unique uint64 instead of a uuid for request context ids

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

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 8852645 [details]
Bug 1348462 - Use a process-unique uint64 instead of a uuid for request context ids

https://reviewboard.mozilla.org/r/124828/#review127796

::: netwerk/base/RequestContextService.cpp:127
(Diff revision 1)
>    sSelf = this;
> +
> +  nsCOMPtr<nsIXULRuntime> runtime = do_GetService("@mozilla.org/xre/runtime;1");
> +  runtime->GetUniqueProcessID(&mRCIDNamespace);
> +  // Ensure this fits in 32 bits
> +  MOZ_RELEASE_ASSERT(mRCIDNamespace < (uint64_t(1) << 32));

I guess we will find out if the implementation starts using those high bits :)

but my guess is it does use them only in the cases where it is recycling process ids, which means its effectively the same as the 32 bit id when used our way.. so if you switch to the 32bit id we won't be courting danger :)

or maybe I misunderstand the recycling risk and how we avoid it after doing the shifting?

Comment 6

[u408661]

Comment on attachment 8852645 [details]
Bug 1348462 - Use a process-unique uint64 instead of a uuid for request context ids

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

Comment 7

[Patrick McManus [:mcmanus]]

Comment on attachment 8852645 [details]
Bug 1348462 - Use a process-unique uint64 instead of a uuid for request context ids

https://reviewboard.mozilla.org/r/124828/#review128706

Comment 8

[Pulsebot]

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

Comment 9

[Carsten Book [:Tomcat]]

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