Closed Bug 1306382 Opened 3 years ago Closed 3 years ago

Automatically ExposeToActiveJS when reading out of a TenuredHeap<T>

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(1 file)

Following on from bug 1297558, I guess we also need to add a barrier on TenuredHeap<T> too.
Blocks: 1306579
Patch to add an auto-exposing read barrier to TenuredHeap<T> just as we did to Heap<T>.
Assignee: nobody → jcoppeard
Attachment #8797642 - Flags: review?(sphink)
Comment on attachment 8797642 [details] [diff] [review]
bug1306382-auto-expose-tenured-heap

Requesting additional review for the non-SM parts.
Attachment #8797642 - Flags: review?(continuation)
Comment on attachment 8797642 [details] [diff] [review]
bug1306382-auto-expose-tenured-heap

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

r=me for the non JS engine parts.
Attachment #8797642 - Flags: review?(continuation) → review+
Comment on attachment 8797642 [details] [diff] [review]
bug1306382-auto-expose-tenured-heap

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

r=me for the non JS engine parts.

::: js/xpconnect/src/XPCInlines.h
@@ +470,5 @@
>  inline
>  void XPCWrappedNativeTearOff::JSObjectMoved(JSObject* obj, const JSObject* old)
>  {
>      MOZ_ASSERT(!IsMarked());
> +    MOZ_ASSERT(mJSObject.unbarrieredGetPtr() == old);

Some kind of operator== for JS::TenuredHeap<JSObject*> would avoid the need for this here and other places. I don't think we ever want to unmarkgray for that.
Comment on attachment 8797642 [details] [diff] [review]
bug1306382-auto-expose-tenured-heap

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

I think I agree with mccr8 that making an unbarriered operator== would be a good idea. Not just for the convenience -- in fact, some of the callsites might be more clear with the current explicit unbarrieredGetPtr() == x, I'm not sure.

But it seems like the current operator== is a bit of a footgun; it feels unexpected that it would affect mark bits. With the operator==, tenuredHeap == x is different from x == tenuredHeap, I guess, but that doesn't seem too awful.
Attachment #8797642 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] from comment #5)
I agree, but for reasons that are still obscure to me I've been unable to provide an operator== that C++ will select in preference to converting a TenuredHeap<T> into the base type and triggering the read barrier.  I'm going to land this as is and follow up in a separate bug.
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6217f779742e
Automatically ExposeToActiveJS when reading out of a TenuredHeap<T> r=sfink r=mccr8
https://hg.mozilla.org/mozilla-central/rev/6217f779742e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.