(inactive) Jim Chen [:jchen] [:darchons] Assignee
	Description • 10 years ago

For chrome scripts, we should record the file name in the hang stack.

	(inactive) Jim Chen [:jchen] [:darchons] Assignee
	Comment 1 • 10 years ago

This patch eliminates a temporary buffer variable to save some memory.

	(inactive) Jim Chen [:jchen] [:darchons] Assignee
	Comment 2 • 10 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.

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)
	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?

	Vladan Djeric (:vladan)
	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

	(inactive) Jim Chen [:jchen] [:darchons] 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.

	Vladan Djeric (:vladan)
	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.

	(inactive) Jim Chen [:jchen] [:darchons] 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

	Ryan VanderMeulen [:RyanVM]
	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