Closed
Bug 783154
Opened 9 years ago
Closed 9 years ago
Refactor the chrome hang code to use the same class as write poisoning
Categories
(Toolkit :: Telemetry, defect)
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: espindola, Assigned: espindola)
References
Details
Attachments
(3 files, 2 obsolete files)
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
Comment 2•9 years ago
|
||
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+
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #652308 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #653744 -
Flags: review?(vdjeric)
Attachment #653744 -
Flags: feedback?(taras.mozilla)
Assignee | ||
Comment 4•9 years ago
|
||
This is what firefox sends in the telemetry ping with this patch attached. I have also tested running a symbol server locally.
Assignee | ||
Comment 5•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #653744 -
Flags: feedback?(taras.mozilla) → review?(bgirard)
Comment 6•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #653744 -
Flags: review?(vdjeric) → review+
Comment 7•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
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)
Assignee | ||
Comment 9•9 years ago
|
||
I have pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=cef9e787d865
Comment 10•9 years ago
|
||
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+
Assignee | ||
Comment 11•9 years ago
|
||
(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•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3047a182724d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•