Closed Bug 1529006 Opened 9 months ago Closed 9 months ago

Use Rooted for NewObjectMetadataState

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

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

Details

Attachments

(2 files, 2 obsolete files)

I split this from bug 1319468, see
https://bugzilla.mozilla.org/show_bug.cgi?id=1319468#c5
https://mozilla.logbot.info/jsapi/20190205#c15925279

But I have some questions

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

https://searchfox.org/mozilla-central/rev/05d4b6962a571585bd679d2bbb0df0a5fb4e4eff/js/public/GCVariant.h#50
https://searchfox.org/mozilla-central/rev/05d4b6962a571585bd679d2bbb0df0a5fb4e4eff/js/public/GCVariant.h#79

the if (thing) check is initially for pointer types
and the pointer check is done again in https://searchfox.org/mozilla-central/rev/05d4b6962a571585bd679d2bbb0df0a5fb4e4eff/js/src/gc/Policy.h#47

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

  1. Do we still need to call TraceRoot
    https://searchfox.org/mozilla-central/rev/05d4b6962a571585bd679d2bbb0df0a5fb4e4eff/js/src/vm/Realm.h#196
    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?

Thanks

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

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]
Patch.

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)
Status: NEW → ASSIGNED
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 allstars.chh@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/584d1d57db0b
Use Rooted for NewObjectMetadataState. r=jonco
Attachment #9046029 - Flags: review?(allstars.chh)
Attachment #9046029 - Flags: review?(allstars.chh)
Status: ASSIGNED → RESOLVED
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.