Closed Bug 1477943 Opened 6 years ago Closed 6 years ago

Add a unique id per counter

Categories

(Toolkit :: Performance Monitoring, enhancement)

Product:
Toolkit
Component:
Performance Monitoring
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: tarek, Assigned: tarek)

References

Details

Attachments

(1 file)

Bug 1477943 - Add a unique id per PerformanceCounter instance -
59 bytes, text/x-review-board-request
froydnj
: review+
baku
: review+
Details 
In order to simply counter tracking work, let's add a unique ID per performance counter instance and return it in PerformanceInfo
Comment on attachment 8994784 [details]
Bug 1477943 - Add a unique id per PerformanceCounter instance -

https://reviewboard.mozilla.org/r/259318/#review266314

I'm not convinced this is a good idea.

::: commit-message-dd386:3
(Diff revision 2)
> +This new id is added in the PerformanceInfo data and helps consumers distinguish
> +counters. The id generator is based on the same pattern than the window IDs.

I think this is the crux of the bug here: why would consumers want to do this?  There's no rationale provided for this in the bug, and no linked bugs that would suggest a need for this.

::: xpcom/threads/PerformanceCounter.h:119
(Diff revision 2)
> +   * Ids are unique across processes and non recyclables,
> +   * so when a Performance Counter instance is created
> +   * we can identify it uniquely throughout its lifetime.

Unique across processes is a pretty strong guarantee from this layer.  I think if you really need this kind of guarantee (why would you?), it'd be better for ids to be unique within the process, and then have some higher-level aggregation thing take care of sorting counters out between processes.

::: xpcom/threads/PerformanceCounter.cpp:24
(Diff revision 2)
> +// Inspired from the window id generator
> +static uint64_t gNextCounterID = 0;
> +static StaticMutex gMutex;

Just make `gNextCounterID` an `Atomic<uint64_t>`, and then there's no need for the mutex.

::: xpcom/threads/PerformanceCounter.cpp:49
(Diff revision 2)
> +  uint64_t processBits = processID & ((uint64_t(1) << kCounterIDProcessBits) - 1);
> +
> +  mozilla::StaticMutexAutoLock lock(gMutex);
> +  uint64_t counterID = ++gNextCounterID;
> +
> +  MOZ_RELEASE_ASSERT(counterID < (uint64_t(1) << kCounterIDWindowBits));

It doesn't seem that it's a good idea to make this `MOZ_RELEASE_ASSERT`, especially since `kCounterIDWindowBits` is only 31.
Attachment #8994784 - Flags: review?(nfroyd)
> I think this is the crux of the bug here: why would consumers want to do this?  
> There's no rationale provided for this in the bug, and no linked bugs that would suggest a need for this.

when about:performance is displaying the counter, we might have situations where a counter for a given
window id and/or host is destroyed and another one for the same window id host is created in the same second.

In that case, the UI might get a value that is smaller than the previous value since a counter starts at 0 when created.

Since the page is based on diffs, we might get negative values for a second on a given tab or worker.

Adding an id prevents this issue since the UI will be aware its not the same counter anymore.
Flags: needinfo?(nfroyd)
Flags: needinfo?(florian)
Flags: needinfo?(florian)
Comment on attachment 8994784 [details]
Bug 1477943 - Add a unique id per PerformanceCounter instance -

https://reviewboard.mozilla.org/r/259318/#review266342

