848449 - GC: Remove AutoAssertNoGC and AssertCanGC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jon Coppeard (:jonco)]

Attachment: 1 - Remove use of AutoAssertNoGC

Comment 2

[Jon Coppeard (:jonco)]

Attachment: 2 - Remove use of AssertCanGC

Comment 3

[Jon Coppeard (:jonco)]

Attachment: 3 - Remove AutoAssertNoGC and AssertCanGC

Comment 4

[Jon Coppeard (:jonco)]

This improves time taken to run jittests and jstests by roughly 13%.

I still like the idea of having the assertions though.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 721792 [details] [diff] [review]
1 - Remove use of AutoAssertNoGC

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

Thanks for doing these!

::: js/src/jsstr.cpp
@@ -2621,1 @@
>  

Remove the line after as well.

Comment 6

[Terrence Cole [:terrence]]

Comment on attachment 721795 [details] [diff] [review]
3 - Remove AutoAssertNoGC and AssertCanGC

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

::: js/src/jscntxt.h
@@ +463,5 @@
>          SavedGCRoot(void *thing, JSGCTraceKind kind) : thing(thing), kind(kind) {}
>      };
>      js::Vector<SavedGCRoot, 0, js::SystemAllocPolicy> gcSavedRoots;
>  
>      bool                gcRelaxRootChecks;

It looks as if the relaxRootsChecks infrastructure is also dead now. Please open a new bug to remove it.

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 721792 [details] [diff] [review]
1 - Remove use of AutoAssertNoGC

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

::: js/src/jsgc.cpp
@@ -4329,5 @@
>  GCCycle(JSRuntime *rt, bool incremental, int64_t budget, JSGCInvocationKind gckind, gcreason::Reason reason)
>  {
> -    /* If we attempt to invoke the GC while we are running in the GC, assert. */
> -    AutoAssertNoGC nogc;
> -

We should be able to do JS_ASSERT(!rt->heapIsBusy()); to keep an equivalent assertion.

Comment 8

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f079105a0966
https://hg.mozilla.org/integration/mozilla-inbound/rev/d01a2a30d626
https://hg.mozilla.org/integration/mozilla-inbound/rev/1768185637fd

Comment 9

[Ed Morley [:emorley]]

Push backed out for Window mochitest-chrome crashes:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1768185637fd

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=7f0a4a8f2013

Comment 10

[Jon Coppeard (:jonco)]

Relanding:

https://hg.mozilla.org/integration/mozilla-inbound/rev/05113da6e613
https://hg.mozilla.org/integration/mozilla-inbound/rev/59f5f4b017ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/908a5ff75e5e

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/05113da6e613
https://hg.mozilla.org/mozilla-central/rev/59f5f4b017ed
https://hg.mozilla.org/mozilla-central/rev/908a5ff75e5e

Comment 12

[Nicholas Nethercote [inactive]]

Oh.  Were these no longer useful?

Comment 13

[Terrence Cole [:terrence]]

Pros of having them:
  In source documentation of GC patterns.
  Runtime assertions of GC safety.

Cons of having them:
  Maintenance.
  Weakness of the assertions -- dependent on AssertCanGC coverage.
  Runtime cost -- Was eating 5-10% of our time in debug builds even with our spotty coverage.

Now that we have mrgiggles, the cons outweigh the pros. Except for Steve, who has to maintain mrgiggles, but that should already be fairly well automated.

Comment 14

[Nicholas Nethercote [inactive]]

Ah, but who has to maintain Steve?

Comment 15

[Terrence Cole [:terrence]]

I should also point out that basically all of the AssertCanGC under Ion were a total lie after Brian implemented SuppressGC.