Closed Bug 1338614 Opened 5 years ago Closed 5 years ago

Refactor incremental barrier APIs to call the read barrier

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: jonco, Assigned: jonco)

Details

Attachments

(3 files)

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.
Firstly, rename IncrementalReferenceBarrier and associated functions to IncrementalPreWriteBarrier since that's what it is.
Attachment #8836157 - Flags: review?(sphink)
Add IncrementalReadBarrier along the same lines and use it for MarkStringAsLive, now renamed StringReadBarrier.
Attachment #8836158 - Flags: review?(sphink)
Finally, use IncrementalReadBarrier in ExposeToActiveJS and remove unused write barrier functions.
Attachment #8836159 - Flags: review?(sphink)
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+
Attachment #8836158 - Flags: review?(sphink) → review+
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+
(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
https://hg.mozilla.org/mozilla-central/rev/84293676546d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.