1068223 - Clean up the Cell hierarchy

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: cleanup_cell_hierarchy-v0.diff

When I introduced the Nursery, we totally broke how Cell interacts with the world. I was never pleased with our ad hoc solution: putting "tenured" on the front of the unsafe actions and hoping people would not misuse it.

With the benefit of hindsight, a much improved solution is to split Cell into:
  * Cell => actions appropriate on all gc cells (e.g. chunk access).
  * TenuredCell : public Cell => actions appropriate on tenured cells (e.g. arena access).
  * TenuredOrNurseryCell : public Cell => sibling to TenuredCell so that we cannot static_cast between them.

To handle the case of nursery allocatable things that are currently tenured, Cell has two methods: isTenured and asTenured. This cross-casts from Cell and TenuredOrNurseryCell to TenuredCell (with assertions) so that we can access the arena header bits if needed. This makes the assertions obvious at the caller. Removes many duplicated internal assertions -- if you have a TenuredCell you've already been verified. We can even pass TenuredCell around most of the GC internals, so it adds very little asTenured() noise in places where things are obviously already tenured. Best of all, it gets rid of the silly looking "tenuredFooBar" methods.

I've also taken it a step further. Now that ObjectImpl has a different base than the other GC things, we no longer need to use BarrieredCell to choose which barriers get run. Thus, we can keep Nick's nice barrier unification and move several templates to normal C++ inheritance for a nice de<>ification. The one downside of this is that we do have to dispatch pre-barriers through MarkKind instead of Mark<T>; however, every single backtrace I've looked at since GGC landed has had at least one call in this path already, so I don't think it will actually add any overhead in practice.

This was an enormously fun patch to write. Let's see if MSVC is going to spoil the party:
https://tbpl.mozilla.org/?tree=Try&rev=f860b3646fdb

Comment 1

[Terrence Cole [:terrence]]

Comment on attachment 8490293 [details] [diff] [review]
cleanup_cell_hierarchy-v0.diff

Try is green.

Note that this backs out some of the allocation rewrite. I think now that we're not using CRTP, MSVC might let us mix in MallocProvider and I want to clear the way for that rewrite.

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 8490293 [details] [diff] [review]
cleanup_cell_hierarchy-v0.diff

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

This looks really good - I like the way that things that are not nursery allocatable can just derive from TenuredCell.

I have one comment though - is it really necessary to have TenuredOrNurseryCell?  Yes it stops people subverting the nursery check by static casting to TenuredCell, but reinterpret_cast will obviously still work if people are going to go out of their way to break this.  The important thing is that Cell won't implicitly convert to TenuredCell.

Thanks for updating the compacting bits too BTW.

::: js/src/gc/Heap.h
@@ +1142,5 @@
>      auxNextLink = 0;
>  }
>  
>  static void
>  AssertValidColor(const void *thing, uint32_t color)

I think we can make this take a TenuredCell* now and drop the conversion.

@@ +1375,5 @@
> +/* static */ MOZ_ALWAYS_INLINE void
> +TenuredCell::writeBarrierPostRemove(TenuredCell *thing, void *cellp)
> +{
> +    JS_ASSERT(!IsInsideNursery(thing));
> +    JS_ASSERT_IF(thing, MapAllocToTraceKind(thing->getAllocKind()) != JSTRACE_OBJECT);

We could factor these assertions out since it's the same thing three times.

::: js/src/vm/ObjectImpl.h
@@ +940,5 @@
>      }
>  
>      /* GC Accessors */
>      void setInitialSlots(HeapSlot *newSlots) { slots = newSlots; }
> +    static bool isNullLike(const ObjectImpl *obj) { return uintptr_t(obj) < (1 << 3); }

Where does the magic 1<<3 come from?  We probably should have a constant for this.

::: js/src/vm/String.cpp
@@ -377,5 @@
>              goto visit_right_child;
>          }
>      }
>  
> -    if (!AllocChars(this, wholeLength, &wholeChars, &wholeCapacity))

I'm not sure why you changed this but I think I don't think it makes any difference.

Comment 3

[Terrence Cole [:terrence]]

(In reply to Jon Coppeard (:jonco) from comment #2)
> I have one comment though - is it really necessary to have
> TenuredOrNurseryCell?  Yes it stops people subverting the nursery check by
> static casting to TenuredCell, but reinterpret_cast will obviously still
> work if people are going to go out of their way to break this.  The
> important thing is that Cell won't implicitly convert to TenuredCell.

Good point, it's only the upcast that happens automatically, so I think it would be fine to remove the indirection. I was mostly thinking that neither should derive from Cell directly, but now that /everything/ is off Cell, I think you are right that we should just kill off the intermediate.

Comment 4

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4e226d27244

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f4e226d27244