892971 - PJS: Clean up a bit by making naming consistent and calling VM functions less painful

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

Should have one consistent scheme of naming parallel MIR/LIR/VM functions.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Part 1: Refactor calling polymodal VM functions

Comment 2

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 774711 [details] [diff] [review]
Part 1: Refactor calling polymodal VM functions

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

I like the refactoring of the code from the user perspective, still, using the operator comma where a constructor can be used would be easier to read and understand.

::: js/src/ion/CodeGenerator.cpp
@@ +575,4 @@
>  typedef ParallelResult (*ParallelIntToStringFn)(ForkJoinSlice *, int, MutableHandleString);
> +static const VMFunctionPolymodal IntToStringInfo =
> +    (FunctionInfo<IntToStringFn>(Int32ToString<CanGC>),
> +     FunctionInfo<ParallelIntToStringFn>(ParIntToString));

nit:

static const VMFunctionPolymodal IntToStringInfo(
    FunctionInfo<IntToStringFn>(Int32ToString<CanGC>),
    FunctionInfo<ParallelIntToStringFn>(ParIntToString));

::: js/src/ion/VMFunctions.h
@@ +256,5 @@
> +struct VMFunctionPolymodal
> +{
> +    VMFunctionPolymodal(const FunctionInfos &infos) {
> +        for (unsigned i = 0; i < NumExecutionModes; i++)
> +            funs_[i].init(*infos.funps[i]);

I do not see the need for FunctionInfos class, the storage can be made part of VMFunctionPolymodal class only, and multiple constructors can be used to add 1 or 2 VM functions to it.

In addition this will remove the copy of the array, knowing that both arrays would be computed during the start-up.

Comment 3

[Shu-yu Guo [:shu]]

(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> Comment on attachment 774711 [details] [diff] [review]
> Part 1: Refactor calling polymodal VM functions
> 
> Review of attachment 774711 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like the refactoring of the code from the user perspective, still, using
> the operator comma where a constructor can be used would be easier to read
> and understand.
> 
> ::: js/src/ion/CodeGenerator.cpp
> @@ +575,4 @@
> >  typedef ParallelResult (*ParallelIntToStringFn)(ForkJoinSlice *, int, MutableHandleString);
> > +static const VMFunctionPolymodal IntToStringInfo =
> > +    (FunctionInfo<IntToStringFn>(Int32ToString<CanGC>),
> > +     FunctionInfo<ParallelIntToStringFn>(ParIntToString));
> 
> nit:
> 
> static const VMFunctionPolymodal IntToStringInfo(
>     FunctionInfo<IntToStringFn>(Int32ToString<CanGC>),
>     FunctionInfo<ParallelIntToStringFn>(ParIntToString));
> 
> ::: js/src/ion/VMFunctions.h
> @@ +256,5 @@
> > +struct VMFunctionPolymodal
> > +{
> > +    VMFunctionPolymodal(const FunctionInfos &infos) {
> > +        for (unsigned i = 0; i < NumExecutionModes; i++)
> > +            funs_[i].init(*infos.funps[i]);
> 
> I do not see the need for FunctionInfos class, the storage can be made part
> of VMFunctionPolymodal class only, and multiple constructors can be used to
> add 1 or 2 VM functions to it.
> 
> In addition this will remove the copy of the array, knowing that both arrays
> would be computed during the start-up.

Fair enough about using the constructor. I had wanted to maintain the syntactic similarity to assigning to a single VMFunction.

Comment 4

[Shu-yu Guo [:shu]]

Attachment: Part 1: Refactor calling multimodal VM functions

Addressed comments. Renamed to multimodal since that seems much more prevalent in the wild than polymodal.

Comment 5

[Nicolas B. Pierron [:nbp] {away}]

Comment on attachment 775745 [details] [diff] [review]
Part 1: Refactor calling multimodal VM functions

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

::: js/src/ion/VMFunctions.h
@@ +236,5 @@
>      void addToFunctions();
>  };
>  
> +// A collection of VM functions for each execution mode.
> +struct VMFunctionMultimodal

As discussed over IRC, Multimodals is "heavy", and "VMFunctionsModal" might be a better solution.

Comment 6

[Shu-yu Guo [:shu]]

Attachment: Part 2: Unify parallel names with the 'Parallel' suffix

Comment 7

[Shu-yu Guo [:shu]]

Part 2 also renames parSlice to forkJoinSlice

Comment 8

[Shu-yu Guo [:shu]]

https://tbpl.mozilla.org/?tree=Try&rev=3d1b143df106

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/150dd1ab14f6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/37d32a10aed3

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/150dd1ab14f6
https://hg.mozilla.org/mozilla-central/rev/37d32a10aed3