1020290 - Introduce templates to clean up code shared between Nursery and ForkJoinNursery

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Lars T Hansen [:lth]]

This is clean-up after 933313 that was deferred so as not to impede landing that patch.

A number of functions are shared between the Nursery and ForkJoinNursery but have awkward ifdefs, conditions, or duplication.  This bug tracks the work of cleaning that up.

- UpdateIonJSFrameForMinorGC should be templatized on the nursery type, and
  the awkward conditional in its middle should make use of the template type to
  call the correct forwardBufferPointer.

- UpdateJitActivationsForMinorGC will then need to be templatized on the
  nursery type.  The version that takes a runtime can be removed, it need
  take only a PerThreadData.

(More will come)

Comment 1

[Lars T Hansen [:lth]]

In some cases there are templates to remove:

A template was introduced for MarkExactStackRoots in gc/RootMarking.cpp to work around what is an awkwardly structured doubly nested loop.  The outer loop loops over types of objects, the inner loop loops over contexts / per-thread-data objects.  However, the regular marker and the PJS marker have different needs for the inner loop.  By inverting the loop we might be able to get cleaner code, and we might be able to get rid of the template.  Reasonably, the outer loop would loop over contexts and per-thread-data objects (and would be trivial for PJS), the inner loop would then loop over types for each per-thread-data object.

Comment 2

[Lars T Hansen [:lth]]

There is some renaming to do:

Right now, there is a notion of "the nursery" which means the GGC nursery; this shows up in method names such as "hasNursery", "nursery" (to get it from the ThreadSafeContext), and "isInsideNursery".  When talking about the ForkJoin nursery we have to use different names, eg, "fjNursery()" gets the ForkJoinNursery from a ThreadSafeContext.

Since there are now two kinds of nurseries it may be that we should rearrange a bit.  Methods for the normal nursery should be moved to ExclusiveContext; methods for the ForkJoin nursery should be moved to the ForkJoinContext.  Methods that use one kind but not the other kind may be templatized on the type of context, or they may have run-time guards (which they have anyway).

Methods for the normal nursery should have less generic names.

(Reference: Shu's remarks on https://bugzilla.mozilla.org/show_bug.cgi?id=933313#c71)

Comment 3

[Lars T Hansen [:lth]]

TryNewNurseryObject / TryNewFJNurseryObject should be refactored, see https://bugzilla.mozilla.org/show_bug.cgi?id=933313#c86 ff.

Comment 4

[Lars T Hansen [:lth]]

Regarding comment 2, there's also Terrence's reply: https://bugzilla.mozilla.org/show_bug.cgi?id=933313#c98.

Comment 5

[Lars T Hansen [:lth]]

Scalpel left in patient: There's a little bit of code left for TypedArrayObject in the GC (in copyObjectToTospace), but this should probably be removed because TypedArrayObjects are not nursery-allocated, cf https://bugzilla.mozilla.org/show_bug.cgi?id=933313#c128.

Comment 6

[Lars T Hansen [:lth]]

Bug 1024567 makes the point that there's some ad-hoc testing for inside-this-nursery or inside-that-nursery that would be conceptually cleaner if it was abstracted and named for its purpose, not its mechanism.

Comment 7

[Lars T Hansen [:lth]]

Attachment: Factor UpdateJitActivationsForMinorGC

Addresses the issue in comment #1.

Comment 8

[Lars T Hansen [:lth]]

(In reply to Lars T Hansen [:lth] from comment #7)
> Addresses the issue in comment #1.

Foo.  Addresses the issue in the bug description.

Comment 9

[Lars T Hansen [:lth]]

Attachment: Remove inert code for TypedArrayObject

Addresses the issue in comment #5.

Comment 10

[Lars T Hansen [:lth]]

Attachment: Move notions of 'nursery' out of ThreadSafeContext, rename 'fjNursery'

Addresses the issue in comment #2.

Comment 11

[Lars T Hansen [:lth]]

Attachment: Reduce the proliferation of names by introducing overloading

Addresses the issue in comment #3.

Comment 12

[Lars T Hansen [:lth]]

Try run with the four patches applied in order:
https://tbpl.mozilla.org/?tree=Try&rev=3aabdce9409e

Comment 13

[Lars T Hansen [:lth]]

Attachment: Invert the MarkExactStackRoots loop to get rid of a template

This pertains to comment #1.  Jon, you may recall we discussed this.  What I have here is /maybe/ marginally cleaner because there is no use of a template at the interface to MarkExactStackRoots, but I'm not sure how much difference it really makes.  Your call.

Comment 14

[Shu-yu Guo [:shu]]

Comment on attachment 8445863 [details] [diff] [review]
Factor UpdateJitActivationsForMinorGC

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

::: js/src/gc/Nursery.cpp
@@ +447,5 @@
> +/*static*/ void
> +js::Nursery::forwardBufferPointer(JSTracer* trc, HeapSlot **pSlotElems)
> +{
> +    trc->runtime()->gc.nursery.forwardBufferPointer(pSlotElems);
> +}

Given how this isn't doing the actual work (as opposed to ForkJoinNursery::forwardBufferPointer), perhaps it should be made static inline?

I'm not sure how much performance matters here.

::: js/src/jit/IonFrames.cpp
@@ +970,5 @@
>      uintptr_t *spill = frame.spillBase();
>      for (GeneralRegisterBackwardIterator iter(safepoint.allGprSpills()); iter.more(); iter++) {
>          --spill;
> +        if (slotsRegs.has(*iter))
> +            T::forwardBufferPointer(trc, reinterpret_cast<HeapSlot **>(spill));

Very nice.

@@ +1270,5 @@
>      return nullptr;
>  }
>  
>  #ifdef JSGC_GENERATIONAL
> +template<typename T>

Style nit: template <typename T>

@@ +1277,5 @@
>  #ifdef JSGC_FJGENERATIONAL
>      JS_ASSERT(trc->runtime()->isHeapMinorCollecting() || trc->runtime()->isFJMinorCollecting());
>  #else
>      JS_ASSERT(trc->runtime()->isHeapMinorCollecting());
>  #endif

Perhaps refactor this as well to T::isHeapMinorCollecting?

::: js/src/jit/IonFrames.h
@@ +270,5 @@
>  JSCompartment *
>  TopmostIonActivationCompartment(JSRuntime *rt);
>  
>  #ifdef JSGC_GENERATIONAL
> +template<typename T>

Style nit: template <typename T>

Comment 15

[Shu-yu Guo [:shu]]

Comment on attachment 8445865 [details] [diff] [review]
Move notions of 'nursery' out of ThreadSafeContext, rename 'fjNursery'

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

Nice.

::: js/src/jsobj.cpp
@@ +2862,1 @@
>                                                                      oldCount, newCount);

Style nit: indentation

@@ +3191,1 @@
>                                                                         oldCount, newCount);

