Rationalise use of AutoValueVector and associated types

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P3
normal
RESOLVED FIXED
7 months ago
5 months ago

People

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

Tracking

(Blocks 1 bug)

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(7 attachments, 4 obsolete attachments)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
sfink
: feedback+
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

From Jonco:
"More explanation: AutoValueVector is defined as AutoVector<Value> which is basically a Rooted<GCVector<Value>>. We pass these around as references in the API (from what I remember). We should move away from this style to using Rooted<>, Handle<>, etc like we do for everything else, because it's simpler and less confusing.
One complication: AutoVector sets an inline size of 8, which is good if this is used on the stack but not so much for the heap. So maybe we do need a StackGCVector concept or something."

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

One complication: AutoVector sets an inline size of 8, which is good if this is used on the stack but not so much for the heap. So maybe we do need a StackGCVector concept or something."

I think this may be a good idea. I've added inline capacity to some Vectors that I suspect were converted from AutoValueVector to Rooted<GCVector> without realizing the inline size difference.

Status: NEW → ASSIGNED
Comment on attachment 9048872 [details] [diff] [review]
Part 1: StackGCVector, and [Rooted|Handle|MutableHandle]Vector

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

Hi Jonco
Could you help to check is this what you have in mind?
So far I've only done for Value, will add another Vectors for jsid and JS::Object in other patches, and remove AutoVector if neccesary.
Attachment #9048872 - Flags: feedback?(jcoppeard)
Comment on attachment 9048873 [details] [diff] [review]
Part 2: Convert to HandleValueArray explicitly.

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

Hi Jonco
Can you check converting explicitly to HandleValueArray is okay?

Thanks
Attachment #9048873 - Flags: feedback?(jcoppeard)
Comment on attachment 9048872 [details] [diff] [review]
Part 1: StackGCVector, and [Rooted|Handle|MutableHandle]Vector

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

Yes, that's exactly what I had in mind!  Thanks.

::: js/public/GCVector.h
@@ +153,5 @@
>  };
>  
> +template <typename T, typename AllocPolicy = js::TempAllocPolicy>
> +class MOZ_STACK_CLASS StackGCVector : public GCVector<T, 8, AllocPolicy> {
> +  using GCVector<T, 8, AllocPolicy>::GCVector;

Oh, you can do this? TIL.

@@ +157,5 @@
> +  using GCVector<T, 8, AllocPolicy>::GCVector;
> +};
> +
> +using HandleValueVector = Handle<StackGCVector<Value>>;
> +using MutableHandleValueVector = MutableHandle<StackGCVector<Value>>;

I think these definitions should live somewhere else, not in GCVector.h.

::: js/src/builtin/Object.cpp
@@ +1328,5 @@
>    }
>  
>    *optimized = true;
>  
> +  JS::RootedValueVector properties(cx);

We should alias JS::RootedValueVector into the js namespace like we do for RootedValue and everything else in js/src/NamespaceImports.h.
Attachment #9048872 - Flags: feedback?(jcoppeard) → feedback+
Comment on attachment 9048873 [details] [diff] [review]
Part 2: Convert to HandleValueArray explicitly.

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

Can you give HandleValueArray a new constructor (like we have for AutoValueVector) so this can convert implicitly?

Otherwise it looks fine.
Attachment #9048873 - Flags: feedback?(jcoppeard)

StackGCVector is a partial template specialization and causes cargo test
failure. Bypass the class by adding it to OPAQUE_TYPES.

Depends on D23184

Attachment #9048872 - Attachment is obsolete: true
Attachment #9048873 - Attachment is obsolete: true
Attachment #9048874 - Attachment is obsolete: true
Attachment #9048875 - Attachment is obsolete: true

Comment on attachment 9050391 [details]
Bug 1521732 - Part 6: rust binding for RootedObjectVector.

Hi Steve
Can you help to check this patch?
In Part 1 I added a MOZ_STACK_CLASS StackGCVector, and then I try to convert AutoObjetVector to RootedObjectVector in Part 5, however I am not sure for the rust binding code, as it call 'new' a Rooted type, not sure if this okay?

Thanks

