1463645 - Unnecessary cast between JSObject* and JSFunction* in CreateFunctionPrototype

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Tooru Fujisawa [:arai]]

CreateFunctionPrototype calls NewFunctionWithProto and receives the return value as `JSObject* functionProto_`.

https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/js/src/vm/JSFunction.cpp#838-841
> static JSObject*
> CreateFunctionPrototype(JSContext* cx, JSProtoKey key)
> {
> ...
>     JSObject* functionProto_ =
>         NewFunctionWithProto(cx, nullptr, 0, JSFunction::INTERPRETED,
>                              enclosingEnv, nullptr, objectProto, AllocKind::FUNCTION,
>                              SingletonObject);

NewFunctionWithProto returns JSFunction*

https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/js/src/vm/JSFunction.h#813-817
> extern JSFunction*
> NewFunctionWithProto(JSContext* cx, JSNative native, unsigned nargs,
>                      JSFunction::Flags flags, HandleObject enclosingEnv, HandleAtom atom,
>                      HandleObject proto, gc::AllocKind allocKind = gc::AllocKind::FUNCTION,
>                      NewObjectKind newKind = GenericObject);

where JSFunction is the derived class of JSObject.

https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/js/src/vm/JSFunction.h#41
> class JSFunction : public js::NativeObject

https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/js/src/vm/NativeObject.h#465
> class NativeObject : public ShapedObject

https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/js/src/vm/ShapedObject.h#21
> class ShapedObject : public JSObject

So, the assignment above performs upcast.

and then it downcasts functionProto_ to JSFunction* with `&functionProto_->as<JSFunction>()` call.

https://searchfox.org/mozilla-central/rev/2aa42f2cab4a110edf21dd7281ac23a1ea8901f9/js/src/vm/JSFunction.cpp#845
> static JSObject*
> CreateFunctionPrototype(JSContext* cx, JSProtoKey key)
> {
> ...
>     RootedFunction functionProto(cx, &functionProto_->as<JSFunction>());

So, the upcast and downcast are completely unnecessary, and we can just receive the return value of NewFunctionWithProto into RootedFunction.



You'll need basic knowledge of C++, and be able to build and test Firefox [1] or SpiderMonkey [2].

A skilled first-time contributor should be able to finish this in a month.  Leave comments / questions here, or ask me (:arai) in IRC #introduction and #jsapi channels.

[1] https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation

Comment 1

[maharsh312]

Hi,

This is my first experience with open source community and I would like to work on this bug. Please guide me on how should I begin with and get the code base.

Comment 2

[Tooru Fujisawa [:arai]]

Hi, thank you for your comment :D

please follow this document to setup the development environment (until the step just before running "./mach build"):
  https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

and then follow this document to build SpiderMonkey shell:
  https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation

Comment 3

[maharsh312]

I have found a remove upcast and downcast JSFunction.cpp. Now, which specific test file should I run to check my changes?

Comment 4

[Tooru Fujisawa [:arai]]

Great :)

First, please confirm that you can build with your change applied.

then, you need to run test which executes the code you've touched,
and actually CreateFunctionPrototype is executed while starting the shell, so any test is fine (in jstests, jit-test),
or just run JS shell and confirm that it doesn't crash.

Comment 5

[maharsh312]

As I changed JSObject to JSFunction in "JSFunction.cpp", I was able to remove the upcast. But, I am getting an error during the build if I remove "->as<JSFunction>()" which performs downcast.

Can you help me with this?

Do tell me if something is wrong with my understanding of bug.

Comment 6

[maharsh312]

Attachment: Bug 1463645 - Removed unnecessary upcast and downcast of JSFunction

Comment 7

[Tooru Fujisawa [:arai]]

Comment on attachment 8980536 [details] [diff] [review]
Bug 1463645 - Removed unnecessary upcast and downcast of JSFunction

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

Thank you for your patch :D

Please address the following comments, and then attach the updated patch, and set "review" flag to "?" and choose me as requestee next time!

::: js/src/vm/JSFunction.cpp
@@ +835,4 @@
>       * give it the guts to be one.
>       */
>      RootedObject enclosingEnv(cx, &self->lexicalEnvironment());
> +        

please remove spaces here,
and also please remove newline.

@@ +836,5 @@
>       */
>      RootedObject enclosingEnv(cx, &self->lexicalEnvironment());
> +        
> +    RootedFunction functionProto(cx, NewFunctionWithProto(cx, nullptr, 0, JSFunction::INTERPRETED,
> +                             							  enclosingEnv, nullptr, objectProto, AllocKind::FUNCTION,

please use spaces instead of tabs,
and align "e" of "enclosing" with "c" of "cx" in the above line.

@@ -838,5 @@
> -    JSObject* functionProto_ =
> -        NewFunctionWithProto(cx, nullptr, 0, JSFunction::INTERPRETED,
> -                             enclosingEnv, nullptr, objectProto, AllocKind::FUNCTION,
> -                             SingletonObject);
> -    if (!functionProto_)

please keep this null-check.
this case can happen if Out Of Memory (OOM) happens inside NewFunctionWithProto,
and this is to properly handle such case.

Comment 8

[maharsh312]

Attachment: Bug 1463645- Added a nullcheck and improved the indentation

Comment 9

[Tooru Fujisawa [:arai]]

Comment on attachment 8980649 [details] [diff] [review]
Bug 1463645- Added a nullcheck and improved the indentation

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

Great
Almost there!

Please fold this and the previous patches into single one.

::: js/src/vm/JSFunction.cpp
@@ +838,4 @@
>      RootedFunction functionProto(cx, NewFunctionWithProto(cx, nullptr, 0, JSFunction::INTERPRETED,
> +                                                          enclosingEnv, nullptr, objectProto,
> +                                                          AllocKind::FUNCTION,
> +                                                          SingletonObject));

this can be moved to previous line.
we're using 99-chars per line for code and 80-chars per line for comment.

Comment 10

[maharsh312]

Attachment: Bug 1463645 - Removed unnecessary upcast and downcast of JSFunction (updated)

Comment 11

[Tooru Fujisawa [:arai]]

Comment on attachment 8980731 [details] [diff] [review]
Bug 1463645 - Removed unnecessary upcast and downcast of JSFunction (updated)

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

Excellent!
Thank you for fixing this :D

here's try run for automated testing:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd6605bd336b3094bb2609f26766da9239fc3b2e
once it completes without failure, you can add "checkin-needed" keyword to Tracking-Keywords field above,
(you can set multiple keywords separated by ",")
so that the patch is landed to mozilla-inbound which will be merged to mozilla-central shortly.

also, you can mark previous patches "obsolete" in Details link in attachments,
click "Details" next to attachment, click "edit details", check "obsolete", and "Submit".
so that it's clear which patch is the latest one.

for next time, when you submitting new attachment, you can mark them obsolete in the form, list under "Obsoletes"

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab890e8f2582
Removed unnecessary upcast and downcast of JSFunction r=arai

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/ab890e8f2582