Update type descriptor objects first when compacting

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

(Blocks 1 bug)

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(3 attachments)

Assignee

Description

3 years ago
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.
Assignee

Comment 1

3 years ago
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)
Assignee

Comment 2

3 years ago
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)
Assignee

Comment 3

3 years ago
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+
Assignee

Comment 7

3 years ago
(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!
Assignee

Updated

11 months ago
Depends on: 1481093
You need to log in before you can comment on or make changes to this bug.