Closed Bug 1371435 Opened 3 years ago Closed 3 years ago

Continue to use CRITICAL_SECTION for Windows Mutex impl on Nightly

Categories

(Core :: XPCOM, enhancement)

Unspecified
Windows
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: aklotz, Assigned: erahm)

References

Details

Attachments

(1 file)

CRITICAL_SECTIONS provide a lot of really, really useful debugging info. In bug 1344827 I modified our mutex implementation to support that debug info on Nightly.

Now that bug 1364624 has switched everything to SRWLOCKs, we don't have access to that rich debug info anymore.

I'd like to propose that we switch back to CRITICAL_SECTIONS with debug info for Nightlies (both debug and release), but continue to use SRWLOCKs on Beta and Release.
Eric, thoughts?
Flags: needinfo?(erahm)
(In reply to Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman) from comment #1)
> Eric, thoughts?

I'm mostly okay with this, but am a little concerned about having such a different implementation b/w nightly and beta.
Flags: needinfo?(erahm)
We discussed this and think the debug info outweighs the small win in lock size and hypothetical (but unconfirmed) perf improvements.
CRITICAL_SECTIONs have useful debug info that we'd like to keep around at least
on nightly, rather than having diverging signatures on crash stats we'd like to
just keep a unified implementation. As we didn't see significant perf
improvements after landing we're going to just back this out.
Attachment #8881944 - Flags: review?(nfroyd)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Comment on attachment 8881944 [details] [diff] [review]
Backed out changeset 5b6d169feb92

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

\o/
Attachment #8881944 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/8c69b1e4d5a6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.