Last Comment Bug 773850 - Redesign non-generic method guarding API
: Redesign non-generic method guarding API
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 777174 854481
Blocks: 770344
  Show dependency treegraph
 
Reported: 2012-07-13 16:14 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2013-03-25 10:46 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (178.59 KB, patch)
2012-07-13 16:14 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Updated, posting for benefit of bug 770344 which builds on this (178.52 KB, patch)
2012-07-13 23:17 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-13 16:14:07 PDT
Created attachment 642125 [details] [diff] [review]
Patch

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.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-13 23:17:09 PDT
Created attachment 642180 [details] [diff] [review]
Updated, posting for benefit of bug 770344 which builds on this
Comment 2 Luke Wagner [:luke] 2012-07-16 10:12:46 PDT
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 @@
>  #if JS_HAS_TOSOURCE
> +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 http://mozilla.org/MPL/2.0/. */
>  
> +/* Error reporting for bad this values. */

Can you move the MethodGuard decls/defns into some other header?
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-16 16:44:22 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d00c508b09a

(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.
Comment 4 Ed Morley [:emorley] 2012-07-17 02:11:49 PDT
https://hg.mozilla.org/mozilla-central/rev/5d00c508b09a

Note You need to log in before you can comment on or make changes to this bug.