Use HandleValue and MutableHandleValue in object hooks where possible

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

Other Branch
mozilla17
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Doing this will be less error prone, as hooks will be free to perform effectful operations after setting their output parameters.
(Assignee)

Comment 1

5 years ago
Created attachment 645293 [details] [diff] [review]
patch (fe623d60bea1)
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+
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/090fd1585e34

Comment 6

5 years ago
https://hg.mozilla.org/mozilla-central/rev/090fd1585e34
Status: NEW → RESOLVED
Last Resolved: 5 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.