1319468 - Remove use of CustomAutoRooter and replace with Rooted in js/

Status: RESOLVED FIXED

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

Comment 1

[Jon Coppeard (:jonco)]

Rooted<T> will now call a trace() method on T if T is not one of the standard GC pointer types.  We should replace use of CustomAutoRooter with this and remove it so there are fewer GC primitives.

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: P1: Use Rooted for AutoRooterGetterSetter::Inner

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: P2: Use Rooted for AutoLookupVector

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: P3: Use Rooted for RootedCount

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

So far there are only 3 places needs to be updated, also the other 2 class, AutoSetNewOBjectMetadata and CacheIRWriter will stay as they are now, so I remove the 'meta' keyword as I think these could be fixed in one bug.

Jonco also mentioned another possibility to use Rooted for NewObjectMetadataState
https://searchfox.org/mozilla-central/rev/490ab7f9b84570573a49d7fa018673ce0d5ddf22/js/src/vm/Realm.h#188,
I'll file another bug for this since it doesn't relate to CustomAutoRooter.

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 9042453 [details] [diff] [review]
P1: Use Rooted for AutoRooterGetterSetter::Inner

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

Yes, this is what we want.

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 9042454 [details] [diff] [review]
P2: Use Rooted for AutoLookupVector

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

Sorry this is one of those Auto classes again.  What we want here is to use Rooted internally like you did for the previous patch, rather than make users of the class root it themselves.

So maybe you could make LookupVector into a GCVector<Lookup> and make |lookups| a Rooted<LookupVector> in the class.

Comment 8

[Jon Coppeard (:jonco)]

Comment on attachment 9042455 [details] [diff] [review]
P3: Use Rooted for RootedCount

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

Looks good.

::: js/public/UbiNodeCensus.h
@@ +179,5 @@
>    // count() method. This provides a stable way to sort sets.
>    Node::Id smallestNodeIdCounted_;
>  };
>  
> +typedef JS::Rooted<CountBasePtr> RootedCount;

nit: it reads better if you use |using RootedCount = ...| rather than typedef.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: Use Rooted<GCVector<T>> for lookups

Hi Steve
Jonco suggested me to use Rooted<GCVector<>> in comment 7, however there are some questions I'll need your suggestion

1. The original AutoLookupVector will overload the operator->
   https://searchfox.org/mozilla-central/source/js/src/vm/SavedStacks.cpp#249, but now with Rooted, I have to use get() to get the instance, is this correct?

2. removal of HandleLookup
   Now we will get JS::Handle<Lookup> or JS::MutableHandle<Lookup> from methods of AutoLookupMethod, and like original HandleLookup, I use get() to update the data structure inside Lookup, is this okay?

Thanks

Comment 10

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9043915 [details] [diff] [review]
Part 2: Use Rooted<GCVector<T>> for lookups

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

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh] from comment #9)

> Created attachment 9043915 [details] [diff] [review]
> WIP Part 2: Use Rooted<GCVector<T>> for lookups
> 
> Hi Steve
> Jonco suggested me to use Rooted<GCVector<>> in comment 7, however there are some questions I'll need your suggestion
> 1. The original AutoLookupVector will overload the operator->
> https://searchfox.org/mozilla-central/source/js/src/vm/SavedStacks.cpp#249, but now with Rooted, I have to use get() to get the instance, is this correct?
> 
> 2. removal of HandleLookup
> Now we will get JS::Handle<Lookup> or JS::MutableHandle<Lookup> from methods of AutoLookupMethod, and like original HandleLookup, I use get() to update the data structure inside Lookup, is this okay?

I had to play with this a bit to figure out the problems, and I think I eventually came to the same conclusions as you.

I think my short answer would be that yes, it's fine to add get()'s so you can convert AutoLookupVector into Rooted<GCVector<Lookup, ASYNC_STACK_MAX_FRAME_COUNT>>> and HandleLookup into Handle<Lookup>.

C++ is talking about allowing operator.() to be overridden, which would be useful in making Handle<Lookup> support `lookup.parent = ...`, but that doesn't exist yet. (I guess that would go on WrappedPtrOperations<SavedFrame::Lookup, W>?)

I do sort of wonder if we could somehow make operator[] return a Handle<Lookup*> instead of a Handle<Lookup>, but maybe that's too weird.

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Steve Fink [:sfink] [:s:] from comment #10)
Thanks for the feedback, this clears the questions in my mind.

I do sort of wonder if we could somehow make operator[] return a
Handle<Lookup*> instead of a Handle<Lookup>, but maybe that's too weird.

So far I am not sure when to use Rooted<T> or Rooted<T*>
but from https://searchfox.org/mozilla-central/rev/38035ee92463c9e9fbca729ac7e66476ac7eb27a/js/public/TraceKind.h#166
If we want to use Rooted<T*>, T will need to have 'kind' (JS::TraceKind) member, that means we have to define a 'TraceKind' for Lookup.
Not sure if adding a TraceKind for Lookup makes sense, but if it does, I'll try to do this and this will help my patch simpler.

