808699 - Change the wire format for hang reports

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: patch

This patch requires a new server to be deployed, but I am opening the bug early in case the code review can be done in parallel.

Comment 1

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: correct patch

Sorry, the old one was missing a merge of the last code review.

Comment 2

[Vladan Djeric (:vladan)]

Comment on attachment 678397 [details] [diff] [review]
correct patch

>diff --git a/toolkit/components/telemetry/ProcessedStack.h b/toolkit/components/telemetry/ProcessedStack.h
>+  // The version of the report format
>+  JSBool ok = JS_DefineProperty(cx, fullReportObj, "version",
>+                                INT_TO_JSVAL(2),
>+                                NULL, NULL, JSPROP_ENUMERATE);

Move the magic version number to a #define at the top of the file



>+    // Current module
>+    const Telemetry::ProcessedStack::Module &module =
>+      mHangReports.GetModule(moduleIndex);

Nit: Ampersand next to data-type (throughout patch)

>+      unsigned Index = 0;

camel-case for variable names

>+    for (size_t pcIndex = 0; pcIndex < pcCount; ++pcIndex) {
>+      const Telemetry::ProcessedStack::Frame &frame = stack[pcIndex];
>+      JSObject *framePair = JS_NewArrayObject(cx, 0, nullptr);

Check for NULL ptr

>+      int modIndex = std::numeric_limits<uint16_t>::max() == frame.mModIndex ?
>+        -1 : frame.mModIndex;

Nit: add parentheses around condition for clarity

>+      if (!JS_SetElement(cx, framePair, 0, &INT_TO_JSVAL(modIndex))) {
>+        return NS_ERROR_FAILURE;
>+      }

I don't know how safe &INT_TO_JSVAL is, especially since it's a macro liable to change. Just declare a jsval instead

>+      if (!JS_SetElement(cx, framePair, 1, &INT_TO_JSVAL(frame.mOffset))) {

Same

>+      jsval v = OBJECT_TO_JSVAL(framePair);

Give it a full name, e.g. framePairVal

>-ProcessedStack GetStackAndModules(const std::vector<uintptr_t> &aPCs, bool aRelative)
>+ProcessedStack GetStackAndModules(const std::vector<uintptr_t> &aPCs)

Put return value on separate line

Comment 3

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: now with the built-in about:telemetry patched

This patch includes the review requests from the previous one and also changes the built-in about:telemetry to use the new format.

I also changes the proposed v2 format a bit. It now looks like

{ memoryMap : [module1, module2, ...] , stacks : [ stack1, stack2, ..] }

Each module is of the form [name, pdb age, pdb sig, pdb name].
Each stack is fo the form { stack: [offset1, offset2], duration: num }

The only difference from the previous proposed v2 format is the s/hangs/stacks. The logic is that this is the format that is used for any symbolication, no just hangs. The format accepts but ignores any extra fields in a stack (duration for example).

A more compact option would be making each stack just a list of offsets. The downside is that the client (firefox) would have to do a bit more work to construct the symbolication request from the hang report. Let me know which one you like best.

Comment 4

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: alternative version

This is the version that filters out unnecessary information (like duration) before contacting the symbolication server.

Comment 5

[Vladan Djeric (:vladan)]

How about this format for chromeHangs:

{ memoryMap: [module1, module2, ...], stacks: [stack1, stack2, ..], durations: [duration1, duration2] }

So if the chromeHang data is going to be used by the Profiler for symbolication instead of hang reporting, it can just remove the "durations" field.

Comment 6

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Comment on attachment 682003 [details] [diff] [review]
now with the built-in about:telemetry patched

Lets go with the alt version.

Comment 7

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: with a durations array

Comment 8

[Vladan Djeric (:vladan)]

Comment on attachment 682459 [details] [diff] [review]
with a durations array

>diff --git a/toolkit/components/telemetry/Telemetry.cpp b/toolkit/components/telemetry/Telemetry.cpp
> NS_IMETHODIMP
> TelemetryImpl::GetChromeHangs(JSContext *cx, jsval *ret)
> {
>   MutexAutoLock hangReportMutex(mHangReportsMutex);
>-  JSObject *reportArray = JS_NewArrayObject(cx, 0, nullptr);
>-  if (!reportArray) {
>+
>+  JSObject *fullReportObj = JS_NewObject(cx, NULL, NULL, NULL);

We use nullptr instead of NULL now

>+  for (size_t moduleIndex = 0; moduleIndex < moduleCount; ++moduleIndex) {
>+    // Current module
>+    const Telemetry::ProcessedStack::Module& module =
>+      mHangReports.GetModule(moduleIndex);
> 
>       JSObject *moduleInfoArray = JS_NewArrayObject(cx, 0, nullptr);
>       if (!moduleInfoArray) {
>         return NS_ERROR_FAILURE;
>       }

Looks like inconsistent indentation

>+  for (size_t i = 0, n = mHangReports.GetStackCount(); i < n; ++i) {
>+    jsval duration = INT_TO_JSVAL(mHangReports.GetDuration(i));
>+    if (!JS_SetElement(cx, durationArray, i, &duration)) {
>+      return NS_ERROR_FAILURE;
>+    }
>+  }
>+
>+  for (size_t i = 0, n = mHangReports.GetStackCount(); i < n; ++i) {
>+    // Represent call stack PCs as (module index, offset) pairs.
>+    JSObject *pcArray = JS_NewArrayObject(cx, 0, nullptr);
>+    if (!pcArray) {
>+      return NS_ERROR_FAILURE;
>+    }

Might as well merge these into a single loop

>diff --git a/toolkit/content/aboutTelemetry.js b/toolkit/content/aboutTelemetry.js
>     let hangs = Telemetry.chromeHangs;
>+    let stacks = hangs.stacks;
>     for (let i = 0; i < jsonResponse.length; ++i) {
>       let stack = jsonResponse[i];
>-      let hangDuration = hangs[i].duration;
>+      let hangDuration = stacks[i].duration;

Change this as we discussed

Comment 9

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b3982c803ec

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2b3982c803ec