Closed Bug 1266107 Opened 4 years ago Closed 4 years ago

Update type descriptor objects first when compacting

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

We have a lot of complicated special case code in TypedObject's trace hook to handle the fact that it accesses TypeDescr objects to find out what how to trace itself, and during the update part of compacting these TypeDescr objects may not themselves have been updated yet.

Instead, we should keep track of all live TypeDescr (and associated) objects in a zone and update these first.  There should not be too many of these relative to the typed objects themselves or other objects in general.
Add a cache on the zone and populate it with type descriptor and associated objects when they are created.  This is a weak cache and is swept so that the objects are removed when they die.  There is no read barrier because it's never accessed outside of the GC and the contents cannot escape back to the mutator.
Attachment #8743310 - Flags: review?(terrence)
Update these objects first when compacting.  This patch adds UpdateCellPointers to update a single cell and renames the previous UpdateCellPointers to UpdateArenaPointers.  This is more consistent with RelocateCell/RelocateArena above.
Attachment #8743315 - Flags: review?(terrence)
Now we can get rid of all the maybeForwardedBlah() methods that were used when tracing typed objects.
Attachment #8743317 - Flags: review?(terrence)
Comment on attachment 8743310 [details] [diff] [review]
bug1266107-1-add-type-descr-set

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

::: js/src/gc/Zone.h
@@ +331,5 @@
>  
> +    // Keep track of all TypeDescr and related objects in this compartment.
> +    // This is used by the GC to trace them all first when compacting, since the
> +    // TypedObject trace hook may access these objects.
> +    using TypeDescrObjectSet = js::GCHashSet<JS::Heap<JSObject*>,

Does this actually require that we not have pre barriers? If not, we should use RelocatablePtr for inlined post barriers.

@@ +334,5 @@
> +    // TypedObject trace hook may access these objects.
> +    using TypeDescrObjectSet = js::GCHashSet<JS::Heap<JSObject*>,
> +                                             js::MovableCellHasher<JS::Heap<JSObject*>>,
> +                                             js::SystemAllocPolicy>;
> +    TypeDescrObjectSet typeDescrObjects;

Could you make this a WeakCache? If so, all of the changes to jsgc.cpp (except the spacing fix) can go away.

::: js/src/jsgc.cpp
@@ +5060,5 @@
>  };
>  
>  #define MAKE_GC_SWEEP_TASK(name)                                              \
>      class name : public GCSweepTask {                                         \
> +        void run() override;                                                  \

Do'h! Thanks for the fix.
Attachment #8743310 - Flags: review?(terrence) → review+
Comment on attachment 8743315 [details] [diff] [review]
bug1266107-2-update-type-descriptors-first

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

I like it!
Attachment #8743315 - Flags: review?(terrence) → review+
Comment on attachment 8743317 [details] [diff] [review]
bug1266107-3-simplify-trace-hook

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

\o/
Attachment #8743317 - Flags: review?(terrence) → review+
(In reply to Terrence Cole [:terrence] from comment #4)

Thanks for the comments.

> Could you make this a WeakCache? If so, all of the changes to jsgc.cpp
> (except the spacing fix) can go away.

Nice, that is much better!
Depends on: 1481093
You need to log in before you can comment on or make changes to this bug.