Remove use of CustomAutoRooter and replace with Rooted in js/

RESOLVED FIXED in Firefox 67

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: jonco, Assigned: allstars.chh)

Tracking

({triage-deferred})

unspecified
mozilla67
Points:
---

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(3 attachments, 7 obsolete attachments)

3.28 KB, patch
jonco
: review+
Details | Diff | Splinter Review
2.25 KB, patch
jonco
: review+
Details | Diff | Splinter Review
19.41 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Reporter)

Comment 1

3 years ago
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.
Keywords: triage-deferred
Priority: -- → P3
Assignee: nobody → allstars.chh
Status: NEW → ASSIGNED

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.

Keywords: meta
Summary: [meta] Remove use of CustomAutoRooter and replace with Rooted → Remove use of CustomAutoRooter and replace with Rooted in js/
Attachment #9042453 - Flags: feedback?(jcoppeard)
Attachment #9042454 - Flags: feedback?(jcoppeard)
Attachment #9042455 - Flags: feedback?(jcoppeard)
(Reporter)

Comment 6

3 months ago
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.
Attachment #9042453 - Flags: feedback?(jcoppeard) → review+
(Reporter)

Comment 7

3 months ago
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.
Attachment #9042454 - Flags: feedback?(jcoppeard)
(Reporter)

Comment 8

3 months ago
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.
Attachment #9042455 - Flags: feedback?(jcoppeard) → review+

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

Attachment #9042454 - Attachment is obsolete: true
Attachment #9043915 - Flags: feedback?(sphink)
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.
Attachment #9043915 - Flags: feedback?(sphink) → feedback+

(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.

(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 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.
Attachment #9043915 - Attachment description: WIP Part 2: Use Rooted<GCVector<T>> for lookups → Part 2: Use Rooted<GCVector<T>> for lookups
Attachment #9043915 - Flags: review?(sphink)
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?
Attachment #9043915 - Flags: review?(sphink) → review+
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?
Attachment #9043915 - Flags: review+

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

Attachment #9046657 - Attachment is obsolete: true
Attachment #9046837 - Flags: review?(sphink)
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.)
Attachment #9046837 - Flags: review?(sphink) → review+
Posted patch Part 2.1: use GCLookupVector (obsolete) — Splinter Review

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

Attachment #9047046 - Flags: review?(sphink)
Attachment #9047046 - Flags: review?(sphink) → review+

merge two patches

Attachment #9046837 - Attachment is obsolete: true
Attachment #9047046 - Attachment is obsolete: true
Attachment #9047171 - Flags: review+

Comment 23

3 months ago
Pushed by allstars.chh@gmail.com:
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

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.