Closed
Bug 852117
Opened 11 years ago
Closed 10 years ago
Tag nested eventloops so they show up in crashstats
Categories
(Firefox :: General, defect)
Tracking
()
RESOLVED
FIXED
Firefox 30
People
(Reporter: taras.mozilla, Assigned: bugzilla)
References
Details
(Whiteboard: [qa-])
Attachments
(2 files, 3 obsolete files)
4.84 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
5.64 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
It would be good to get an idea whether nested event loops increase probability of certain crashes
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → aklotz
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
This patch moves nesting level increment/decrement into a function so that we can update the crash reporter with the new nesting level when it changes.
Attachment #728279 -
Attachment is obsolete: true
Attachment #729135 -
Flags: review?(roc)
Attachment #729135 -
Flags: review?(roc) → review+
Assignee | ||
Comment 3•11 years ago
|
||
Try build: https://tbpl.mozilla.org/?tree=Try&rev=624647310b6c Push to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/794d86e866c3
Comment 4•11 years ago
|
||
Hmm, shouldn't this be attempting to only annotate when the event loop depth is >= 2? I'd worry about being surprised (now or in the future) by having crashreporter code suddenly causing a performance regression by making the main (hot) event loop slow.
Comment 5•11 years ago
|
||
The annotation for >1 won't go away unless you reannotate with 1 once the nesting disappears.
Comment 6•11 years ago
|
||
Sure, of course when the nesting goes from 2->1 the annotation should be updated. I'm saying we are calling these functions over and over when it's going from 0->1 and 1->0, and that's pure overhead that isn't useful.
Assignee | ||
Comment 7•11 years ago
|
||
I do agree about performance regression concerns; I'm already receiving Talos mail regarding the patch on inbound; it increased the malloc count by 3.5% to 4.5% depending on platform, causing a very noticeable blip on the graph, for example: http://mzl.la/YdgIfE I've noticed that the implementation of AnnotateCrashReport involves an enumeration of the annotation hash table and numerous string concatenations on every call; probably not something we want in the event loop hot path. ;-) Backed out for now due to perf regressions. https://hg.mozilla.org/integration/mozilla-inbound/rev/c6471aba7776
Comment 8•11 years ago
|
||
FYI, pretty sure this was causing infrequent mochitest-2 leaks as well. At least, I could get them to happen on the push prior to yours. https://tbpl.mozilla.org/php/getParsedLog.php?id=21127036&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=21127706&tree=Mozilla-Inbound
Comment 9•11 years ago
|
||
s/could/couldn't, of course
Comment 10•11 years ago
|
||
Keep a close eye on Windows xpcshell and cross-platform mochitest-2 crashes in your Try pushes, too - there was a rash of both while you were in the tree, which seems to have gone away with your backout.
Assignee | ||
Comment 11•11 years ago
|
||
I suspect that the mochitest-2 leaks were instances of bug 856600 but this patch amplified that bug's effects.
Depends on: 856600
Comment 12•11 years ago
|
||
My bug 860505 comment 1 stands here. We typically just add special-cases for things that need to be annotated in hot or unsafe code paths.
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8373774 -
Flags: review?(ted)
Assignee | ||
Comment 14•10 years ago
|
||
This patch moves nesting level increment/decrement into a function so that we can update the crash reporter with the new nesting level when it changes. Unlike the previous version of this patch, we now call the new CrashReporter::SetEventloopNestingLevel function as implemented in Part 1 instead of calling CrashReporter::AnnotateCrashReport.
Attachment #729135 -
Attachment is obsolete: true
Attachment #8373775 -
Flags: review?(roc)
Attachment #8373775 -
Flags: review?(roc) → review+
Comment 15•10 years ago
|
||
Comment on attachment 8373774 [details] [diff] [review] Part 1: Add event loop nesting level API to crash reporter Review of attachment 8373774 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/crashreporter/nsExceptionHandler.cpp @@ +590,5 @@ > > + if (eventloopNestingLevel > 0) { > + WriteFile(hFile, kEventLoopNestingLevelParameter, kEventLoopNestingLevelParameterLen, > + &nBytes, nullptr); > + int numChars = _snprintf(buffer, sizeof(buffer), "%u\n", eventloopNestingLevel); You probably don't want to use snprintf on Windows, it doesn't actually work right. You can use ltoa like XP_TTOA does for 32-bit time_t. @@ +699,5 @@ > } > + if (eventloopNestingLevel > 0) { > + unused << sys_write(fd, kEventLoopNestingLevelParameter, kEventLoopNestingLevelParameterLen); > + char buffer[16]; > + int numChars = snprintf(buffer, sizeof(buffer), "%u\n", eventloopNestingLevel); You're not allowed to call stdlib functions on Linux, it's potentially crashy in an exception handler. You can cheat and just use XP_TTOA here.
Attachment #8373774 -
Flags: review?(ted) → review-
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #15) > Comment on attachment 8373774 [details] [diff] [review] > Part 1: Add event loop nesting level API to crash reporter > > Review of attachment 8373774 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/crashreporter/nsExceptionHandler.cpp > @@ +590,5 @@ > > > > + if (eventloopNestingLevel > 0) { > > + WriteFile(hFile, kEventLoopNestingLevelParameter, kEventLoopNestingLevelParameterLen, > > + &nBytes, nullptr); > > + int numChars = _snprintf(buffer, sizeof(buffer), "%u\n", eventloopNestingLevel); > > You probably don't want to use snprintf on Windows, it doesn't actually work > right. You can use ltoa like XP_TTOA does for 32-bit time_t. Fixed using _ultoa. > > @@ +699,5 @@ > > } > > + if (eventloopNestingLevel > 0) { > > + unused << sys_write(fd, kEventLoopNestingLevelParameter, kEventLoopNestingLevelParameterLen); > > + char buffer[16]; > > + int numChars = snprintf(buffer, sizeof(buffer), "%u\n", eventloopNestingLevel); > > You're not allowed to call stdlib functions on Linux, it's potentially > crashy in an exception handler. You can cheat and just use XP_TTOA here. Fixed as suggested.
Attachment #8373774 -
Attachment is obsolete: true
Attachment #8375173 -
Flags: review?(ted)
Comment 17•10 years ago
|
||
Comment on attachment 8375173 [details] [diff] [review] Part 1: Add event loop nesting level API to crash reporter (v2) Review of attachment 8375173 [details] [diff] [review]: ----------------------------------------------------------------- Thanks.
Assignee | ||
Comment 18•10 years ago
|
||
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #17) > Comment on attachment 8375173 [details] [diff] [review] > Part 1: Add event loop nesting level API to crash reporter (v2) > > Review of attachment 8375173 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks. Did you mean to mark that as r+?
Flags: needinfo?(ted)
Comment 19•10 years ago
|
||
Comment on attachment 8375173 [details] [diff] [review] Part 1: Add event loop nesting level API to crash reporter (v2) Review of attachment 8375173 [details] [diff] [review]: ----------------------------------------------------------------- Uh, yes. Oops!
Attachment #8375173 -
Flags: review?(ted) → review+
Updated•10 years ago
|
Flags: needinfo?(ted)
Assignee | ||
Comment 20•10 years ago
|
||
Try: https://tbpl.mozilla.org/?tree=Try&rev=8f15ef0c013e https://hg.mozilla.org/integration/mozilla-inbound/rev/d9faf52a7d5f https://hg.mozilla.org/integration/mozilla-inbound/rev/af983860c74d
Comment 21•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d9faf52a7d5f https://hg.mozilla.org/mozilla-central/rev/af983860c74d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Updated•10 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•