Attachment #9050391 - Flags: feedback?(sphink)
Attachment #9050391 - Flags: feedback?(sphink) → feedback+
Attachment #9050386 - Attachment description: Bug 1521732 - Part 1: StackGCVector and its Rooted class. → Bug 1521732 - Part 1: StackGCVector and its Rooted class. r=sfink
Attachment #9050387 - Attachment description: Bug 1521732 - Part 2: convert RootedValueVector to HandleValueArray → Bug 1521732 - Part 2: convert RootedValueVector to HandleValueArray. r=sfink
Attachment #9050388 - Attachment description: Bug 1521732 - Part 3: Use RootedValueVector instead → Bug 1521732 - Part 3: Use RootedValueVector instead. r=sfink
Attachment #9050389 - Attachment description: Bug 1521732 - Part 4: fix SM rust binging test → Bug 1521732 - Part 4: fix SM rust binging test. r=fitzgen
Attachment #9050390 - Attachment description: Bug 1521732 - Part 5: RootedObjectVector → Bug 1521732 - Part 5: RootedObjectVector. r=sfink
Attachment #9050391 - Attachment description: Bug 1521732 - Part 6: rust binding for RootedObjectVector → Bug 1521732 - Part 6: rust binding for RootedObjectVector. r=sfink
Attachment #9050386 - Attachment description: Bug 1521732 - Part 1: StackGCVector and its Rooted class. r=sfink → Bug 1521732 - Part 1: StackGCVector and its Rooted class. r?sfink
Attachment #9050387 - Attachment description: Bug 1521732 - Part 2: convert RootedValueVector to HandleValueArray. r=sfink → Bug 1521732 - Part 2: convert RootedValueVector to HandleValueArray. r?sfink
Attachment #9050388 - Attachment description: Bug 1521732 - Part 3: Use RootedValueVector instead. r=sfink → Bug 1521732 - Part 3: Use RootedValueVector instead. r?sfink
Attachment #9050389 - Attachment description: Bug 1521732 - Part 4: fix SM rust binging test. r=fitzgen → Bug 1521732 - Part 4: fix SM rust binging test. r?fitzgen
Attachment #9050390 - Attachment description: Bug 1521732 - Part 5: RootedObjectVector. r=sfink → Bug 1521732 - Part 5: RootedObjectVector. r?sfink
Attachment #9050391 - Attachment description: Bug 1521732 - Part 6: rust binding for RootedObjectVector. r=sfink → Bug 1521732 - Part 6: rust binding for RootedObjectVector. r?sfink
Attachment #9050759 - Attachment description: Bug 1521732 - Part 7: remove AutoValueVector and AutoObjectVector. r=sfink → Bug 1521732 - Part 7: remove AutoValueVector and AutoObjectVector. r?sfink
Attachment #9050759 - Attachment description: Bug 1521732 - Part 7: remove AutoValueVector and AutoObjectVector. r?sfink → Bug 1521732 - Part 7: remove AutoValueVector and AutoObjectVector.
Attachment #9050386 - Attachment description: Bug 1521732 - Part 1: StackGCVector and its Rooted class. r?sfink → Bug 1521732 - Part 1: StackGCVector and its Rooted class.
Attachment #9050387 - Attachment description: Bug 1521732 - Part 2: convert RootedValueVector to HandleValueArray. r?sfink → Bug 1521732 - Part 2: convert RootedValueVector to HandleValueArray.
Attachment #9050388 - Attachment description: Bug 1521732 - Part 3: Use RootedValueVector instead. r?sfink → Bug 1521732 - Part 3: Use RootedValueVector instead.
Attachment #9050389 - Attachment description: Bug 1521732 - Part 4: fix SM rust binging test. r?fitzgen → Bug 1521732 - Part 4: fix SM rust binging test.
Attachment #9050390 - Attachment description: Bug 1521732 - Part 5: RootedObjectVector. r?sfink → Bug 1521732 - Part 5: RootedObjectVector.
Attachment #9050391 - Attachment description: Bug 1521732 - Part 6: rust binding for RootedObjectVector. r?sfink → Bug 1521732 - Part 6: rust binding for RootedObjectVector.
Attachment #9050390 - Attachment description: Bug 1521732 - Part 5: RootedObjectVector. → Bug 1521732 - Part 5: RootedObjectVector. r?sfink
Attachment #9050391 - Attachment description: Bug 1521732 - Part 6: rust binding for RootedObjectVector. → Bug 1521732 - Part 6: rust binding for RootedObjectVector. r?sfink
Attachment #9050759 - Attachment description: Bug 1521732 - Part 7: remove AutoValueVector and AutoObjectVector. → Bug 1521732 - Part 7: remove AutoValueVector and AutoObjectVector. r?sfink
Attachment #9050386 - Attachment description: Bug 1521732 - Part 1: StackGCVector and its Rooted class. → Bug 1521732 - Part 1: StackGCVector and its Rooted class. r?sfink
Attachment #9050387 - Attachment description: Bug 1521732 - Part 2: convert RootedValueVector to HandleValueArray. → Bug 1521732 - Part 2: convert RootedValueVector to HandleValueArray. r?sfink
Attachment #9050388 - Attachment description: Bug 1521732 - Part 3: Use RootedValueVector instead. → Bug 1521732 - Part 3: Use RootedValueVector instead. r?sfink
Attachment #9050389 - Attachment description: Bug 1521732 - Part 4: fix SM rust binging test. → Bug 1521732 - Part 4: fix SM rust binging test. r?fitzgen
Attachment #9050759 - Attachment is obsolete: true
Attachment #9050390 - Attachment is obsolete: true
Attachment #9050389 - Attachment is obsolete: true
Attachment #9050388 - Attachment is obsolete: true
Attachment #9050387 - Attachment is obsolete: true
Attachment #9050386 - Attachment is obsolete: true
Attachment #9050386 - Attachment is obsolete: false
Attachment #9050387 - Attachment is obsolete: false
Attachment #9050388 - Attachment is obsolete: false
Attachment #9050389 - Attachment is obsolete: false
Attachment #9050390 - Attachment is obsolete: false
Attachment #9050759 - Attachment is obsolete: false
Attachment #9050386 - Attachment description: Bug 1521732 - Part 1: StackGCVector and its Rooted class. r?sfink → Bug 1521732 - Part 1: StackGCVector and RootedVector. r=sfink
Attachment #9050387 - Attachment description: Bug 1521732 - Part 2: convert RootedValueVector to HandleValueArray. r?sfink → Bug 1521732 - Part 2: convert RootedValueVector to HandleValueArray. r=sfink
Attachment #9050388 - Attachment description: Bug 1521732 - Part 3: Use RootedValueVector instead. r?sfink → Bug 1521732 - Part 3: Use RootedValueVector instead. r?jonco
Attachment #9050389 - Attachment description: Bug 1521732 - Part 4: fix SM rust binging test. r?fitzgen → Bug 1521732 - Part 4: fix SM rust binging test. r=fitzgen
Attachment #9050390 - Attachment description: Bug 1521732 - Part 5: RootedObjectVector. r?sfink → Bug 1521732 - Part 5: RootedObjectVector. r?jonco
Attachment #9050391 - Attachment description: Bug 1521732 - Part 6: rust binding for RootedObjectVector. r?sfink → Bug 1521732 - Part 6: rust binding for RootedObjectVector. r=sfink
Attachment #9050759 - Attachment description: Bug 1521732 - Part 7: remove AutoValueVector and AutoObjectVector. r?sfink → Bug 1521732 - Part 7: remove AutoValueVector and AutoObjectVector. r=sfink
Attachment #9050386 - Attachment description: Bug 1521732 - Part 1: StackGCVector and RootedVector. r=sfink → Bug 1521732 - Part 1: StackGCVector and RootedVector.
Attachment #9050387 - Attachment description: Bug 1521732 - Part 2: convert RootedValueVector to HandleValueArray. r=sfink → Bug 1521732 - Part 2: convert RootedValueVector to HandleValueArray.
Attachment #9050388 - Attachment description: Bug 1521732 - Part 3: Use RootedValueVector instead. r?jonco → Bug 1521732 - Part 3: Use RootedValueVector instead.
Attachment #9050389 - Attachment description: Bug 1521732 - Part 4: fix SM rust binging test. r=fitzgen → Bug 1521732 - Part 4: fix SM rust binging test.
Attachment #9050390 - Attachment description: Bug 1521732 - Part 5: RootedObjectVector. r?jonco → Bug 1521732 - Part 5: RootedObjectVector.
Attachment #9050391 - Attachment description: Bug 1521732 - Part 6: rust binding for RootedObjectVector. r=sfink → Bug 1521732 - Part 6: rust binding for RootedObjectVector.
Attachment #9050759 - Attachment description: Bug 1521732 - Part 7: remove AutoValueVector and AutoObjectVector. r=sfink → Bug 1521732 - Part 7: remove AutoValueVector and AutoObjectVector.
Attachment #9050387 - Attachment is obsolete: true
Attachment #9050387 - Attachment is obsolete: false
Pushed by allstars.chh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/adce0075257a
Part 1: StackGCVector and RootedVector. r=sfink
Pushed by allstars.chh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e705c899efe9
Part 2: convert RootedValueVector to HandleValueArray. r=sfink,jonco
Pushed by allstars.chh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/66414629b2e3
Part 3: Use RootedValueVector instead. r=sfink,jonco
Pushed by allstars.chh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b48232ea7b09
Part 4: fix SM rust binging test. r=fitzgen
Pushed by allstars.chh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9e33009abc2a
Part 5: RootedObjectVector. r=sfink,jonco
Pushed by allstars.chh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c6e36a52f326
Part 6: rust binding for RootedObjectVector. r=sfink
Pushed by allstars.chh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5e54fd8872b4
Part 7: remove AutoValueVector and AutoObjectVector. r=sfink
You need to log in before you can comment on or make changes to this bug.