Closed
Bug 1266107
Opened 8 years ago
Closed 8 years ago
Update type descriptor objects first when compacting
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: jonco, Assigned: jonco)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
13.40 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
8.78 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
8.30 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
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•8 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•8 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•8 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 4•8 years ago
|
||
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 5•8 years ago
|
||
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 6•8 years ago
|
||
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•8 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!
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeb033507f6a https://hg.mozilla.org/integration/mozilla-inbound/rev/67aee454e51e https://hg.mozilla.org/integration/mozilla-inbound/rev/72615136e898
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/eeb033507f6a https://hg.mozilla.org/mozilla-central/rev/67aee454e51e https://hg.mozilla.org/mozilla-central/rev/72615136e898
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•