Last Comment Bug 776579 - Use HandleValue and MutableHandleValue in object hooks where possible
: Use HandleValue and MutableHandleValue in object hooks where possible
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Brian Hackett (:bhackett)
:
Mentors:
Depends on: 779393
Blocks: 750733
  Show dependency treegraph
 
Reported: 2012-07-23 09:59 PDT by Brian Hackett (:bhackett)
Modified: 2012-07-31 19:23 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (fe623d60bea1) (607.22 KB, patch)
2012-07-24 07:29 PDT, Brian Hackett (:bhackett)
wmccloskey: review+
dmandelin: superreview+
Details | Diff | Splinter Review

Description Brian Hackett (:bhackett) 2012-07-23 09:59:24 PDT
Doing this will be less error prone, as hooks will be free to perform effectful operations after setting their output parameters.
Comment 1 Brian Hackett (:bhackett) 2012-07-24 07:29:06 PDT
Created attachment 645293 [details] [diff] [review]
patch (fe623d60bea1)
Comment 2 David Mandelin [:dmandelin] 2012-07-24 15:12:04 PDT
Waiting for Bill's review before sr'ing fully. At first glance, the jsapi.h and jsclass.h changes look reasonable. I like the fact that IIUC it really means strictly less work and less to understand for implementers of those callbacks. Also actually kind of like replacing 'jsval *', which is rather ambiguous about the semantics (is it an array?) with JSMutableHandleValue. I still wonder how many different kinds of things we'll have in play at the end, but this does seem like a step in the right direction, and not too risky. (I'll think about it more carefully when I see what Bill noticed that I didn't. ;-) )
Comment 3 Bill McCloskey (:billm) 2012-07-25 12:42:02 PDT
Comment on attachment 645293 [details] [diff] [review]
patch (fe623d60bea1)

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

Thanks for doing this.

In the future, I think we need to consider how to deal with arrays of rooted things. It seems like that's where most of the calls to fromMarkedLocation come from. It would be great if we could eliminate those.

::: js/src/jsanalyze.cpp
@@ +20,5 @@
>  /////////////////////////////////////////////////////////////////////
>  
>  #ifdef DEBUG
>  void
> +PrintBytecode(JSContext *cx, JSScript *script_, jsbytecode *pc)

Luke wants vars like this to be called scriptArg now.

::: js/src/jsapi.cpp
@@ +6157,5 @@
> +    return true;
> +}
> +
> +JS_PUBLIC_API(JSBool)
> +JS_ParseJSONWithReviver(JSContext *cx, const jschar *chars, uint32_t len, jsval reviver_, jsval *vp)

reviverArg

::: js/src/jsarray.cpp
@@ +412,5 @@
>  static bool
>  GetElementsSlow(JSContext *cx, HandleObject aobj, uint32_t length, Value *vp)
>  {
>      for (uint32_t i = 0; i < length; i++) {
> +        if (!aobj->getElement(cx, i, MutableHandleValue::fromMarkedLocation(&vp[i])))

Could you maybe comment this function and GetElements, saying that vp needs to be rooted somehow?

@@ +1807,5 @@
>  
>      const Value* end = vector + count;
>      while (vector < end && start <= MAX_ARRAY_INDEX) {
>          if (!JS_CHECK_OPERATION_LIMIT(cx) ||
> +            !SetArrayElement(cx, obj, start++, HandleValue::fromMarkedLocation(vector++))) {

Same here.

::: js/src/jsdate.cpp
@@ +2605,5 @@
>          return true;
>      }
>  
>      /* Step 4. */
> +    MutableHandleValue toISO = MutableHandleValue::fromMarkedLocation(&vp[0]);

It seems like we could just make a local var holding vp[0] and then copy it back.

::: js/src/jsdbgapi.cpp
@@ +800,5 @@
>          lastException = cx->getPendingException();
>      cx->clearPendingException();
>  
>      Rooted<jsid> id(cx, shape->propid());
> +    if (!baseops::GetProperty(cx, obj, id, MutableHandleValue::fromMarkedLocation(&pd->value))) {

I'd rather keep uses of fromMarkedLocation to a minimum. In cases like this, where performance doesn't matter, can't we root it and then copy it back?

@@ +1727,5 @@
>  
>  #endif /* __linux__ */
>  
>  JS_PUBLIC_API(void)
> +JS_DumpBytecode(JSContext *cx, JSScript *script_)

scriptArg

@@ +1744,5 @@
>  #endif
>  }
>  
>  extern JS_PUBLIC_API(void)
> +JS_DumpPCCounts(JSContext *cx, JSScript *script_)

scriptArg

::: js/src/jsnum.cpp
@@ +1159,5 @@
>          return NULL;
>  
>      /* ES5 15.1.1.1, 15.1.1.2 */
>      if (!DefineNativeProperty(cx, global, cx->runtime->atomState.NaNAtom,
> +                              HandleValue::fromMarkedLocation(&cx->runtime->NaNValue),

Could you just root this one instead? Performance doesn't seem to be an issue here.

@@ +1165,3 @@
>                                JSPROP_PERMANENT | JSPROP_READONLY, 0, 0) ||
>          !DefineNativeProperty(cx, global, cx->runtime->atomState.InfinityAtom,
> +                              HandleValue::fromMarkedLocation(&cx->runtime->positiveInfinityValue),

Same here.

::: js/xpconnect/src/XPCQuickStubs.h
@@ +627,5 @@
>      JS::RootedId id(cx);
>      if (!JS_ValueToId(cx, v, id.address()))
>          return false;
>      JS_SET_RVAL(cx, vp, argval);
> +    return ApplyPropertyOp<Op>(cx, *popp, obj, id, JSMutableHandleValue::fromMarkedLocation(vp));

Can you use CallArgs here?
Comment 4 David Mandelin [:dmandelin] 2012-07-27 16:25:47 PDT
Comment on attachment 645293 [details] [diff] [review]
patch (fe623d60bea1)

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

Looks nice. The changes at usage sites seem very simple and mechanical, which seems like a good sign.
Comment 5 Brian Hackett (:bhackett) 2012-07-30 04:19:58 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/090fd1585e34
Comment 6 Ed Morley [:emorley] 2012-07-31 06:14:10 PDT
https://hg.mozilla.org/mozilla-central/rev/090fd1585e34

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