Closed Bug 1443443 Opened 6 years ago Closed 6 years ago

Extend PContent to retrieve Performance Counters in the parent process

Categories

(Core :: DOM: Content Processes, defect, P3)

Product:
Core
Component:
DOM: Content Processes
Version:
57 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: tarek, Assigned: tarek)

References

Details

(Whiteboard: [perf-tools])

Attachments

(1 file)

Bug 1443443 - Extend PContent to retrieve Performance Counters in the parent process -
59 bytes, text/x-review-board-request
baku
: review+
Details 
Extends the PContent protocol so the parent process can retrieve performance counter values from all DocGroup and Workers (see Bug 1439929 and 1437438)
No longer depends on: 1437438
Comment on attachment 8956757 [details]
Bug 1443443 - Extend PContent to retrieve Performance Counters in the parent process -

https://reviewboard.mozilla.org/r/225726/#review231580

This works for e10s, what about non e10s?

::: dom/base/DocGroup.cpp:10
(Diff revision 1)
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include <unistd.h> // for getpid()
>  #include "mozilla/dom/DocGroup.h"
>  #include "mozilla/dom/TabGroup.h"
> +#include "mozilla/dom/DOMTypes.h"

alphabetic order.

::: dom/base/DocGroup.cpp:18
(Diff revision 1)
>  #include "nsDOMMutationObserver.h"
>  
>  namespace mozilla {
>  namespace dom {
>  
> +

no extra line.

::: dom/base/DocGroup.cpp:89
(Diff revision 1)
> +  uint16_t count = 0;
> +  uint64_t duration = 0;
> +  nsCString host = NS_LITERAL_CSTRING("None");
> +
> +  // XXX cache ?
> +  for (const auto& _doc : *this) {

why _doc and not doc ? Call it document instead.

::: dom/base/DocGroup.cpp:91
(Diff revision 1)
> +  nsCString host = NS_LITERAL_CSTRING("None");
> +
> +  // XXX cache ?
> +  for (const auto& _doc : *this) {
> +    // grabbing the host name of the first document
> +    nsCOMPtr<nsIDocument> doc = do_QueryInterface(_doc);

MOZ_ASSERT(doc)

::: dom/base/DocGroup.cpp:93
(Diff revision 1)
> +  // XXX cache ?
> +  for (const auto& _doc : *this) {
> +    // grabbing the host name of the first document
> +    nsCOMPtr<nsIDocument> doc = do_QueryInterface(_doc);
> +    nsCOMPtr<nsIURI> docURI = doc->GetDocumentURI();
> +    if (docURI) {

quick continue. if (!docURI) {\ncontinue;\n} with indentation :)

::: dom/base/DocGroup.cpp:101
(Diff revision 1)
> +        pwid = wid;
> +        nsPIDOMWindowInner* win = doc->GetInnerWindow();
> +        if (win) {
> +          nsPIDOMWindowOuter* outer = win->GetOuterWindow();
> +          if (outer) {
> +            nsCOMPtr<nsPIDOMWindowOuter> top = outer->GetTop();

GetTop or GetScriptableTop?

::: dom/base/DocGroup.cpp:111
(Diff revision 1)
> +        }
> +        break;
> +    }
> +  }
> +
> +  for (uint32_t key = 0; key < (size_t)TaskCategory::Count; key++) {

why casting to size_t ? and not uint32_t?

::: dom/base/nsIPerformanceMetrics.idl:19
(Diff revision 1)
> +  readonly attribute unsigned long pid;
> +  readonly attribute unsigned long long wid;
> +  readonly attribute unsigned long long pwid;
> +  readonly attribute unsigned long count;
> +  readonly attribute unsigned long long duration;
> +  readonly attribute bool worker;

please, comment each one of these attributes.

It's unclear what pid/wpid/pwid are.
Also count... count what? duration of what? key?

::: dom/base/nsPerformanceMetrics.cpp:19
(Diff revision 1)
> +PerformanceMetricsData::PerformanceMetricsData(uint32_t aPid, uint64_t aWid,
> +                                               uint64_t aPwid, const nsCString& aHost,
> +                                               uint32_t aKey, uint32_t aCount,
> +                                               uint64_t aDuration,
> +                                               bool aWorker)
> +      : mPid(aPid), mWid(aWid), mPwid(aPwid), mCount(aCount), mHost(aHost),

2 spaces here and not 4.

::: dom/base/nsPerformanceMetrics.cpp:23
(Diff revision 1)
> +                                               bool aWorker)
> +      : mPid(aPid), mWid(aWid), mPwid(aPwid), mCount(aCount), mHost(aHost),
> +        mKey(aKey), mDuration(aDuration), mWorker(aWorker)
> +{
> +}
> +

no extra line here.

::: dom/base/nsPerformanceMetrics.cpp:76
(Diff revision 1)
> +{
> +  *aPwid = mPwid;
> +  return NS_OK;
> +};
> +
> +

no extra line here.

::: dom/base/nsPerformanceMetrics.cpp:84
(Diff revision 1)
> +PerformanceMetricsData::GetCount(uint32_t* aCount)
> +{
> +  *aCount = mCount;
> +  return NS_OK;
> +};
> +

no extra line here.

::: dom/ipc/ContentChild.h:192
(Diff revision 1)
>      Endpoint<PImageBridgeChild>&& aImageBridge,
>      Endpoint<PVRManagerChild>&& aVRBridge,
>      Endpoint<PVideoDecoderManagerChild>&& aVideoManager,
>      nsTArray<uint32_t>&& namespaces) override;
>  
> +  virtual mozilla::ipc::IPCResult

why virtual? You want:

mozilla::ipc::IPCResult
RecvRequestPerformanceMatrics() override;

::: dom/ipc/ContentChild.cpp:1353
(Diff revision 1)
> +  nsTArray<PerformanceInfo> data;
> +
> +  // iterate on all WorkerDebugger
> +  RefPtr<WorkerDebuggerManager> wdm = WorkerDebuggerManager::Get();
> +
> +  for (size_t index = 0; index < wdm->GetDebuggersLength(); index++) {

no size. uint32_t please.

::: dom/ipc/ContentChild.cpp:1354
(Diff revision 1)
> +
> +  // iterate on all WorkerDebugger
> +  RefPtr<WorkerDebuggerManager> wdm = WorkerDebuggerManager::Get();
> +
> +  for (size_t index = 0; index < wdm->GetDebuggersLength(); index++) {
> +    WorkerDebugger* debugger = wdm->GetDebuggerAt(index);

MOZ_ASSERT(debugger);

::: dom/ipc/ContentChild.cpp:1355
(Diff revision 1)
> +  // iterate on all WorkerDebugger
> +  RefPtr<WorkerDebuggerManager> wdm = WorkerDebuggerManager::Get();
> +
> +  for (size_t index = 0; index < wdm->GetDebuggersLength(); index++) {
> +    WorkerDebugger* debugger = wdm->GetDebuggerAt(index);
> +    data.AppendElement(debugger->ReportPerformanceInfo());

Can we make this nsTArray fallible?

::: dom/ipc/ContentChild.cpp:1370
(Diff revision 1)
> +        docGroup->ReportPerformanceInfo(&data);
> +    }
> +  }
> +
> +  // send the array asynchronously
> +  ContentChild::GetSingleton()->SendAddPerformanceMetrics(data);

Just do:
SendAddPerformanceMatrics(data);

::: dom/ipc/ContentParent.cpp:3277
(Diff revision 1)
>    }
>    return IPC_OK();
>  }
>  
> +mozilla::ipc::IPCResult
> +ContentParent::RecvAddPerformanceMetrics(nsTArray <PerformanceInfo>&& aMetrics)

no extra space before <

::: dom/ipc/ContentParent.cpp:3280
(Diff revision 1)
>  
> +mozilla::ipc::IPCResult
> +ContentParent::RecvAddPerformanceMetrics(nsTArray <PerformanceInfo>&& aMetrics)
> +{
> +  // converting the data we get from a child as a notification
> +  if (aMetrics.Length() == 0) {

IsEmpty()

::: dom/ipc/ContentParent.cpp:3284
(Diff revision 1)
> +  // converting the data we get from a child as a notification
> +  if (aMetrics.Length() == 0) {
> +      return IPC_OK();
> +  }
> +
> +  nsCOMPtr<nsIMutableArray> xpMetrics = do_CreateInstance(NS_ARRAY_CONTRACTID);

if (NS_WARN_IF(!xpMatrics)) ...

::: dom/ipc/ContentParent.cpp:3287
(Diff revision 1)
> +  }
> +
> +  nsCOMPtr<nsIMutableArray> xpMetrics = do_CreateInstance(NS_ARRAY_CONTRACTID);
> +
> +  for (size_t i = 0; i<aMetrics.Length(); i++) {
> +       PerformanceInfo entry = aMetrics[i];

const PerformanceInfo&

::: dom/workers/WorkerDebuggerManager.h:80
(Diff revision 1)
>    UnregisterDebuggerMainThread(WorkerPrivate* aWorkerPrivate);
>  
> +  size_t
> +  GetDebuggersLength()
> +  {
> +    return mDebuggers.Length();

IsEmpty() ?

::: dom/workers/WorkerDebuggerManager.h:86
(Diff revision 1)
> +  }
> +
> +  WorkerDebugger*
> +  GetDebuggerAt(size_t index)
> +  {
> +    return mDebuggers.ElementAt(index);

1. Use SafeElementAt()... or make MOZ_DIAGNOSTIC_ASSERT(index < mDebuggers.Length()).

2. aIndex
Attachment #8956757 - Flags: review?(amarchesini) → review-
Comment on attachment 8956757 [details]
Bug 1443443 - Extend PContent to retrieve Performance Counters in the parent process -

https://reviewboard.mozilla.org/r/225726/#review231580

I don't think we need to track the performances in non es10 - is there a flag I can use to deactivate the feature in non-es10 ?

> GetTop or GetScriptableTop?

I am not sure actually. I've looked at the difference but I am a bit confused about the impact on workers and docgroup docs

> IsEmpty() ?

is this issue a mistake?
Comment on attachment 8956757 [details]
Bug 1443443 - Extend PContent to retrieve Performance Counters in the parent process -

https://reviewboard.mozilla.org/r/225726/#review231580

> Can we make this nsTArray fallible?

I am not sure how to do this. the array contains IPDL class that won't allow me to copy one into another. Or I've failed to find the way.
Comment on attachment 8956757 [details]
Bug 1443443 - Extend PContent to retrieve Performance Counters in the parent process -

https://reviewboard.mozilla.org/r/225726/#review231960

::: dom/base/DocGroup.cpp:87
(Diff revision 2)
> +  uint64_t pwid = 0;
> +  uint16_t count = 0;
> +  uint64_t duration = 0;
> +  nsCString host = NS_LITERAL_CSTRING("None");
> +
> +  // XXX cache ?

What is this comment about?

::: dom/ipc/ContentChild.cpp:1351
(Diff revision 2)
> +ContentChild::RecvRequestPerformanceMetrics()
> +{
> +  nsTArray<PerformanceInfo> data;
> +
> +  // iterate on all WorkerDebugger
> +  RefPtr<WorkerDebuggerManager> wdm = WorkerDebuggerManager::Get();

if (NS_WARN_IF(!wdm)) {
  return IPC_OK();
}

::: dom/ipc/DOMTypes.ipdlh:131
(Diff revision 2)
>   * in WorkerPrivate & DocGroup instances
>   */
>  struct PerformanceInfo
>  {
>    nsCString host;
> +  uint16_t category;

I asked a comment about what these attributes are... 
It's hard to review them without knowing what pwid/wid are and so on.

::: dom/ipc/DOMTypes.ipdlh:143
(Diff revision 2)
> +};
> +
> +/**
> + * Used to pass performance info stored
> + * in WorkerPrivate & DocGroup instances
> + */

There is a merge issue here... 2 performanceInfo?

::: dom/workers/WorkerDebuggerManager.h:78
(Diff revision 2)
>  
>    void
>    UnregisterDebuggerMainThread(WorkerPrivate* aWorkerPrivate);
>  
> +  uint32_t
> +  GetDebuggersLength();

GetDebuggersLength() const

::: dom/workers/WorkerDebuggerManager.h:81
(Diff revision 2)
>  
> +  uint32_t
> +  GetDebuggersLength();
> +
> +  WorkerDebugger*
> +  GetDebuggerAt(uint32_t aIndex);

GetDebuggerAt(uint32_t aIndex) const
Attachment #8956757 - Flags: review?(amarchesini) → review-
Comment on attachment 8956757 [details]
Bug 1443443 - Extend PContent to retrieve Performance Counters in the parent process -

https://reviewboard.mozilla.org/r/225726/#review232014
Attachment #8956757 - Flags: review?(amarchesini) → review+
Comment on attachment 8956757 [details]
Bug 1443443 - Extend PContent to retrieve Performance Counters in the parent process -

https://reviewboard.mozilla.org/r/225726/#review231580

after a discussion on IRC, I will externalize the code added in ContentChild, so it can also be executed in the main process via a runnable to collect performance counters on the parent process.

> I am not sure actually. I've looked at the difference but I am a bit confused about the impact on workers and docgroup docs

after a discussion on IRC with baku, GetTop() is the right one to use (since I want to try to crawl back to the tab)
Priority: -- → P3
Assignee: nobody → tarek
Whiteboard: [perf-tools]
Comment on attachment 8956757 [details]
Bug 1443443 - Extend PContent to retrieve Performance Counters in the parent process -

https://reviewboard.mozilla.org/r/225724/#review235080

::: dom/base/ChromeUtils.cpp:660
(Diff revision 10)
>  #endif // NIGHTLY_BUILD
>  
> +#ifndef RELEASE_OR_BETA
> +/* static */ void
> +ChromeUtils::RequestPerformanceMetrics(GlobalObject&)
> +{

MOZ_ASSERT(XRE_IsParentProcess());
Pushed by tziade@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/20e9096156b0
Extend PContent to retrieve Performance Counters in the parent process - r=baku
Fixed (see previous try ^ )
Flags: needinfo?(tarek)
Pushed by tziade@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fae7d9814403
Extend PContent to retrieve Performance Counters in the parent process - r=baku
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/25ddde05fe9a
Backed out changeset fae7d9814403 for mochitest browser-chrome failure on browser_test_performance_metrics.js
Pushed by tziade@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e5d375c99fa4
Extend PContent to retrieve Performance Counters in the parent process - r=baku
https://hg.mozilla.org/mozilla-central/rev/e5d375c99fa4
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1456048
I don't understand the code landed here.
https://hg.mozilla.org/mozilla-central/rev/e5d375c99fa4#l10.69 
Several TabChilds may share one TabGroup.
Flags: needinfo?(tarek)
We might hit duplicates then, I guess we need to ignore TabGroup that were already iterated over.

I have filed bug 1468444 - thanks!
Flags: needinfo?(tarek)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: