RequestContextService's uuid generation also shows up in profiles

RESOLVED FIXED in Firefox 55

Status

()

Core
Networking
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: Ehsan, Assigned: nwgh)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

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)

Similar to bug 1348461.  Can this one be an integer or something also?
Flags: needinfo?(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..
Flags: needinfo?(mcmanus) → needinfo?(hurley)
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.
Assignee: nobody → hurley
Whiteboard: [necko-active]
We have the content child ID that is unique per content process, if needed.  Would that work?
Comment hidden (mozreview-request)
Flags: needinfo?(hurley)

Comment 5

3 months ago
mozreview-review
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?
Attachment #8852645 - Flags: review?(mcmanus)
Comment hidden (mozreview-request)

Comment 7

3 months ago
mozreview-review
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
Attachment #8852645 - Flags: review?(mcmanus) → review+

Comment 8

3 months ago
Pushed by hurley@mozilla.com:
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

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f469eec0a987
Status: NEW → RESOLVED
Last Resolved: 3 months 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.