Closed Bug 776579 Opened 7 years ago Closed 7 years ago

Use HandleValue and MutableHandleValue in object hooks where possible

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: bhackett, Assigned: bhackett)

References

Details

(Whiteboard: [js:t])

Attachments

(1 file)

Doing this will be less error prone, as hooks will be free to perform effectful operations after setting their output parameters.
Assignee: general → bhackett1024
Attachment #645293 - Flags: superreview?(dmandelin)
Attachment #645293 - Flags: review?(wmccloskey)
Whiteboard: [js:t]
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 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?
Attachment #645293 - Flags: review?(wmccloskey) → review+
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.
Attachment #645293 - Flags: superreview?(dmandelin) → superreview+
https://hg.mozilla.org/mozilla-central/rev/090fd1585e34
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Depends on: 779393
You need to log in before you can comment on or make changes to this bug.