Mark objects black at API boundaries

RESOLVED FIXED

Status

()

Core
JavaScript: GC
RESOLVED FIXED
4 years ago
5 days ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Currently we have a hodge-podge of ExposeToActiveJS, GetObjectPreserveColor, and other wackiness scattered about that probably more or less works most of the time. We can do much better here.
Which boundaries exactly? It always seemed to me that we should do gray unmarking when things move from JS::Heap to JS::Rooted/JS::Handle. Would that make sense?
(Assignee)

Comment 2

4 years ago
(In reply to Bobby Holley (:bholley) from comment #1)
> Which boundaries exactly? It always seemed to me that we should do gray
> unmarking when things move from JS::Heap to JS::Rooted/JS::Handle. Would
> that make sense?

I was going to spend a bit thinking about it before opining and probably needinfo you and Bill asking if I was on the right track. Thank you for saving me the trouble! :-)
Here's a conversation we had about this over email 18 months ago:

> ----- Original Message -----
> > From: "Bobby Holley" <bobbyholley@gmail.com>
> > My thinking is to use HandleObject API to enforce non-grayness.
> > Specifically, we'd have the invariant that all HandleObjects were
> > non-gray,
> > since in the common case any handles we're passing as arguments will
> > probably eventually enter JSAPI. If we needed to, we could introduce
> > a
> > GrayableHandleObject with automatic coercion to HandleObject via a
> > call to
> > xpc_UnmarkGrayObject.

On Thu, Dec 27, 2012 at 1:39 PM, Bill McCloskey <wmccloskey@mozilla.com> wrote:
> The new rooting API isn't really ready for this right now. Except for callbacks, we barely use handles at all in JSAPI. I'm guessing we're at least a few months away from starting to add handles to JSAPI. We need to get them working inside the JS engine first.
>
> However, it seems like a great idea for the future. Whatever rooters we expose in JSAPI could call xpc_UnmarkGray automatically. That might even allow us to eliminate all the other calls to xpc_UnmarkGray (except for ones during the forget skippable phase of cycle collection I guess).
>
> -Bill
(Assignee)

Comment 4

4 years ago
Thanks Bobby! The Rooted/Handle infrastructure is now in an excellent position to do this. I'll try to whip something up soon.
(Assignee)

Updated

4 years ago
Assignee: nobody → terrence
(Assignee)

Comment 5

4 years ago
Created attachment 8433407 [details] [diff] [review]
auto_unmark_gray-v0.diff

https://tbpl.mozilla.org/?tree=Try&rev=b160cb165cf2

This does ExposeToActiveJS whenever we move a TenuredHeap or Heap into a Rooted and adds assertions to Handle that we are not marked gray. The attached patch builds locally and renders cnn.com without triggering any of the new asserts in Handle. This may be because ExposeToActiveJS is still called everywhere in the browser: removing all of those is the next step.

This most challenging thing here (after understanding how gray marking works) was changing the include dependencies between Value.h, RootingAPI.h, GCAPI.h, and Heap.h so that we can actually call ExposeToActiveJS from RootingAPI.h.
Attachment #8433407 - Flags: review?(sphink)
It looks like this calls ExposeToActiveJS only when the thing is marked gray. That's not sufficient though. We also need to call it during incremental GC:

http://mxr.mozilla.org/mozilla-central/source/js/public/GCAPI.h#490

During incremental, we buffer up stuff that we plan to mark gray later on. Gray marking happens in one atomic step right before sweeping. If somebody calls ExposeToActiveJS before that, we need to specifically mark it black so that it doesn't get marked gray.
(Assignee)

Comment 7

4 years ago
Comment on attachment 8433407 [details] [diff] [review]
auto_unmark_gray-v0.diff

Thanks, Bill, good catch!
Attachment #8433407 - Flags: review?(sphink)
(Assignee)

Comment 8

4 years ago
Created attachment 8437862 [details] [diff] [review]
rearrange_gcapi_include_order-v0.diff

