Try increasing the ThreadStackHelper's initial max buffer size to avoid (chrome script) in BHR pseudostacks

RESOLVED FIXED in Firefox 52

Status

()

Core
XPCOM
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Blocks: 1 bug)

50 Branch
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

Bug 1023461 added the ability to get chrome script URIs in thread hang stacks for BHR.

That's great, except that when looking at BHR data (particularly for bug 1310255), I'm noticing we're getting a lot of (chrome script) entries instead of URIs.

I've messed around with ThreadStackHelper locally, and what I think is going on is that ThreadStackHelper is initializing with too small a buffer. The max buffer size that it starts with is actually 0, and then as thread hangs come in (and smash through that limit), the max buffer size is adjusted for next time.

I'm pretty sure that means that for the first bunch of hangs in a process, the chrome script URIs are going to be missing.

I suggest that we increase the initial buffer size from 0 to something else.

I'm not sure what a good value is though. Locally, I noticed that the max buffer size in my content process stabilized around 340 bytes after a few minutes of use, but I really have no idea what it should be.

Hey jchen, is my above analysis / suspicion in the ballpark? If so, any suggestions on what to set the max buffer size to be initially?
Flags: needinfo?(nchen)
Is it worth adding some kind of Telemetry probe to see where buffers out in the wild converge for their max buffer size? Or do we think we can make an educated guess on what an appropriate initial max buffer size would be?
I think adding a telemetry probe is a good idea, but for the mean time I believe using an empirical value is good enough. The origin concern was there would be a bunch of threads that never hang, so we would waste memory if we pre-allocate a large buffer for each thread, but for several hundred bytes that shouldn't be a concern.
Flags: needinfo?(nchen)
(In reply to Jim Chen [:jchen] [:darchons] from comment #2)
> I think adding a telemetry probe is a good idea, but for the mean time I
> believe using an empirical value is good enough. The origin concern was
> there would be a bunch of threads that never hang, so we would waste memory
> if we pre-allocate a large buffer for each thread, but for several hundred
> bytes that shouldn't be a concern.

Would 512 bytes be reasonable?
Flags: needinfo?(nchen)
Yep, sounds good!
Flags: needinfo?(nchen)
Assignee: nobody → mconley
Comment hidden (mozreview-request)

Comment 6

a year ago
mozreview-review
Comment on attachment 8804389 [details]
Bug 1312597 - Increase ThreadStackHelper's initial max buffer size to avoid (chrome script) placeholder in BHR pseudostacks.

https://reviewboard.mozilla.org/r/88388/#review87446
Attachment #8804389 - Flags: review?(nchen) → review+

Comment 7

a year ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/570182c077ee
Increase ThreadStackHelper's initial max buffer size to avoid (chrome script) placeholder in BHR pseudostacks. r=jchen

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/570182c077ee
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.