::: dom/tests/browser/browser_test_performance_metrics.js:78
(Diff revisions 3 - 4)
> -        if (!counterIds.includes(entry.counterId)) {
> -          counterIds.push(entry.counterId);
> +        if (!counterIds.includes(entry.pid + entry.counterId)) {
> +          counterIds.push(entry.pid + entry.counterId);

This is not a good check, because depending on the pids assigned, you can have collisions between distinct entries.
Attachment #8994784 - Flags: review?(nfroyd)
(In reply to Tarek Ziadé (:tarek) from comment #5)
> > I think this is the crux of the bug here: why would consumers want to do this?  
> > There's no rationale provided for this in the bug, and no linked bugs that would suggest a need for this.
> 
> when about:performance is displaying the counter, we might have situations
> where a counter for a given
> window id and/or host is destroyed and another one for the same window id
> host is created in the same second.
> 
> In that case, the UI might get a value that is smaller than the previous
> value since a counter starts at 0 when created.
> 
> Since the page is based on diffs, we might get negative values for a second
> on a given tab or worker.
> 
> Adding an id prevents this issue since the UI will be aware its not the same
> counter anymore.

A couple of thoughts: I'm not sure whether the logic for finding the outer top window in DocGroup::ReportPerformanceInfo is really what we want...the loop says that we're trying to find the top window, but we have a `isTopLevel` variable that indicates that we might *not* have found the top window?  That seems weird.

I'm not completely sure that the outer window ID is the thing that PerformanceInfos should be instantiated with, too.

The scenario you're describing sounds like it only comes about if the original DocGroup was destroyed and a new (identical?) one was created...so maybe the right solution is to have the ID at the DocGroup (and individual Worker) level?

My DOM knowledge is hand-wavy at best, so maybe all of the above suggestions are nonsense. :)
Flags: needinfo?(nfroyd)
> the right solution is to have the ID at the DocGroup (and individual Worker) level

That's what we end up having since there's a single PerformanceCounter per DocGroup and Worker. So the ID is effectively at the DocGroup & Worker level.
Comment on attachment 8994784 [details]
Bug 1477943 - Add a unique id per PerformanceCounter instance -

https://reviewboard.mozilla.org/r/259318/#review266416

I need to know if PerformanceCounter must have a unique ID per process. If yes, this approach doesn't work.
See: https://searchfox.org/mozilla-central/rev/bdfd20ef30d521b57d5b6feeda71325e8b4cad66/dom/ipc/ContentChild.cpp#3306-3325

::: xpcom/threads/PerformanceCounter.h:119
(Diff revision 5)
>    uint64_t GetTotalDispatchCount();
>  
> +  /**
> +   * Returns the unique id for the instance.
> +   */
> +  uint64_t GetID();

uint64_t GetID() const

::: xpcom/threads/PerformanceCounter.h:128
(Diff revision 5)
>  
>    Atomic<uint64_t> mExecutionDuration;
>    Atomic<uint64_t> mTotalDispatchCount;
>    DispatchCounter mDispatchCounter;
>    nsCString mName;
> +  uint64_t mID;

const uint64_t mID;

::: xpcom/threads/PerformanceCounter.cpp:20
(Diff revision 5)
>  #define LOG(args) MOZ_LOG(sPerformanceCounter, mozilla::LogLevel::Debug, args)
>  
> +// global counter used to uniquely identify a window
> +static Atomic<uint64_t> gNextCounterID(0);
> +
> +uint64_t

static

::: xpcom/threads/PerformanceCounter.cpp:23
(Diff revision 5)
> +static Atomic<uint64_t> gNextCounterID(0);
> +
> +uint64_t
> +NextCounterID()
> +{
> +  return ++gNextCounterID;

This would not work cross processes. Do we have PerformanceCounter for each process? if yes, we need a better approach. See how WindowIDs are generated.

::: xpcom/threads/PerformanceCounter.cpp:82
(Diff revision 5)
>  {
>    return mDispatchCounter[aCategory.GetValue()];
>  }
> +
> +uint64_t
> +PerformanceCounter::GetID()

const
Attachment #8994784 - Flags: review?(amarchesini)
Comment on attachment 8994784 [details]
Bug 1477943 - Add a unique id per PerformanceCounter instance -

https://reviewboard.mozilla.org/r/259318/#review266416

That was my initial patch, but with Nathan feedback I ended up removing the part that adds the pid in the value because when the PerformanceCounter is sent back to the main process, it already has a "pid" field. So the consumer of the data can do pid+":"+counterId even if the counterId can be the same in two processes.
Comment on attachment 8994784 [details]
Bug 1477943 - Add a unique id per PerformanceCounter instance -

https://reviewboard.mozilla.org/r/259318/#review266810

::: commit-message-dd386:4
(Diff revision 6)
> +Bug 1477943 - Add a unique id per PerformanceCounter instance - r?baku,froydnj
> +
> +This new id is added in the PerformanceInfo data and helps consumers distinguish
> +counters. The id generator is a thread-safe autoincrement.

I don't think we need to include implementation details about the id generator in the commit message.

::: dom/base/DocGroup.cpp:129
(Diff revision 6)
> -      return PerformanceInfo(host, pid, windowID, duration, false, isTopLevel, items);
> +      return PerformanceInfo(host, pid, windowID, duration, mPerformanceCounter->GetID(),
> +                             false, isTopLevel, items);

I think we could just `break` here as a followup, so we didn't duplicate the construction of `PerformanceInfo`?

::: xpcom/threads/PerformanceCounter.h:117
(Diff revision 6)
>     * Returns the total number of dispatches.
>     */
>    uint64_t GetTotalDispatchCount();
>  
> +  /**
> +   * Returns the unique id for the instance.

Probably worth saying something here about why we want a unique ID for the instance.

::: xpcom/threads/PerformanceCounter.cpp:17
(Diff revision 6)
>  #ifdef LOG
>  #undef LOG
>  #endif
>  #define LOG(args) MOZ_LOG(sPerformanceCounter, mozilla::LogLevel::Debug, args)
>  
> +// global counter used to uniquely identify a window

Nit: Comments start with a capital letter and end with a period, please.

::: xpcom/threads/PerformanceCounter.cpp:24
(Diff revision 6)
> +  // we're fine with this behavior since the PerformanceCounter
> +  // is returned with the PID via IPDL.

Maybe "...we're fine with this behavior because consumers can use a `(pid, counter_id)` tuple to make instances globally unique in a browser session." or something like that.
Attachment #8994784 - Flags: review?(nfroyd) → review+
Comment on attachment 8994784 [details]
Bug 1477943 - Add a unique id per PerformanceCounter instance -

https://reviewboard.mozilla.org/r/259318/#review267216
Attachment #8994784 - Flags: review?(amarchesini) → review+
Pushed by tziade@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b4bef97f6573
Add a unique id per PerformanceCounter instance - r=baku,froydnj
https://hg.mozilla.org/mozilla-central/rev/b4bef97f6573
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: