1521732 - Rationalise use of AutoValueVector and associated types

Status: RESOLVED FIXED

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

Description

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

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

Comment 1

[Jan de Mooij [:jandem]]

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

Comment 2

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

Attachment: Part 1: StackGCVector, and [Rooted|Handle|MutableHandle]Vector

Comment 3

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

Attachment: Part 2: Convert to HandleValueArray explicitly.

Comment 4

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

Attachment: Part 3: Use Rooted.*Vector in Codegen.py

Comment 5

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

Attachment: Part 4: fix SM rust binging test

Comment 6

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

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.

Comment 7

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

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

Comment 8

[Jon Coppeard (:jonco)]

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.

Comment 9

[Jon Coppeard (:jonco)]

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.

Comment 10

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

Attachment: Bug 1521732 - Part 1: StackGCVector and RootedVector.

Comment 11

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

Attachment: Bug 1521732 - Part 2: convert RootedValueVector to HandleValueArray.

Depends on D23182

Comment 12

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

Attachment: Bug 1521732 - Part 3: Use RootedValueVector instead.

Depends on D23183

Comment 13

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

Attachment: Bug 1521732 - Part 4: fix SM rust binging test.

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

Depends on D23184

Comment 14

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

Attachment: Bug 1521732 - Part 5: RootedObjectVector.

Depends on D23185

Comment 15

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

Attachment: Bug 1521732 - Part 6: rust binding for RootedObjectVector.

Depends on D23186

Comment 16

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

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

Comment 17

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

Attachment: Bug 1521732 - Part 7: remove AutoValueVector and AutoObjectVector.

Depends on D23187

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/adce0075257a
Part 1: StackGCVector and RootedVector. r=sfink

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e705c899efe9
Part 2: convert RootedValueVector to HandleValueArray. r=sfink,jonco

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/66414629b2e3
Part 3: Use RootedValueVector instead. r=sfink,jonco

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b48232ea7b09
Part 4: fix SM rust binging test. r=fitzgen

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9e33009abc2a
Part 5: RootedObjectVector. r=sfink,jonco

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c6e36a52f326
Part 6: rust binding for RootedObjectVector. r=sfink

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e54fd8872b4
Part 7: remove AutoValueVector and AutoObjectVector. r=sfink

Comment 25

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/adce0075257a
https://hg.mozilla.org/mozilla-central/rev/e705c899efe9
https://hg.mozilla.org/mozilla-central/rev/66414629b2e3
https://hg.mozilla.org/mozilla-central/rev/b48232ea7b09
https://hg.mozilla.org/mozilla-central/rev/9e33009abc2a
https://hg.mozilla.org/mozilla-central/rev/c6e36a52f326
https://hg.mozilla.org/mozilla-central/rev/5e54fd8872b4