As a first step we need to make ExposeToActiveJS callable from RootingAPI.h. Currently we have (foo <- bar means foo.h includes bar.h):

GCAPI <- RootingAPI  (So ObjectPtr can use Heap<T>)
      <- Value       (So GCAPI can implement ExposeValueToActiveJS)

This patch simply inverts those two includes and moves the relevant code to the other file. This allows RootingAPI to include GCAPI and Value and gain access to all of the Expose*ToActiveJS it needs.

This was green on try, so getting it out of the way first.
Attachment #8433407 - Attachment is obsolete: true
Attachment #8437862 - Flags: review?(jcoppeard)
(Assignee)

Comment 9

4 years ago
Created attachment 8437933 [details] [diff] [review]
auto_expose_to_active_js_from_heap-v0.diff

https://tbpl.mozilla.org/?tree=Try&rev=eb883a177e03

Actually do the ExposeToActiveJS. This doesn't add the assertions because currently does absolutely nothing. The browser never goes straight from Heap<T> -> Rooted<T>, it always has an intermediate accessor that returns the raw pointer. I think the right solution here is going to be to make all of the intermediate accessors return a const ref to the Heap<T>. We should be able to hunt these down by looking for all the ExposeToActiveJS, but it's going to be a bit of work.
Attachment #8437933 - Flags: review?(wmccloskey)
(Assignee)

Comment 11

4 years ago
I forgot to add a null test to ExposeToActiveJS when I added one to isGrayGCThing.

https://tbpl.mozilla.org/?tree=Try&rev=bac3026937be

Updated

4 years ago
Attachment #8437862 - Flags: review?(jcoppeard) → review+
(Assignee)

Comment 12

4 years ago
Created attachment 8438575 [details] [diff] [review]
auto_expose_to_active_js_from_heap-v2.diff

https://tbpl.mozilla.org/?tree=Try&rev=8e61d0f6712d

This patch trips up when console.log is passed a string that happens to be a PermanentAtom because js::IncrementalReferenceBarrier was not able to handle those. The new patch adds an explicit check for this case.

With this change, I'm pretty sure we're finally catching all the new edge cases.
Attachment #8437933 - Attachment is obsolete: true
Attachment #8437933 - Flags: review?(wmccloskey)
Attachment #8438575 - Flags: review?(wmccloskey)
Comment on attachment 8438575 [details] [diff] [review]
auto_expose_to_active_js_from_heap-v2.diff

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

This looks nice. I'd like to wait on reviewing until the PreserveColor stuff is ready though.

::: js/public/HeapAPI.h
@@ +264,5 @@
>  
>  static MOZ_ALWAYS_INLINE bool
>  GCThingIsMarkedGray(void *thing)
>  {
> +    if (js::gc::IsNullTaggedPointer(thing))

I think the only time you need to worry about values in [1, 32) is for TaggedProto. I'd rather just have a special check there if possible.
Attachment #8438575 - Flags: review?(wmccloskey)
(Assignee)

Comment 14

4 years ago
Created attachment 8440176 [details] [diff] [review]
remove_unused_proto_arg-v0.diff

(In reply to Bill McCloskey (:billm) from comment #13)
> I think the only time you need to worry about values in [1, 32) is for
> TaggedProto. I'd rather just have a special check there if possible.

Easier said than done, sadly. Here is the first half.
Attachment #8440176 - Flags: review?(efaustbmo)
Comment on attachment 8440176 [details] [diff] [review]
remove_unused_proto_arg-v0.diff

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

Nice. Thanks for doing this. It moved to ProxyOptions (or maybe WrapperOptions?) a while ago. r=me
Attachment #8440176 - Flags: review?(efaustbmo) → review+
Please be sure to get a DOM reviewer or whatever for Gecko changes.  Olli or I will probably work.
This work was finished up in bug 1297558.
Status: NEW → RESOLVED
Last Resolved: 5 days ago
Resolution: --- → FIXED
See Also: → bug 1297558
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.