Closed Bug 1529006 Opened 9 months ago Closed 9 months ago

Use Rooted for NewObjectMetadataState


(Core :: JavaScript: GC, enhancement, P3)




Tracking Status
firefox67 --- fixed


(Reporter: allstars.chh, Assigned: allstars.chh)



(2 files, 2 obsolete files)

I split this from bug 1319468, see

But I have some questions

  1. Do we need the if clause check below?
    if (!mozilla::IsPointer<T>::value || thing)

the if (thing) check is initially for pointer types
and the pointer check is done again in

and !mozilla::IsPointer<T>::value is also confusing to me.
Do we really need this?

  1. Do we still need to call TraceRoot
    I think Rooted<> will call the trace function through DispatchWrapper, however the name argument is different, I am not sure if this matters.
Summary: Use Rooted<NewObjectMetadataState> for AutoSetNewObjectMetadata → Use Root for NewObjectMetadataState
Summary: Use Root for NewObjectMetadataState → Use Rooted for NewObjectMetadataState
Attached patch Patch. (obsolete) — Splinter Review

Hi Steve
Base on your suggestion I work out this patch, I use IgnoreGCPolicy to prevent calling 'trace' on the objects(ImmediateMetadata and DelayMetadata), can you help to check is this correct?


Attachment #9045942 - Flags: feedback?(sphink)
Comment on attachment 9045942 [details] [diff] [review]

Review of attachment 9045942 [details] [diff] [review]:

Oh, I hadn't thought of that approach. I was thinking of allowing Rooted<Variant<...>> to contain untraceable types. But come to think of it, your approach seems good -- it keeps the exception localized to types involved.

So I'm going to mark this feedback+ from me because I think it's a good approach, but I want to check with jonco for which one he thinks is better. I'll attach a sample patch with the approach I was originally thinking of.
Attachment #9045942 - Flags: feedback?(sphink)
Attachment #9045942 - Flags: feedback?(jcoppeard)
Attachment #9045942 - Flags: feedback+
Priority: -- → P3
Comment on attachment 9045942 [details] [diff] [review]

Review of attachment 9045942 [details] [diff] [review]:

I like this approach, it's simple and it's obvious what's happening.  Steve's approach is more general though.

I guess we'll see how often we need this - if it turns out to be a lot we can shift to the other approach, but for now I'd say let's go with this.

::: js/src/vm/Realm.h
@@ +330,5 @@
>        JS::GCHashSet<JSAtom*, js::DefaultHasher<JSAtom*>, js::SystemAllocPolicy>;
>    VarNamesSet varNames_;
>    friend class js::AutoSetNewObjectMetadata;
> +  JS::PersistentRooted<js::NewObjectMetadataState> objectMetadataState_;

We generally trace roots such as these manually rather than using PersistentRooted everywhere.  Can you keep this as is and call GCPolicy<NewObjectMetadataState>::trace in Realm::traceRoots()?
Attachment #9045942 - Flags: feedback?(jcoppeard) → feedback+
Attached patch Patch v2 (obsolete) — Splinter Review

addressed Jonco's comments

Attachment #9045942 - Attachment is obsolete: true
Attachment #9047168 - Flags: review?(jcoppeard)
Comment on attachment 9047168 [details] [diff] [review]
Patch v2

Review of attachment 9047168 [details] [diff] [review]:

Great!  r=me with the changes below.

::: js/src/vm/Realm.h
@@ +183,5 @@
>  using NewObjectMetadataState =
>      mozilla::Variant<ImmediateMetadata, DelayMetadata, PendingMetadata>;
>  class MOZ_RAII AutoSetNewObjectMetadata : private JS::CustomAutoRooter {

We should be able to remove the inheritance from CustomAutoRooter now, and the empty trace() method below.
Attachment #9047168 - Flags: review?(jcoppeard) → review+
Attached patch Patch v3Splinter Review

removed inherit from CustomAutoRooter

Attachment #9047168 - Attachment is obsolete: true
Attachment #9047376 - Flags: review+
Pushed by
Use Rooted for NewObjectMetadataState. r=jonco
Attachment #9046029 - Flags: review?(allstars.chh)
Attachment #9046029 - Flags: review?(allstars.chh)
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.