Closed Bug 1052422 Opened 5 years ago Closed 5 years ago

Remove trivial shim functions that call into the GC

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: jonco, Assigned: jonco)

Details

Attachments

(1 file)

As a followup to bug 988486, there are now a bunch of one-line functions that just serve to call a method on the GCRuntime which can be removed and their use replaced with a direct call to that method.
I also added the convenience methods JSContext::minorGC() and GCRuntime::evictNursery().
Attachment #8472217 - Flags: review?(sphink)
Comment on attachment 8472217 [details] [diff] [review]
bug1052422-remove-shims

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

r=me either way, but I'm wondering what you think about the other evictNursery reasons. I'll leave it up to you which way is best.

::: js/src/gc/GCRuntime.h
@@ +277,5 @@
>      bool maybeGC(Zone *zone);
>      void maybePeriodicFullGC();
>      void minorGC(JS::gcreason::Reason reason);
>      void minorGC(JSContext *cx, JS::gcreason::Reason reason);
> +    void evictNursery() { minorGC(JS::gcreason::EVICT_NURSERY); }

I think you might want to pass in a reason here, probably defaulting to EVICT_NURSERY. Or maybe callers who don't want to use EVICT_NURSERY should be calling minorGC directly.

::: js/src/jsapi.cpp
@@ +4817,5 @@
>      // flushing of this memory (such as setting requestAnimationFrame).
>      if (script->length() > LARGE_SCRIPT_LENGTH) {
>          script = nullptr;
>          PrepareZoneForGC(cx->zone());
> +        cx->runtime()->gc.gc(GC_NORMAL, JS::gcreason::FINISH_LARGE_EVALUTE);

Heh. "EVALUTE". Oh well.

::: js/src/jsfriendapi.cpp
@@ +809,5 @@
>  js::DumpHeapComplete(JSRuntime *rt, FILE *fp, js::DumpHeapNurseryBehaviour nurseryBehaviour)
>  {
>  #ifdef JSGC_GENERATIONAL
>      if (nurseryBehaviour == js::CollectNurseryBeforeDump)
> +        rt->gc.evictNursery();

Is EVICT_NURSERY or API a better reason here?

::: js/src/jsgc.cpp
@@ +1185,5 @@
>          VerifyBarriers(rt, PostBarrierVerifier);
>  
>  #ifdef JSGC_GENERATIONAL
>      if (zealMode == ZealGenerationalGCValue) {
> +        evictNursery();

DEBUG_GC seems like a better reason than EVICT_NURSERY here.
Attachment #8472217 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] from comment #2)

Thanks for the comments.

> > +    void evictNursery() { minorGC(JS::gcreason::EVICT_NURSERY); }
> 
> I think you might want to pass in a reason here, probably defaulting to
> EVICT_NURSERY. Or maybe callers who don't want to use EVICT_NURSERY should
> be calling minorGC directly.

The latter was what I intended, however having a reason code seems like a good idea too so I'll add that.

I mainly added the separate evictNursery() method because when I was working on semispace nursery it was necessary in some places to tenure live things in both generations whereas a regular minor GC would only tenure things in the old generation.  Even without semispace it seems like a good idea to know at the callsite whether a minor GC is being done for this specific effect rather than because we decided it was a good time to do a collection.

> > +        cx->runtime()->gc.gc(GC_NORMAL, JS::gcreason::FINISH_LARGE_EVALUTE);
> 
> Heh. "EVALUTE". Oh well.

Ugh, I'll fix this.
https://hg.mozilla.org/mozilla-central/rev/a24871f33bf9
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.