Style nit: indentation

Comment 16

[Lars T Hansen [:lth]]

(In reply to Shu-yu Guo [:shu] from comment #14)
> Comment on attachment 8445863 [details] [diff] [review]
> Factor UpdateJitActivationsForMinorGC
> 
> @@ +1277,5 @@
> >  #ifdef JSGC_FJGENERATIONAL
> >      JS_ASSERT(trc->runtime()->isHeapMinorCollecting() || trc->runtime()->isFJMinorCollecting());
> >  #else
> >      JS_ASSERT(trc->runtime()->isHeapMinorCollecting());
> >  #endif
> 
> Perhaps refactor this as well to T::isHeapMinorCollecting?

I'm sympathetic.  However, since JSGC_FJGENERATIONAL is going away (bug 1019871) I've decided to leave it for now.

Comment 17

[Lars T Hansen [:lth]]

First four patches:

https://hg.mozilla.org/integration/mozilla-inbound/rev/173fd059805a
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdf4539783a3
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f8cc863fe30
https://hg.mozilla.org/integration/mozilla-inbound/rev/dca83d9c4132

Comment 18

[Jon Coppeard (:jonco)]

Comment on attachment 8446051 [details] [diff] [review]
Invert the MarkExactStackRoots loop to get rid of a template

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

This is cleaner, so I say go for it.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/173fd059805a
https://hg.mozilla.org/mozilla-central/rev/fdf4539783a3
https://hg.mozilla.org/mozilla-central/rev/6f8cc863fe30
https://hg.mozilla.org/mozilla-central/rev/dca83d9c4132
https://hg.mozilla.org/mozilla-central/rev/248b27831f24
https://hg.mozilla.org/mozilla-central/rev/c3d4833a1735

Comment 20

[Lars T Hansen [:lth]]

Patch "Invert the MarkStackExactRoots loop ..."
https://hg.mozilla.org/integration/mozilla-inbound/rev/955a638544a0

Comment 21

[Lars T Hansen [:lth]]

(In reply to Lars T Hansen [:lth] from comment #20)
> Patch "Invert the MarkStackExactRoots loop ..."
> https://hg.mozilla.org/integration/mozilla-inbound/rev/955a638544a0

Previous try run:
https://tbpl.mozilla.org/?tree=Try&rev=20770b737de6

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/955a638544a0