Make multiple methods using the same self-hosted function reuse one cloned version instead of replacing previous ones in the intrinsics holder.

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: till, Assigned: till)

Tracking

unspecified
mozilla27
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

6 years ago
This adds a check in JS_DefineFunctions for self-hosted functions to only create a new function if none exists with the same self-hosted name in the intrinsics holder, already.

The outcome is that we don't just overwrite the old function, but install it, identity-preserving, on the current object. So if, say Map and Set both install a function "entries" and use the same self-hosting name for it, `myMap.entries === mySet.entries` will hold.
Comment on attachment 807753 [details] [diff] [review]
Make multiple methods using the same self-hosted function reuse one cloned version instead of replacing previous ones in the intrinsics holder.

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

LGTM, thank you very much!  (See bug 907077 comment 33 for the problem I was running into without this patch.  I had attempted to do this very same thing but somehow forgot that at this point we were in a loop; how embarrassing!  Thank you for handling this.)
Attachment #807753 - Flags: review?(wingo) → review+
Assignee

Updated

6 years ago
Blocks: 907077
Comment on attachment 807753 [details] [diff] [review]
Make multiple methods using the same self-hosted function reuse one cloned version instead of replacing previous ones in the intrinsics holder.

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

My only comment is that this code looks like it belongs in GlobalObject.cpp or SelfHosting.cpp, just for the sake of loose coupling. The code in JS_DefineFunctions should read like:

        if (fs->selfHostedName) {
            /*
             * During creation of the self-hosting global, we ignore all
             * self-hosted functions, as that means we're currently setting up
             * the global object that the self-hosted code is then compiled
             * in. Self-hosted functions can access each other via their names,
             * but not via the builtin classes they get installed into.
             */
            if (cx->runtime()->isSelfHostingGlobal(cx->global()))
                continue;

            if (!cx->global()->defineSelfHostedFunction(cx, obj, id, fs))
                return false;
        } else {
            JSFunction *fun = DefineFunction(cx, obj, id, fs->call.op, fs->nargs, flags);
            if (!fun)
                return false;
            if (fs->call.info)
                fun->setJitInfo(fs->call.info);
        }

Actually it'd be nice to factor out the Define part from the rest. As always, you decide how much to bite off today.

Btw, could you do me a favor and fix this typo in vm/Interpreter.cpp?

> BEGIN_CASE(JSOP_BINDINTRINSIC)
>     PUSH_OBJECT(*cx->global()->intrinsicsHolder());
> END_CASE(JSOP_BINDGNAME)

The third line should say BINDINTRINSIC, not BINDGNAME.
Attachment #807753 - Flags: review?(jorendorff) → review+
Assignee

Comment 4

6 years ago
Attachment #807808 - Flags: review?(jorendorff)
Assignee

Updated

6 years ago
Attachment #807753 - Attachment is obsolete: true
Comment on attachment 807808 [details] [diff] [review]
v2, now with some heavy refactoring

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

Thanks. r=me.

::: js/src/jsapi.cpp
@@ +4246,2 @@
>                  return false;
> +            if (!JSObject::defineGeneric(cx, obj, id, funVal, NULL, NULL, 0))

It's a little funny passing 0 here instead of flags, but I guess that's what we were doing before. :-\
Attachment #807808 - Flags: review?(jorendorff) → review+
Assignee

Comment 6

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/98034be3508b

Thanks for the quick review!

> ::: js/src/jsapi.cpp
> @@ +4246,2 @@
> >                  return false;
> > +            if (!JSObject::defineGeneric(cx, obj, id, funVal, NULL, NULL, 0))
> 
> It's a little funny passing 0 here instead of flags, but I guess that's what
> we were doing before. :-\

Yep, I checked this all through and it's behavior-preserving.
https://hg.mozilla.org/mozilla-central/rev/98034be3508b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

Updated

6 years ago
No longer blocks: 907077
You need to log in before you can comment on or make changes to this bug.