Closed Bug 783154 Opened 8 years ago Closed 8 years ago

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

Categories

(Toolkit :: Telemetry, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: espindola, Assigned: espindola)

References

Details

Attachments

(3 files, 2 obsolete files)

No description provided.
Attached patch Refactor it (obsolete) — Splinter Review
Assignee: nobody → respindola
Status: NEW → ASSIGNED
Attachment #652308 - Flags: review?(vdjeric)
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 :)
Attachment #652308 - Flags: review?(vdjeric) → review+
Attachment #653744 - Flags: review?(vdjeric)
Attachment #653744 - Flags: feedback?(taras.mozilla)
This is what firefox sends in the telemetry ping with this patch attached. I have also tested running a symbol server locally.
Attachment #653744 - Flags: feedback?(taras.mozilla) → review?(bgirard)
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 :(
Attachment #653744 - Flags: review?(taras.mozilla)
Attachment #653744 - Flags: review?(bgirard)
Attachment #653744 - Flags: review+
Attachment #653744 - Flags: review?(vdjeric) → review+
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?
Attachment #653744 - Flags: review?(taras.mozilla) → review+
Attached patch up updated patchSplinter Review
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.
Attachment #653744 - Attachment is obsolete: true
Attachment #653934 - Flags: review?(taras.mozilla)
Comment on attachment 653934 [details] [diff] [review]
up updated patch

up to you if you think jlebar should review it. Looks good
Attachment #653934 - Flags: review?(taras.mozilla) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/3047a182724d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Blocks: 791442
You need to log in before you can comment on or make changes to this bug.