Closed Bug 1384513 Opened 7 years ago Closed 7 years ago

Enable passing either JS::AutoCheckCannotGC or JS::AutoSuppressGCAnalysis to functions that currently only take the former

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: till, Assigned: jonco)

References

Details

Attachments

(2 files, 1 obsolete file)

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 tschneidereit@gmail.com:
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 tschneidereit@gmail.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
https://hg.mozilla.org/mozilla-central/rev/000f28217a30
https://hg.mozilla.org/mozilla-central/rev/78859e22ce17
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
NI to make sure we don't forget about the regressions.
Flags: needinfo?(jcoppeard)
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.
Flags: needinfo?(till)
Pushed by jandemooij@gmail.com:
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
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.
Assignee: till → jcoppeard
This is the original patch with just the changes to make API functions take AutoRequireNoGC instead of AutoCheckCannotGC.
Attachment #8890290 - Attachment is obsolete: true
Attachment #8903229 - Flags: review+
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 jcoppeard@mozilla.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
https://hg.mozilla.org/mozilla-central/rev/559aa293d7cc
https://hg.mozilla.org/mozilla-central/rev/c048372283ce
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1396156
Too late for 56. Mass won't fix for 56.
Flags: needinfo?(till)
You need to log in before you can comment on or make changes to this bug.