Closed Bug 892971 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: shu, Assigned: shu)

Details

Attachments

(2 files, 1 obsolete file)

Should have one consistent scheme of naming parallel MIR/LIR/VM functions.
Assignee: general → shu
Attachment #774711 - Flags: review?(nicolas.b.pierron)
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.
Attachment #774711 - Flags: review?(nicolas.b.pierron)
(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.
Addressed comments. Renamed to multimodal since that seems much more prevalent in the wild than polymodal.
Attachment #774711 - Attachment is obsolete: true
Attachment #775745 - Flags: review?(nicolas.b.pierron)
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.
Attachment #775745 - Flags: review?(nicolas.b.pierron) → review+
Part 2 also renames parSlice to forkJoinSlice
Attachment #775923 - Flags: review?(nmatsakis) → review+
https://hg.mozilla.org/mozilla-central/rev/150dd1ab14f6
https://hg.mozilla.org/mozilla-central/rev/37d32a10aed3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.