1439929 - extend nsIWorkerDebugger to get performance counters (IPDL-ready)

Status: RESOLVED INVALID

(Core :: DOM: Workers, enhancement, P2)

Description

[Tarek Ziadé (:tarek)]

Let's add a API to retrieve WorkerPrivate instances (main-thread)

Comment 1

[Andrea Marchesini [:baku]]

(In reply to Tarek Ziadé (:tarek) from comment #0)
> Let's add a API to retrieve WorkerPrivate instances (main-thread)

Can you write a couple of lines about why you need this?
Maybe you want to filter by window/windowID, worker type (Dedicated, Chrome, Service, Shared) ?

Comment 2

[Tarek Ziadé (:tarek)]

In the about:energy project, we would like to measure the time spent in each runnnable in nsThread,
and link that time with the impacted tab.

The patch in bug 1437438 is doing this for all runnnables that are linked to a DocGroup,
and keeps track of the duration by storing the value in the DocGroup instance.

All values are then collected upon request via IDPL by iterating on tab groups with TabChild::GetAll()

This works well, and allow us to display the activity per tab and the details per window id.

The missing part though is workers, since a worker runnable is not linked directly to a docgroup.
The patch in bug 1437438 will be extended so we take into account runnnable for workers as well, 
and the values will be stored into the WorkerPrivate instances in that case.

What will be missing is a way to iterate over WorkerPrivate instances in IPDL's ContentChild.



Here's what I had in mind:


  void GetWorkers(nsTArray<WorkerPrivate*>& aWorkers) {
    for (auto iter = mDomainMap.Iter(); !iter.Done(); iter.Next()) {
      WorkerDomainInfo* aData = iter.UserData();
      aWorkers.AppendElements(aData->mActiveWorkers);
      aWorkers.AppendElements(aData->mActiveServiceWorkers);
    }
  }


I was thinking about adding that function along with a test for this patch,
so one may do something like:

  AutoTArray<WorkerPrivate*, 100> workers;
  RuntimeService* srv = RuntimeService::GetService();
  if (srv) {
    srv->GetWorkers(workers);
  }


:baku, if that sounds right I can push a patch here, doing that change

Comment 3

[Andrea Marchesini [:baku]]

Use nsIWorkerDebuggerManager to retrieve the list of nsIWorkerDebugger calling getWorkerDebuggerEnumerator().
You probably need to extend nsIWorkerDebugger interface.

Comment 4

[Tarek Ziadé (:tarek)]

These are IDL interfaces, and I guess I can extend nsIWorkerDebugger to grab the counters stored in workerPrivate and send back an array of structs.

But then I'll need to send them via IPDL via dom/ipc/ContentChild, so the parent process is able to collect the info from all processes.

My question is therefore: do I need to define the data I am sending via nsIWorkerDebugger in both IDL and IPDL ? or is there a way to define a struct that can be used in both places (nsIWorkerDebugger and ContentChild). It sounds like extra work to serialize the data into an IDL defined structure and then reserialize it so I can send it via IPDL.

Comment 5

[Tarek Ziadé (:tarek)]

From a discussion with baku on IRC : we can use an IPDL struct to hold the data and extend nsIWorkerDebugger with a C++ api we can use directky in PContent

Comment 6

[Tarek Ziadé (:tarek)]

Attachment: Bug 1439929 - extend nsIWorkerDebugger to get performance counters (IPDL-ready)

The new ReportPerformanceInfo() method sends back a PerformanceInfo struct.

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

Comment 7

[Tarek Ziadé (:tarek)]

Comment on attachment 8956382 [details]
Bug 1439929 - extend nsIWorkerDebugger to get performance counters (IPDL-ready)

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

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8956382 [details]
Bug 1439929 - extend nsIWorkerDebugger to get performance counters (IPDL-ready)

https://reviewboard.mozilla.org/r/225256/#review231288

::: dom/ipc/PContent.ipdl:115
(Diff revision 2)
>  namespace mozilla {
>  namespace dom {
>  
> +// Used to pass performance info stored in
> +// WorkerPrivate & DocGroup
> +struct PerformanceInfo

Move this into dom/ipc/DOMTypes.ipdlh

::: dom/ipc/PContent.ipdl:124
(Diff revision 2)
> +  uint16_t  pid;
> +  uint64_t  wid;
> +  uint64_t  pwid;
> +  uint16_t  count;
> +  uint64_t  duration;
> +  bool      worker;

Don't do this indentation. Just write type<space>name;

::: dom/workers/WorkerDebugger.h:47
(Diff revision 2)
>  
>    void
>    ReportErrorToDebugger(const nsAString& aFilename, uint32_t aLineno,
>                          const nsAString& aMessage);
>  
> +  PerformanceInfo

Write a comment saying that this method resets the Performance data.

::: dom/workers/WorkerDebugger.h:48
(Diff revision 2)
>    void
>    ReportErrorToDebugger(const nsAString& aFilename, uint32_t aLineno,
>                          const nsAString& aMessage);
>  
> +  PerformanceInfo
> +  ReportPerformanceInfo();

line before 'private:'

Comment 9

[Tarek Ziadé (:tarek)]

Comment on attachment 8956382 [details]
Bug 1439929 - extend nsIWorkerDebugger to get performance counters (IPDL-ready)

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

Comment 10

[Tarek Ziadé (:tarek)]

Comment on attachment 8956382 [details]
Bug 1439929 - extend nsIWorkerDebugger to get performance counters (IPDL-ready)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225256/diff/3-4/

Comment 11

[Tarek Ziadé (:tarek)]

Closing this bug and merging the patch with Bug 1443443 so we can add a browser test