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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

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 1

[Till Schneidereit [:till]]

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

Comment 2

[Andy Wingo [:wingo]]

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.)

Comment 3

[Jason Orendorff [:jorendorff]]

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.

Comment 4

[Till Schneidereit [:till]]

Attachment: v2, now with some heavy refactoring

Comment 5

[Jason Orendorff [:jorendorff]]

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. :-\

Comment 6

[Till Schneidereit [:till]]

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.

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/98034be3508b