Comment 12

[Steve Fink [:sfink] [:s:]]

(We discussed this in our meeting, and decided that Handle<Lookup*> is a bad idea because it doesn't match Rooted<Lookup>. It was probably a bad idea anyway.)

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 9043915 [details] [diff] [review]
Part 2: Use Rooted<GCVector<T>> for lookups

Hi Steve
This is the same patch you feedback+'ed last week, since we don't have to use Rooted<Lookup*>, then I have nothing to update.

If you think Jonco should review it (since he reviewed the other two) feel free to let me know.

Comment 14

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9043915 [details] [diff] [review]
Part 2: Use Rooted<GCVector<T>> for lookups

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

::: js/src/vm/SavedStacks.cpp
@@ +1571,5 @@
>  
>  SavedFrame* SavedStacks::getOrCreateSavedFrame(
> +    JSContext* cx, SavedFrame::Lookup& lookup) {
> +  //TODO
> +  const SavedFrame::Lookup& lookupInstance = lookup;

I think you can replace lookupInstance with lookup now?

Comment 15

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9043915 [details] [diff] [review]
Part 2: Use Rooted<GCVector<T>> for lookups

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

::: js/src/vm/SavedStacks.cpp
@@ +1596,1 @@
>    RootedSavedFrame frame(cx, SavedFrame::create(cx));

Oh, wait. This looks like a rooting hazard, which won't show up in the main report so it won't turn the job red. (It will show up as an "unsafe reference" in the caller, hopefully -- though not definitely, there's a bug about that.)

Is it possible to keep this a Handle?

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: Use Rooted<GCVector> for lookups. v2

Addressed comment 15 to use Handle<Lookup>
One problem (left as a TODO) in this patch is GCVector.operator[] will return a [Mutable]Handle<T>, whereas GCVector.back() will return T&

operator[]
https://searchfox.org/mozilla-central/source/js/public/GCVector.h#173
https://searchfox.org/mozilla-central/source/js/public/GCVector.h#197
https://searchfox.org/mozilla-central/source/js/public/GCVector.h#200

back()
https://searchfox.org/mozilla-central/source/js/public/GCVector.h#170
https://searchfox.org/mozilla-central/source/js/public/GCVector.h#195

and these two will be used in
https://searchfox.org/mozilla-central/source/js/src/vm/SavedStacks.cpp#1486
https://searchfox.org/mozilla-central/source/js/src/vm/SavedStacks.cpp#1504

trying to figure out how to fix this.

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: Use Rooted<GCVector> for lookups. v2

Comment 18

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: Use Rooted<GCVector> for AutoLookupVector v2

Hi Steve
Jonco suggested me to use WrappedPtrOperation to get rid of get(), also try to use Rooted<GCVector> for AutoLookupVector, could you help to review this again?

Thanks

Comment 19

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9046837 [details] [diff] [review]
Part 2: Use Rooted<GCVector> for AutoLookupVector v2

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

This is fine, but do you need to declare AutoLookupVector in SavedFrame.h at all? It seems like leaving ASYNC_STACK_MAX_FRAME_COUNT in the .cpp file and doing the `using AutoLookupVector = Rooted<GCVector<Lookup, ASYNC_STACK_MAX_FRAME_COUNT>>` there would be a little cleaner. Or you could even get rid of it entirely and replace it with Rooted<GCVector<Lookup, ASYNC_STACK_MAX_FRAME_COUNT>> in the 3 places it occurs, but I guess that's kind of ugly. What I thought you were going to do was something like

    using GCLookupVector = GCVector<Lookup, ASYNC_STACK_MAX_FRAME_COUNT>;

and then using `Rooted<GCLookupVector>` in place of AutoLookupVector. But I reviewed it with AutoLookupVector still there, and didn't notice, which was a mistake. Sorry about that.

I think my preference would probably be `Rooted<GCLookupVector>`. `AutoLookupVector` hides the Rooted nature of it, and doesn't add anything. If you agree, I'm happy to re-review. (If you disagree, you can just land this patch.)

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2.1: use GCLookupVector

Hi Steve
Your comment 19 is a good idea, I have addressed them (also with some nits I found), can you review this?
I'll merge this with Part 2 once they both get r+.

Thanks

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: Use Rooted<GCVector> for AutoLookupVector v3

merge two patches

Comment 22

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: Use Rooted<GCVector> for AutoLookupVector v3

rebase after https://hg.mozilla.org/mozilla-central/rev/37828dbf6749

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a0de0939e2f
P1: Use Rooted for AutoRooterGetterSetter::Inner. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba49b929457a
Part 2: Use Rooted<GCVector> for AutoLookupVector. r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/9685cdc4b7cb
P3: Use Rooted for RootedCount. r=jonco

Comment 24

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/7a0de0939e2f
https://hg.mozilla.org/mozilla-central/rev/ba49b929457a
https://hg.mozilla.org/mozilla-central/rev/9685cdc4b7cb