783154 - Refactor the chrome hang code to use the same class as write poisoning

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Comment 1

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

Attachment: Refactor it

Comment 2

[Vladan Djeric (:vladan)]

Comment on attachment 652308 [details] [diff] [review]
Refactor it

I'm not module owner on this code, so I don't know if you should also get someone else to r+ this.
Also, have you tested your patch on builds without profiling enabled?

>+const ProcessedStack::Frame &ProcessedStack::GetFrame(unsigned aIndex) const
>+{
>+  return mStack[aIndex];
>+}

>+const ProcessedStack::Module &ProcessedStack::GetModule(unsigned aIndex) const
>+{
>+  return mModules[aIndex];
>+}

Maybe add ASSERT bounds checks in the two functions above?

+    // The size of this mapping. May or may not be the entire file.
+    // FIXME: why do we need this?
+    size_t mMappingSize;

We can remove this. It was meant to be used as a sanity check on the Symbolication server

>+ProcessedStack GetStackAndModules(const std::vector<uintptr_t> &aPCs, bool aRelative)
>+{
>+  std::vector<StackFrame> rawStack;
>+  for (std::vector<uintptr_t>::const_iterator I = aPCs.begin(),
>+         E = aPCs.end(); I != E; ++I) {
>+    uintptr_t aPC = *I;

I think the iterator names are supposed to be lower-case. Same for other single-letter variables in this function

>       if (waitCount >= 2) {
>         PRUint32 hangDuration = PR_IntervalToSeconds(now - lastTimestamp);
>-        Telemetry::RecordChromeHang(hangDuration, hangStack, hangModuleMap);
>-        hangStack.Clear();
>-        hangModuleMap.Clear();
>+        Telemetry::RecordChromeHang(hangDuration, stack);
>       }

The "Clear"s aren't necessary but they do free up a tiny bit of unneeded memory :)

Comment 3

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

Attachment: updated patch

Comment 4

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

Attachment: Content of chromeHangs with this patch

This is what firefox sends in the telemetry ping with this patch attached. I have also tested running a symbol server locally.

Comment 5

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

Attachment: Content of chromeHangs without this patch

Comment 6

[Benoit Girard (:BenWa)]

Comment on attachment 653744 [details] [diff] [review]
updated patch

Looks like we correctly escape MOZ_ENABLE_PROFILER_SPS and the changes to tools/profiler are fine. I don't have any opinion on the Telemetry stuff so I'm flagging Taras for that rubber-stamp sorry :(

Comment 7

[(dormant account)]

Comment on attachment 653744 [details] [diff] [review]
updated patch

This looks ok. Please move vector/string out of telemetry.h by creating a new ProcessedStack.h file. Submit that change as a patch on top of this one.

Are poisoned write changes straightforward enough to forgo ted's review?

Comment 8

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

Attachment: up updated patch

I don't see the added value of having ProcessedStack first is Telemetry.h and then moved to its own file, so I am attaching a combined patch.

The changes to mozPoisonWriteMac.cpp is to make it use the new ProcessedStack in telemetry. Ted was not involved in that code, it was originally reviewed by jlebar. I can ask him to review this patch if you are uncomfortable with it.

Comment 9

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

I have pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=cef9e787d865

Comment 10

[(dormant account)]

Comment on attachment 653934 [details] [diff] [review]
up updated patch

up to you if you think jlebar should review it. Looks good

Comment 11

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

(In reply to Taras Glek (:taras) from comment #10)
> Comment on attachment 653934 [details] [diff] [review]
> up updated patch
> 
> up to you if you think jlebar should review it. Looks good

3 r+ should be enough for any patch.

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3047a182724d

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3047a182724d