Use CallArgs in jsarray.cpp

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla10
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 563275 [details] [diff] [review]
Use CallArgs in jsarray.cpp.

Except for array_splice.
(Assignee)

Comment 1

6 years ago
Created attachment 563422 [details] [diff] [review]
Except for array_splice and array_concat.

Missed some use of argc and one use of vp.
Attachment #563275 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #563422 - Flags: review?(jwalden+bmo)
Comment on attachment 563422 [details] [diff] [review]
Except for array_splice and array_concat.

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

Mostly style nits and other such things, nice cleanup otherwise.

::: js/src/jsarray.cpp
@@ +1657,4 @@
>      if (!obj)
>          return false;
>  
> +    Value &join = args.calleev();

Currently, stack layout on entry looks like [callee, thisv, arg0, arg1, ...].  On exit it looks like [rval, #t, #0, #1, ...] where #X is anything.  (This is an artifact of using those locations as GC roots, back before we scanned the native stack for things that the GC had to keep alive.)  We want to move to a point where all examination of the callee takes place before setting the return value, and all of this happens through CallArgs.  Using the reference like this breaks that.  So just use a Value local to do this.

@@ +1697,5 @@
>      /*
>       *  Passing comma here as the separator. Need a way to get a
>       *  locale-specific version.
>       */
> +    return array_toString_sub(cx, obj, JS_TRUE, NULL, &args.rval());

Generally when we've argsified shared methods, we've just passed the CallArgs along, rather than pull out one part or another.

@@ +1826,5 @@
>  /*
>   * Perl-inspired join, reverse, and sort.
>   */
>  static JSBool
>  array_join(JSContext *cx, uintN argc, Value *vp)

This order of operations (do stuff with arguments, then ToObject(this)) is wrong.  Please file a followup to fix this to agree with the spec algorithm.

@@ +1857,5 @@
>  
>      jsuint len;
>      if (!js_GetLengthProperty(cx, obj, &len))
>          return false;
> +    args.rval().setObject(*obj);

This should move down to immediately adjacent to the early return-trues.

@@ +1923,2 @@
>              !SetOrDeleteArrayElement(cx, obj, len - i - 1, hole, tvr.value()) ||
> +            !SetOrDeleteArrayElement(cx, obj, i, hole2, args.rval())) {

These should store to a local Value -- the only store to rval() should be the actual return value.

@@ +2157,4 @@
>  }
>  
>  JSBool
>  js::array_sort(JSContext *cx, uintN argc, Value *vp)

Despite not having exact steps, this also should be doing things more in spec order, by the prose paragraphs in the algorithm.  File another followup.

@@ +2590,5 @@
>                  return JS_FALSE;
>              return JS_TRUE;
>          }
>  
> +        /* Get the to-be-deleted property's value into rval ASAP. */

Just remove this comment, I think.

@@ +2651,5 @@
>                  optimized = true;
>              } while (false);
>  
>              if (!optimized) {
>                  last = length;

Could you move the declaration of |last| down to here, so you have |jsdouble last = length;|?  It'd help with seeing that |last + args.length()| won't overflow *before* conversion to double, which I initially missed because I assumed |last| was a uint32_t.

@@ +2982,4 @@
>          return JS_TRUE;
>      }
>  
> +    /* Create a new Array object and root it in args. */

The manual rooting here is no longer necessary with stack scanning, so just remove the comment.

@@ +2986,5 @@
>      nobj = NewDenseAllocatedArray(cx, end - begin);
>      if (!nobj)
>          return JS_FALSE;
>      TryReuseArrayType(obj, nobj);
> +    args.rval().setObject(*nobj);

Let's do this at the end of the method, since that's where it happens in the spec algorithm.

@@ +3002,5 @@
>      return JS_TRUE;
>  }
>  
>  static JSBool
> +array_indexOfHelper(JSContext *cx, JSBool isLast, CallArgs &args)

Make that |bool isLast| while you're changing the signature, and pass true/false in the two callers.

