Closed
Bug 1023461
Opened 11 years ago
Closed 10 years ago
Record chrome script file name in BHR
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: jchen, Assigned: jchen)
Details
Attachments
(3 files)
3.49 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
8.57 KB,
patch
|
vladan
:
review+
|
Details | Diff | Splinter Review |
6.76 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
For chrome scripts, we should record the file name in the hang stack.
Assignee | ||
Comment 1•11 years ago
|
||
This patch eliminates a temporary buffer variable to save some memory.
Attachment #8437959 -
Flags: review?(snorp)
Assignee | ||
Comment 2•11 years ago
|
||
This patch moves the hang stack structure, from a Vector, to its own HangStack class that inherits from Vector. The new HangStack class contains an internal string buffer, so that we can use arbitrary text (like chrome script filename or unwound symbol name) as stack frames.
Attachment #8437978 -
Flags: review?(vdjeric)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #8437986 -
Flags: review?(snorp)
Updated•10 years ago
|
Attachment #8437959 -
Flags: review?(snorp) → review+
Comment 4•10 years ago
|
||
Comment on attachment 8437986 [details] [diff] [review]
Record filename and line number for chrome JS entries (v1)
Review of attachment 8437986 [details] [diff] [review]:
-----------------------------------------------------------------
::: xpcom/threads/ThreadStackHelper.cpp
@@ +257,5 @@
> + unsigned lineno = JS_PCToLineNumber(nullptr, aEntry->script(),
> + aEntry->pc());
> + MOZ_ASSERT(filename);
> +
> + char buffer[48]; // Enough to fit longest js file name from the tree
Should we add some padding here to future proof it a bit? Maybe 64 bytes?
Attachment #8437986 -
Flags: review?(snorp) → review+
Comment 5•10 years ago
|
||
Comment on attachment 8437978 [details] [diff] [review]
Add HangStack class to support internal string buffer (v1)
Review of attachment 8437978 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/components/telemetry/Telemetry.cpp
@@ +3096,5 @@
> }
> if (mStack.length() != aOther.mStack.length()) {
> return false;
> }
> + for (size_t i = 0; i < mStack.length(); i++) {
how about moving this logic into a HangStack::operator== method? It'd be nice if this code and the code in the closely-tied isSameAsEntry were in the same method
::: toolkit/components/telemetry/ThreadHangStats.h
@@ +63,5 @@
> + , mBuffer(mozilla::Move(aOther.mBuffer))
> + {
> + }
> +
> + bool isInBuffer(const char* aEntry) const {
Capitalize the first letter of the method names everywhere
Attachment #8437978 -
Flags: review?(vdjeric) → review+
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Vladan Djeric (:vladan) from comment #5)
> Comment on attachment 8437978 [details] [diff] [review]
> Add HangStack class to support internal string buffer (v1)
>
> ::: toolkit/components/telemetry/ThreadHangStats.h
> @@ +63,5 @@
> > + , mBuffer(mozilla::Move(aOther.mBuffer))
> > + {
> > + }
> > +
> > + bool isInBuffer(const char* aEntry) const {
>
> Capitalize the first letter of the method names everywhere
I chose lowercase because HangStack inherits from mozilla::Vector, which has lowercase names. Also we're overriding base methods like clear(), so IMO for HangStack, we should keep the lowercase convention.
Comment 7•10 years ago
|
||
Yeah I figured out that you're using lowercase because you're inheriting from Vector, but it looks odd, since methods like "isInBuffer" and "isSameAsEntry" are clearly not STL methods. Anyway, I prefer uppercase, but if you feel very strongly about it, keep the lowercase.
Assignee | ||
Comment 8•10 years ago
|
||
Okay, I changed the methods to uppercase except for clear(), which overrides mozilla::Vector::clear().
https://hg.mozilla.org/integration/mozilla-inbound/rev/c03a1baf9f47
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cc1bec060f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd86c2dc0b13
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c03a1baf9f47
https://hg.mozilla.org/mozilla-central/rev/5cc1bec060f9
https://hg.mozilla.org/mozilla-central/rev/cd86c2dc0b13
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•