Closed Bug 773850 Opened 8 years ago Closed 8 years ago

Redesign non-generic method guarding API


(Core :: JavaScript Engine, defect)

Not set





(Reporter: Waldo, Assigned: Waldo)



(Whiteboard: [js:t])


(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
The current NonGenericMethodGuard idiom, needed to implement methods which are agnostic to |this| being a wrapper around an actual object, is *very* awkward and doesn't read naturally.  It asks for a JSClass* even when the method in question might need to accept objects (this values, more generally) that are of multiple classes.  It also results in weird function-pointer-passing behaviors that aren't easy to use, or to understand on the implementation side.  It also means that if a primitive this is acceptable, it must be specially handled before the NonGenericMethodGuard idiom is used.

This patch introduces a new system to address this, which will take a test function (that says if a value is an acceptable this) and an implementation function (which will perform the implemented function's semantics, knowing that the this value passed the test).  The actual system itself is pretty small.  However, touching every place that does non-generic method guarding is quite a bit of touching, hence the patch's size.

Ideally I would make these APIs and hooks all use handle types right from the start.  Unfortunately, the thought didn't occur to me until I was mostly done :-( and it'd be a lot of changes to make it work, given that changing the signature of CallReceiver::thisv() affects a *ton* of code.  In a very brief attempt in which I went no further than jsarray.cpp, it looks like one big complaint would be, if args.thisv() were a MutableHandle<Value>, that &args.thisv() would take the address of a temporary.  That could be fixed, to be sure.  But I also leave for vacation next week, and this patch will bitrot, and I have things depending on this which I really really want to land before I leave.  So I'd like to defer this for now and just get it landed; I'm more than willing to add handles to it when I return, when I don't have a sword of Damocles over my head.

There's perhaps some bikeshedding of names here, like maybe s/CallNonGenericMethod/JS_CallNonGenericMethod/g and perhaps some of the typedef names could be different, but other than that I think this is good to go.  I do think we should take this opportunity to use nice C++ types like CallArgs and bool in the interface, tho -- we need to start that transition sometime in new APIs, and now seems a good time to do it.

Longer-range, it occurs to me that perhaps -- perhaps -- we might want to have the JS engine (and maybe even the JITs) know about the is-acceptable-this and actual-guts distinction.  That would mean we could get rid of the current JSNative-like signatures that just create a CallArgs and then CallNonGenericMethod.  But it's a longer-range speculative idea.  Just mentioning it to stimulate thought.
Attachment #642125 - Flags: review?(luke)
Attachment #642125 - Attachment is obsolete: true
Attachment #642125 - Flags: review?(luke)
Attachment #642180 - Flags: review?(luke)
Comment on attachment 642180 [details] [diff] [review]
Updated, posting for benefit of bug 770344 which builds on this

Review of attachment 642180 [details] [diff] [review]:

Nice.  Just to be safe, I'd verify our assumption that the fast path does inline the test and nativeimpl call as well as run v8/ss/kraken.

::: js/src/jsapi.h
@@ +1471,5 @@
> + *
> + * To implement a method that accepts |this| values from multiple compartments,
> + * define two functions.  The first function matches the IsAcceptableThis type
> + * and indicates whether the provided value is an acceptable |this| for the
> + * method; it must be idempotent.

'pure' is stronger and, I think, necessary

@@ +1483,5 @@
> + *           return false;
> + *       return JS_GetClass(&v.toObject()) == &AnswerClass;
> + *   }
> + *
> + * The second function matches the NativeImpl type and defines the behavior of

s/matches the NativeImpl/implements the NativeImpl signature/ ?

::: js/src/jsbool.cpp
@@ +55,3 @@
> +static bool
> +bool_toSourceImpl(JSContext *cx, CallArgs args)

_impl instead?

@@ +194,4 @@
>  bool
>  BooleanGetPrimitiveValueSlow(JSContext *cx, JSObject &obj, Value *vp)
>  {
> +    // XXX test for JSON on Boolean object before Boolean class initialized


::: js/src/json.cpp
@@ +906,4 @@
> +    /*
> +     * JSON requires that Boolean.prototype.valueOf be created and stashed in a
> +     * reserved slot on the global object.

Could you add a reference to the code that depends on this?

::: js/src/vm/MethodGuard.h
@@ +5,4 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at */
> +/* Error reporting for bad this values. */

Can you move the MethodGuard decls/defns into some other header?
Attachment #642180 - Flags: review?(luke) → review+

(In reply to Luke Wagner [:luke] from comment #2)
> Nice.  Just to be safe, I'd verify our assumption that the fast path does
> inline the test and nativeimpl call as well as run v8/ss/kraken.

v8/ss/kraken differences between a build pre-this and post-this were all in the noise.

As for inlining, I'd prefer to let compilers do their thing, then if we see a difference act on it.  More readable code, more data-driven code-optimization decisions, and all that.
Target Milestone: --- → mozilla17
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 777174
You need to log in before you can comment on or make changes to this bug.