978262 - Ignore duplicate frames when getting stack for background hang

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

For re-entrant pseudo-stack frames like "nsAppShell::ProcessNextNativeEvent" and "js::RunScript", it's not very helpful to have many of the same frames in a row. We should avoid that to save some memory, storage, and bandwidth.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Ignore duplicate frames when getting BHR stack (v1)

This patch deduplicates identical, consecutive frames to save memory, storage, and bandwidth. It also allows us to better match other similar stacks.

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8383949 [details] [diff] [review]
Ignore duplicate frames when getting BHR stack (v1)

Review of attachment 8383949 [details] [diff] [review]:
-----------------------------------------------------------------

This change affects not only the hang monitor, but it affects the profiler, too, right?

::: xpcom/threads/ThreadStackHelper.cpp
@@ +193,3 @@
>      }
> +    const char* const label = entry->label();
> +    if (label == prevLabel) {

This comparison can be pointer comparison rather than string comparison because this is only ever comparing pseudo-stack labels, which are essentially interned, correct?

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Nathan Froyd (:froydnj) from comment #2)
> Comment on attachment 8383949 [details] [diff] [review]
> Ignore duplicate frames when getting BHR stack (v1)
> 
> Review of attachment 8383949 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This change affects not only the hang monitor, but it affects the profiler,
> too, right?

It only affects the hang monitor because the profiler handles the pseudo-stack internally and doesn't use ThreadStackHelper.

> ::: xpcom/threads/ThreadStackHelper.cpp
> @@ +193,3 @@
> >      }
> > +    const char* const label = entry->label();
> > +    if (label == prevLabel) {
> 
> This comparison can be pointer comparison rather than string comparison
> because this is only ever comparing pseudo-stack labels, which are
> essentially interned, correct?

Yup!

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8383949 [details] [diff] [review]
Ignore duplicate frames when getting BHR stack (v1)

Review of attachment 8383949 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the clarifications!

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9a37d31251e

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c9a37d31251e