894092 - IonMonkey: clean-up: Remove VMFunction copy constructor.

Status: RESOLVED DUPLICATE of bug 905210

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

Currently every VMFunction is created with a copy constructor, such as:

typedef JSObject *(*LambdaFn)(JSContext *, HandleFunction, HandleObject);
static const VMFunction LambdaInfo = FunctionInfo<LambdaFn>(js::Lambda);

A FunctionInfo<…> is already a VMFunction, and there is no need to make a copy of the FunctionInfo at the start-up.  Instead we should write something such as:

typedef JSObject *(*LambdaFn)(JSContext *, HandleFunction, HandleObject);
static const FunctionInfo<LambdaFn> LambdaInfo(js::Lambda);

And hide the copy constructor of VMFunction.  This should also be applied on VMFunctionsModal, by storing a VMFunction pointer instead of a copy.

Comment 1

[Shu-yu Guo [:shu]]

There are several problems with this, I believe.

> typedef JSObject *(*LambdaFn)(JSContext *, HandleFunction, HandleObject);
> static const FunctionInfo<LambdaFn> LambdaInfo(js::Lambda);

You can't actually write that, because that's ambiguous with function decls. You end up having to write the following, which is more typing:

  static const FunctionInfo<LambdaFn> LambdaInfo = FunctionInfo<LambdaFn>(js::Lambda);

> This should also be applied on VMFunctionsModal, by storing a VMFunction pointer instead of a copy.

I'm not sure that'll work, if I understand your intention correctly. If I have this (A is, say, FunctionInfo<T> and B is VMFunctionsModal):

struct A {
   A() { ... }
  ~A() { ... }
};

struct B {
   B(const A &b) { ... }
  ~B() { ... }
};

static B = B(A());

int main() { ... }

I think the order of execution will be: B() A() ~B(), main(), ~A()

That is, we can't really save a pointer to A.

All in all, I'm not sure this is worth it.

Comment 2

[Shu-yu Guo [:shu]]

Err, the above order should be:

A() B() ~A(), main(), ~B()

Comment 3

[Snigdha Agarwal]

Hey,

I would like to contribute to this bug.
However, I am a beginner, so it would be great if someone could guide me on how to start.

Comment 4

[Nicolas B. Pierron [:nbp]]

This code involved in this bug implies that you know well the C++ and Macro as well as templates.  Are you sure you want to work on it?

The logic is in js/src/jit/VMFunction.h [1]

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/VMFunctions.h

Comment 5

[Snigdha Agarwal]

Yes, I am well acquainted with those areas.

Comment 6

[Snigdha Agarwal]

Attachment: (WIP) Removed most of VMFunction errors

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8373985 [details] [diff] [review]
(WIP) Removed most of  VMFunction errors

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

This looks good, pay attention with the trailing white-spaces in your patch, as well as when you insert/remove lines.

::: dom/tests/mochitest/gamepad/test_gamepad_connect_events.html
@@ +53,3 @@
>  
>  </script>
> +<!--<iframe id="f1" src="gamepad_frame.html" onload="frame_loaded()"></iframe>-->

Why did you modified this test case?  Personaly, I never run the mochitest, as this always implies building the browser, I always check with the JavaScript shell.

See https://developer.mozilla.org/en-US/docs/SpiderMonkey/Build_Documentation#Test

::: js/src/jit/BaselineCompiler.cpp
@@ +510,1 @@
>  typedef bool (*DebugPrologueFn)(JSContext *, BaselineFrame *, jsbytecode *, bool *);

nit: re-add an empty line between the closing braces and the typedef.

::: js/src/jit/VMFunctions.h
@@ +220,5 @@
>          JS_ASSERT_IF(outParam != Type_Void && executionMode == SequentialExecution,
>                       returnType == Type_Bool);
>          JS_ASSERT(returnType == Type_Bool ||
>                    returnType == Type_Object);
>      }

You will have to add a call to init in this function.

@@ +229,4 @@
>  
>      void init(const VMFunction &o) {
>          JS_ASSERT(!wrapped);
>          *this = o;

and remove this line from the init function, as we no longer need to copy it into it-self.

Comment 8

[Snigdha Agarwal]

The changes in test_gamepad_connect_events.html are related to another bug. By mistake, I made those changes with this patch on top and qref'd them. And now I don't know how to revert it.

How to check for trailing spaces? They don't show up in diff of patch.
I'll submit an updated patch soon, with the above changes you mentioned as well as cleaned up VMFunctionsModal errors.

Comment 9

[Nicolas B. Pierron [:nbp]]

(In reply to snigdhaagarwal93 from comment #8)
> How to check for trailing spaces?

You can open the patch from your queue and configure your editor to highlight them.
With mercurial, you might be able to change the diff program in your hgrc, to configure it to highlight trailing white space.

Comment 10

[Snigdha Agarwal]

Attachment: (WIP) Changes made in VMFunctionsModal structure

I have been trying to modify VMFunctionsModal Class. 

An error I don't understand how to resolve: 
js/src/jit/CodeGenerator.cpp:746:59: error: no matching function for call to ‘js::jit::CodeGenerator::oolCallVM(const js::jit::VMFunctionsModal<JSFlatString* (*)(js::ThreadSafeContext*, int), JSFlatString* (*)(js::ForkJoinSlice*, int)>&, js::jit::LIntToString*&, js::jit::ArgSeq<js::jit::ArgSeq<void, void>, js::jit::Register>, js::jit::StoreRegisterTo)’

in this line : OutOfLineCode *ool = oolCallVM(IntToStringInfo, lir, (ArgList(), input),
                                   StoreRegisterTo(output));

where IntToStringInfo has been defined as : 
static const VMFunctionsModal<IntToStringFn,IntToStringParFn> IntToStringInfo(Int32ToString<CanGC>,IntToStringPar);

All the changes made in VMFunctionsModal and oolCallVM, are present in this patch.

P.S. - Please ignore the comments, or any other discrepancies, as it is still a work in progress.

Comment 11

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8384683 [details] [diff] [review]
(WIP) Changes made in VMFunctionsModal structure

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

::: js/src/jit/shared/CodeGenerator-shared.h
@@ +376,5 @@
>      inline OutOfLineCode *oolCallVM(const VMFunction &fun, LInstruction *ins, const ArgSeq &args,
>                                      const StoreOutputTo &out);
>  
> +    template <class ArgSeq, class StoreOutputTo>
> +    bool callVM(const VMFunctionsModal/*VMFunctionsModal &f*/<ArgSeq, StoreOutputTo> &f, LInstruction *ins, const Register *dynStack = nullptr) {

They are probably not named ArgSeq nor StoreOutput.  Also their type do not match the VMFunctionsModal definition that you wrote in VMFunction.h.

@@ +386,1 @@
>                                      const ArgSeq &args, const StoreOutputTo &out)

Why do you reuse the current template parameters of the function, while you are adding 2 to the VMFunctionsModal ?

Comment 12

[vinayakagarwal6996]

Hello,
Is the bug still available to fix?

Thanks

Comment 13

[Nicolas B. Pierron [:nbp]]

(In reply to vinayakagarwal6996 from comment #12)
> Is the bug still available to fix?

Yes, see my answer in Bug 905210