Closed Bug 1384513 Opened 4 years ago Closed 4 years ago
Enable passing either JS::Auto
Check Cannot GC or JS::Auto Suppress GCAnalysis to functions that currently only take the former
As discussed on IRC yesterday, this changes the signatures of public functions taking JS::AutoCheckCannotGC to instead take a common sub-class of that and JS::AutoSuppressGCAnalysis.
Attachment #8890290 - Flags: review?(jcoppeard)
Comment on attachment 8890290 [details] [diff] [review] Change all public APIs to take JS::AutoRequireNoGC instead of JS::AutoCheckCannotGC Review of attachment 8890290 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for doing this. One problem that I didn't spot earlier: the zero-argument constructor of AutoAssertNoAlloc does nothing unless you later call disallowAlloc(), whereas if you construct AutoAssertNoGC with no arguments it gets the current context from TLS and assumes it to be valid. The use of AutoSuppressGCAnalysis in HelperThread::threadLoop() happens when there is no valid context. This behaviour of AutoAssertNoAlloc was surprising and arguably wrong, but that's not the point here. To preserve the existing behaviour, can you make the bodies of AutoAssertNoGC's constructor and destructor conditional on cx_ being non-null?
Attachment #8890290 - Flags: review?(jcoppeard) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/000f28217a30 Change all public APIs to take JS::AutoRequireNoGC instead of JS::AutoCheckCannotGC. r=jonco
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/78859e22ce17 Follow-up to fix bustage from template usage of functions with a changed signature, on a CLOSED TREE. r=bustage
NI to make sure we don't forget about the regressions.
I'll back this out this afternoon. I can work around the issue that I created the patch for, so it doesn't seem useful to keep the regression in.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/725d71e0ccda Backed out changeset 78859e22ce17 for perf regressions. https://hg.mozilla.org/integration/mozilla-inbound/rev/94b3c2e4a6fd Backed out changeset 000f28217a30 for perf regressions. https://hg.mozilla.org/integration/mozilla-inbound/rev/d7d20b3b2f30 backout followup - Add AutoCheckCannotGC in Stream.cpp. r=me
It has been almost a week and we have a merge approaching, so I backed this out myself: https://hg.mozilla.org/integration/mozilla-inbound/rev/725d71e0ccdaf3aac178a54339fc4c2fa429d632 https://hg.mozilla.org/integration/mozilla-inbound/rev/94b3c2e4a6fddabe0a67fd9a67f312206d49a5d6 https://hg.mozilla.org/integration/mozilla-inbound/rev/d7d20b3b2f303590edba39d9d86943e6e757a243
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This caused a regression in at least the sunspider format-xparb test. This is probably caused by the fact that AutoAssertNoGC is compiled in release builds, because it uses MOZ_DIAGNOSTIC_ASSERT not MOZ_ASSERT.
This is the original patch with just the changes to make API functions take AutoRequireNoGC instead of AutoCheckCannotGC.
This patch removes AutoAssertNoAlloc which was only used by AutoSuppressGCAnalysis and updates the latter to use AutoAssertNoGC in debug builds, or to have no assertions in release builds. The problem with the original patch was that AutoAssertNoGC is a diagnostic mode assert and so is compiled in nightly debug builds. This caused performance regressions.
Attachment #8903262 - Flags: review?(sphink)
Attachment #8903262 - Flags: review?(sphink) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/559aa293d7cc Change all public APIs to take JS::AutoRequireNoGC instead of JS::AutoCheckCannotGC. r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/c048372283ce Remove AutoAssertNoAlloc r=sfink
You need to log in before you can comment on or make changes to this bug.