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)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla57
People
(Reporter: till, Assigned: jonco)
References
Details
Attachments
(2 files, 1 obsolete file)
30.93 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
5.10 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•7 years ago
|
||
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
Comment 4•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/000f28217a30 https://hg.mozilla.org/mozilla-central/rev/78859e22ce17
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 5•7 years ago
|
||
NI to make sure we don't forget about the regressions.
Flags: needinfo?(jcoppeard)
Reporter | ||
Comment 6•7 years ago
|
||
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.
Updated•7 years ago
|
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
Comment 8•7 years ago
|
||
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
Flags: needinfo?(jcoppeard)
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/725d71e0ccdaf3aac178a54339fc4c2fa429d632 https://hg.mozilla.org/mozilla-central/rev/94b3c2e4a6fddabe0a67fd9a67f312206d49a5d6 https://hg.mozilla.org/mozilla-central/rev/d7d20b3b2f303590edba39d9d86943e6e757a243
Target Milestone: mozilla56 → ---
Assignee | ||
Comment 10•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: till → jcoppeard
Assignee | ||
Comment 11•7 years ago
|
||
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+
Assignee | ||
Comment 12•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8903262 -
Flags: review?(sphink) → review+
Comment 13•7 years ago
|
||
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
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/559aa293d7cc https://hg.mozilla.org/mozilla-central/rev/c048372283ce
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 15•7 years ago
|
||
Too late for 56. Mass won't fix for 56.
Reporter | ||
Updated•3 years ago
|
Flags: needinfo?(till)
You need to log in
before you can comment on or make changes to this bug.
Description
•