Closed
Bug 1338614
Opened 7 years ago
Closed 7 years ago
Refactor incremental barrier APIs to call the read barrier
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: jonco, Assigned: jonco)
Details
Attachments
(3 files)
5.62 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
4.45 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
2.97 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
ExposeToActiveJS and MarkGCThingAsLive currently call IncrementalReferenceBarrier, which in turns calls our pre-write barrier. This is strange since these are really read barriers. The code for the read and pre-write barriers is almost, but not quite, the same. Even regardless of this, for the sake of sanity these should both call the read barrier.
Assignee | ||
Comment 1•7 years ago
|
||
Firstly, rename IncrementalReferenceBarrier and associated functions to IncrementalPreWriteBarrier since that's what it is.
Attachment #8836157 -
Flags: review?(sphink)
Assignee | ||
Comment 2•7 years ago
|
||
Add IncrementalReadBarrier along the same lines and use it for MarkStringAsLive, now renamed StringReadBarrier.
Attachment #8836158 -
Flags: review?(sphink)
Assignee | ||
Comment 3•7 years ago
|
||
Finally, use IncrementalReadBarrier in ExposeToActiveJS and remove unused write barrier functions.
Attachment #8836159 -
Flags: review?(sphink)
Comment 4•7 years ago
|
||
Comment on attachment 8836157 [details] [diff] [review] 1 - rename-incremental-reference-barrier Review of attachment 8836157 [details] [diff] [review]: ----------------------------------------------------------------- I definitely like the name change. "incremental" and "pre" seem a little redundant, but "pre" seems important and "incremental" reinforces that it is only needed during iGC, so I guess it's fine. (In theory, we might grow a new non-incremental need for a pre-write barrier.) And if you removed "incremental", you'd need to rename IsIncrementalBarrierNeededOnTenuredGCThing or it would sound weird. ::: js/public/GCAPI.h @@ +442,3 @@ > > extern JS_PUBLIC_API(void) > +IncrementalObjectPreWriteBarrier(JSObject* obj); Why have a separate Object version instead of overloading IncrementalPreWriteBarrier for JSObject*?
Attachment #8836157 -
Flags: review?(sphink) → review+
Updated•7 years ago
|
Attachment #8836158 -
Flags: review?(sphink) → review+
Comment 5•7 years ago
|
||
Comment on attachment 8836159 [details] [diff] [review] 3 - use-read-barrier-in-expose Review of attachment 8836159 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/GCAPI.h @@ +434,5 @@ > IsIncrementalBarrierNeeded(JSContext* cx); > > /* > + * Notify the GC that a reference to a JSObject is about to be overwritten. > + * This methods must be called if IsIncrementalBarrierNeeded. *method @@ -648,5 @@ > MOZ_DIAGNOSTIC_ASSERT(BarriersAreAllowedOnCurrentThread()); > > if (IsIncrementalBarrierNeededOnTenuredGCThing(thing)) > - JS::IncrementalPreWriteBarrier(thing); > - else if (!thing.mayBeOwnedByOtherRuntime() && js::gc::detail::CellIsMarkedGray(thing.asCell())) Maybe a dumb question, but why can the other-runtime check be removed?
Attachment #8836159 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Steve Fink [:sfink] [:s:] from comment #5) > Maybe a dumb question, but why can the other-runtime check be removed? We already did the check a few lines above this.
Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/84293676546d Refactor incremental barrier APIs and make them call the read barrier r=sfink
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/84293676546d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•