Closed
Bug 808699
Opened 12 years ago
Closed 12 years ago
Change the wire format for hang reports
Categories
(Toolkit :: Telemetry, defect)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: espindola, Assigned: espindola)
Details
Attachments
(1 file, 4 obsolete files)
18.66 KB,
patch
|
vladan
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #678395 -
Flags: review?(vdjeric)
Assignee | ||
Comment 1•12 years ago
|
||
Sorry, the old one was missing a merge of the last code review.
Attachment #678395 -
Attachment is obsolete: true
Attachment #678395 -
Flags: review?(vdjeric)
Attachment #678397 -
Flags: review?
Comment 2•12 years ago
|
||
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
Attachment #678397 -
Flags: review? → review+
Assignee | ||
Comment 3•12 years ago
|
||
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.
Attachment #678397 -
Attachment is obsolete: true
Attachment #682003 -
Flags: review?(vdjeric)
Assignee | ||
Comment 4•12 years ago
|
||
This is the version that filters out unnecessary information (like duration) before contacting the symbolication server.
Attachment #682060 -
Flags: review?(vdjeric)
Comment 5•12 years ago
|
||
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.
Assignee | ||
Comment 6•12 years ago
|
||
Comment on attachment 682003 [details] [diff] [review] now with the built-in about:telemetry patched Lets go with the alt version.
Attachment #682003 -
Attachment is obsolete: true
Attachment #682003 -
Flags: review?(vdjeric)
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #682060 -
Attachment is obsolete: true
Attachment #682060 -
Flags: review?(vdjeric)
Attachment #682459 -
Flags: review?(vdjeric)
Comment 8•12 years ago
|
||
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
Attachment #682459 -
Flags: review?(vdjeric) → review+
Assignee | ||
Comment 9•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b3982c803ec
Comment 10•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2b3982c803ec
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in
before you can comment on or make changes to this bug.
Description
•