@@ +3054,5 @@
>      }
>  
>      for (;;) {
>          if (!JS_CHECK_OPERATION_LIMIT(cx) ||
> +            !GetElement(cx, obj, (jsuint)i, &hole, &args.rval())) {

Store to a local please.

@@ +3119,5 @@
>       * First, get or compute our callee, so that we error out consistently
>       * when passed a non-callable object.
>       */
> +    if (args.length() == 0) {
> +        js_ReportMissingArg(cx, args.rval(), 0);

That should be args.calleev().

@@ +3156,4 @@
>          } else {
>              JSBool hole;
>              do {
> +                if (!GetElement(cx, obj, start, &hole, &args.rval()))

Use a stacked local.

@@ +3177,5 @@
>          newtype = GetTypeCallerInitObject(cx, JSProto_Array);
>          if (!newtype)
>              return JS_FALSE;
>          newarr->setType(newtype);
> +        args.rval().setObject(*newarr);

And here, and for all the other cases here.

@@ +3603,4 @@
>      JSObject *array;
>  
> +    for (uintN i = 0; i < args.length(); i++) {
> +        Value &arg = args[i];

Don't use a reference here, this being debug there's no reason to attempt to save space or anything here (assuming that's why you changed it).

@@ +3624,5 @@
>          fputs(")\n", stderr);
>          cx->free_(bytes);
>      }
>  
> +    args.rval() = JSVAL_VOID;

That should be UndefinedValue() or .setUndefined().
Attachment #563422 - Flags: review?(jwalden+bmo) → review-
(Assignee)

Comment 3

6 years ago
Thank you for the extremely thorough and clear review!  Just filed bugs https://bugzilla.mozilla.org/show_bug.cgi?id=691501 and https://bugzilla.mozilla.org/show_bug.cgi?id=691504.  Still working on fixing up the patch.
(Assignee)

Comment 4

6 years ago
Created attachment 568094 [details] [diff] [review]
v2: With review feedback.

Applied review feedback, except for array_extra, which I have exported to bug 692547, based on top of the existing work in this patch.
Attachment #563422 - Attachment is obsolete: true
Attachment #568094 - Flags: review?(jwalden+bmo)
Comment on attachment 568094 [details] [diff] [review]
v2: With review feedback.

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

I think this looks pretty reasonable.  (I skipped the extras bits because they're being rewritten in that other bug, tho.)  Make these changes and post a new patch, please.

Given the extras get changed by this patch but then muchly-changed in the other bug, I'd say let's finish up the extras bug and then land these two changes simultaneously (separate revisions, same push).

::: js/src/jsarray.cpp
@@ +1624,5 @@
>              if (!JS_CHECK_OPERATION_LIMIT(cx))
>                  return false;
>  
>              JSBool hole;
> +            Value tmp;

|elt| would be a slightly better name, I think.

@@ +1662,5 @@
>  {
>      JS_CHECK_RECURSION(cx, return false);
>  
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    JSObject *obj = ToObject(cx, &args.thisv());

This may want to be something generic-y now, too.  Although if you generated this patch on the 19th, all that stuff should have landed well ago.  So maybe I'm just completely misremembering.  :-)

@@ +1928,4 @@
>          return true;
>      } while (false);
>  
> +    Value vLo, vHi;

lowval and hival would probably be more idiomatic names.

@@ +2418,5 @@
>      jsuint length;
>  
>      if (!js_GetLengthProperty(cx, obj, &length))
>          return JS_FALSE;
> +    if (!InitArrayElements(cx, obj, length, args.length(), args.array(), true))

File a followup to change that last argument to an enum, per more modern SpiderMonkey style?  I had thought on initial read here that it indicated if the passed-in vector contained holes, but it doesn't.  (I think it did, once.)  Having the last value as an enum would make the call more self-describing.

@@ +2508,5 @@
>          return false;
>  
>      /* Insist on one argument and obj of the expected class. */
> +    if (args.length() != 1 || !obj->isDenseArray())
> +        return array_push_slowly(cx, obj, args);

It turns out we have a bug here, preexisting.  If the array is dense, but there's a setter at the index that push will fill in, we're supposed to invoke the setter.

@@ +2521,2 @@
>      if (!js_GetLengthProperty(cx, obj, &index))
>          return JS_FALSE;

Make all the JS_FALSE/JS_TRUE in this method false/true while you're here, and touching so much of it.  (Feel free to do that to any code you touch, really.  We've generally only tended to convert when we're already touching the code, as here.  The exceptions to conversion are any methods which are JSAPI and must be C-compatible and methods which JITs invoke, which might not yet know about the C++ bool type.)

@@ +2545,2 @@
>  {
> +    jsuint index = obj->getArrayLength();

uint32

@@ +3137,5 @@
>      return JS_TRUE;
>  }
>  
>  static JSBool
> +array_indexOfHelper(JSContext *cx, bool isLast, CallArgs &args)

Could you change bool isLast to |enum IndexOfKind { IndexOf, LastIndexOf }| since you're changing this already?

@@ +3188,5 @@
>          direction = 1;
>      }
>  
>      for (;;) {
> +        Value tmp;

elt again.
Attachment #568094 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 6

6 years ago
Created attachment 568563 [details] [diff] [review]
v3: More review feedback.

One caveat: I've kept the array_extra changes in place, since I used those as a starting point when splitting array_extra.  They are extremely basic and don't break any tests.

This patch doesn't make everything in jsarray sparkly unicorn rainbows, but it's getting up to 43KiB in size at this point and has already succeeded in its primary motivation: using CallArgs throughout jsarray.
Attachment #568094 - Attachment is obsolete: true
Attachment #568563 - Flags: checkin?(jwalden+bmo)
Has this been sent to try? If not, I'll do so before pushing, once the try situation is resolved (bug 696682).
(Assignee)

Comment 8

6 years ago
(In reply to Ed Morley [:edmorley] from comment #7)
> Has this been sent to try? If not, I'll do so before pushing, once the try
> situation is resolved (bug 696682).

It has not.  I would greatly appreciate a run through try.
https://tbpl.mozilla.org/?tree=Try&rev=7a3b99cd4c9f

:-)

Comment 10

6 years ago
Try run for 7a3b99cd4c9f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=7a3b99cd4c9f
Results (out of 160 total builds):
    exception: 1
    success: 151
    warnings: 8
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bmo@edmorley.co.uk-7a3b99cd4c9f
Comment on attachment 568563 [details] [diff] [review]
v3: More review feedback.

https://hg.mozilla.org/integration/mozilla-inbound/rev/933582bd3a8d
Attachment #568563 - Flags: checkin?(jwalden+bmo) → checkin+
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/933582bd3a8d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.