Closed Bug 848449 Opened 7 years ago Closed 7 years ago

GC: Remove AutoAssertNoGC and AssertCanGC

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(3 files)

No description provided.
Attachment #721792 - Flags: review?(terrence)
Attachment #721793 - Flags: review?(terrence)
Attachment #721795 - Flags: review?(terrence)
This improves time taken to run jittests and jstests by roughly 13%.

I still like the idea of having the assertions though.
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.
Attachment #721792 - Flags: review?(terrence) → review+
Attachment #721793 - Flags: review?(terrence) → review+
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.
Attachment #721795 - Flags: review?(terrence) → review+
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.
Oh.  Were these no longer useful?
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.
Ah, but who has to maintain Steve?
I should also point out that basically all of the AssertCanGC under Ion were a total lie after Brian implemented SuppressGC.
Depends on: 850061
You need to log in before you can comment on or make changes to this bug.