Last Comment Bug 692547 - Split up array_extra
: Split up array_extra
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla10
Assigned To: Terrence Cole [:terrence]
:
:
Mentors:
Depends on:
Blocks: 697310 716955
  Show dependency treegraph
 
Reported: 2011-10-06 12:17 PDT by Terrence Cole [:terrence]
Modified: 2012-01-10 10:52 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Split up array_extra. (17.40 KB, patch)
2011-10-07 15:09 PDT, Terrence Cole [:terrence]
jwalden+bmo: review-
Details | Diff | Splinter Review
v2: With review feedback. (17.49 KB, patch)
2011-10-26 16:17 PDT, Terrence Cole [:terrence]
jwalden+bmo: review+
Details | Diff | Splinter Review
v3: with cleanups from second review. (17.46 KB, patch)
2011-11-04 11:08 PDT, Terrence Cole [:terrence]
terrence.d.cole: review+
Details | Diff | Splinter Review

Description Terrence Cole [:terrence] 2011-10-06 12:17:34 PDT
Using array_extra as a common implementation method does not shave off many bytes of code and makes understanding all of the functions it implements very difficult.  We should split this mess up and work to make these methods follow ECMA steps.
Comment 1 Terrence Cole [:terrence] 2011-10-07 15:09:01 PDT
Created attachment 565662 [details] [diff] [review]
Split up array_extra.

Split up into four methods: array_readonlyCommon (implements Every, Some, and ForEach), array_map, array_filter, and array_reduceCommon (implements reduce and reduceRight).  It also completely removes array_extra.  I have attempted to use templates where possible to eliminate excess runtime branches in the two all-in-one methods.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-19 16:17:54 PDT
Comment on attachment 565662 [details] [diff] [review]
Split up array_extra.

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

I haven't looked too hard at the actual common-ized some/every/forEach algorithm here, mostly just style and setup stuff, for what it's worth.  I did look at the others, which looked right to me.

::: js/src/jsarray.cpp
@@ +3125,5 @@
>      CallArgs args = CallArgsFromVp(argc, vp);
>      return array_indexOfHelper(cx, true, args);
>  }
>  
> +typedef enum ArrayReadonlyModes {

The typedef of the enum is a long holdover from C and is no longer necessary for pure C++ like we are now.  (But see later.)

@@ +3131,3 @@
>      SOME,
> +    FOREACH
> +} ArrayReadonlyModes;

Style nit, but these days we're going for Every, Some, ForEach names for enum constants, not macro-y shouting.  (But see later.)

@@ +3134,5 @@
> +
> +/* ECMA 15.4.4.16 - 15.4.4.18 */
> +template<ArrayReadonlyModes Mode>
> +static inline bool
> +array_readonlyCommon(JSContext *cx, CallArgs &args) {

{ at start of new line.

Templatizing based on an enum constant, when really what you're templatizing on is behavior, is kind of old-school C-ish.  New-school would be on a C++ class with two static methods (mark them as JS_ALWAYS_INLINE to be mostly guaranteed the perf of it all being exactly inline code), one for what to do on each step of execution, one for what to do at the end.  Existing examples of this are Behavior in jsobjinlines.h and SplitHelper (without static-ness) in jsstr.cpp.  Mind doing that?

@@ +3136,5 @@
> +template<ArrayReadonlyModes Mode>
> +static inline bool
> +array_readonlyCommon(JSContext *cx, CallArgs &args) {
> +    /* Step 1. */
> +    JSObject *O = ToObject(cx, &args.thisv());

As mentioned a bit ago, |O| is just not readable enough.  Use |obj| here even if it's not the spec name, unfortunately.

I think Luke's generic method stuff has bitrotted this particular verb, although I could be misremembering.

@@ +3142,2 @@
>          return false;
> +    Value Ov = ObjectValue(*O);

This shouldn't be needed, just assign ObjectValue(*obj) the one place it's used.  Generally you should keep variables with the most specific type possible for them, so JSObject*, JSFunction*, uint32, etc. and not Value, for best understandability, and only convert when needed.

@@ +3154,5 @@
>      }
>      JSObject *callable = js_ValueToCallableObject(cx, &args[0], JSV2F_SEARCH_STACK);
>      if (!callable)
> +        return false;
> +    Value callbackfn = ObjectValue(*callable);

Hm, this stuff is so much less readable than |if (!js_IsCallable(...)) ...|.  Something to clean up in a followup bug, I guess.

@@ +3157,5 @@
> +        return false;
> +    Value callbackfn = ObjectValue(*callable);
> +
> +    /* Step 5. */
> +    Value T = args.length() > 1 ? args[1] : UndefinedValue();

We use thisv for values used in this manner.

@@ +3160,5 @@
> +    /* Step 5. */
> +    Value T = args.length() > 1 ? args[1] : UndefinedValue();
> +
> +    /* Step 6. */
> +    jsuint k = 0;

jsuint as a type is going away eventually; use uint32 (which is identical, and clearer) instead.

@@ +3165,5 @@
> +
> +    /* Step 7. */
> +    InvokeArgsGuard ag;
> +    while (k < len) {
> +        if (!JS_CHECK_OPERATION_LIMIT(cx))

It occurs to me that we could probably substantially optimize these methods for dense arrays (along the lines concat, splice, etc. do).  Another followup bug.

@@ +3180,5 @@
> +            /* Step ii. */
> +            if (!ag.pushed() && !cx->stack.pushInvokeArgs(cx, 3, &ag))
> +                return false;
> +            ag.calleeHasBeenReset();
> +            ag.calleev() = callbackfn;

I just looked, and all places that call calleeHasBeenReset then immediately assign to calleev().  That suggests, and my understanding of these only supports, that the idiom should be a single method -- resetCallee(Value).  Write up a standalone patch to make that change, and either post it here or poke someone on IRC to give it a quick rubberstamp review -- latter is probably faster, but it doesn't really matter either way.  (This change is small enough that it seems reasonable to just do it now, and not wait for a followup.)

@@ +3221,3 @@
>  }
>  
> +/* ECMA 15.4.4.16 */

/* ES5 ... */ throughout, so it's clear exactly what version's being referenced, and for consistency with existing places that do this.

@@ +3250,5 @@
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    /* Step 1. */
> +    JSObject *O = ToObject(cx, &args.thisv());

obj again, and I think you want some Luke-based newness here, still.

@@ +3253,5 @@
> +    /* Step 1. */
> +    JSObject *O = ToObject(cx, &args.thisv());
> +    if (!O)
> +        return false;
> +    Value Ov = ObjectValue(*O);

Not needed.

@@ +3256,5 @@
> +        return false;
> +    Value Ov = ObjectValue(*O);
> +
> +    /* Step 2 & 3. */
> +    jsuint len;

uint32

@@ +3268,5 @@
> +    }
> +    JSObject *callable = js_ValueToCallableObject(cx, &args[0], JSV2F_SEARCH_STACK);
> +    if (!callable)
> +        return false;
> +    Value callbackfn = ObjectValue(*callable);

Not needed.

@@ +3271,5 @@
> +        return false;
> +    Value callbackfn = ObjectValue(*callable);
> +
> +    /* Step 5. */
> +    Value T = args.length() > 1 ? args[1] : UndefinedValue();

thisv.

Hm, the spec could have just used thisArg directly here; too bad it didn't, for a little more concision (although maybe that's less clear).

@@ +3274,5 @@
> +    /* Step 5. */
> +    Value T = args.length() > 1 ? args[1] : UndefinedValue();
> +
> +    /* Step 6. */
> +    JSObject *A = NewDenseAllocatedArray(cx, len);

arr

@@ +3283,5 @@
> +        return false;
> +    A->setType(newtype);
> +
> +    /* Step 7. */
> +    jsuint k = 0;

uint32

@@ +3288,5 @@
> +
> +    /* Step 8. */
> +    InvokeArgsGuard ag;
> +    while (k < len) {
> +        if (!JS_CHECK_OPERATION_LIMIT(cx))

More followup optimization opportunity.

@@ +3310,5 @@
> +            ag[1] = NumberValue(k);
> +            ag[2] = Ov;
> +            if (!Invoke(cx, ag))
> +                return false;
> +            Value mappedValue = ag.rval();

No need for this extra intermediate value, just to name it.

@@ +3333,5 @@
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    /* Step 1. */
> +    JSObject *O = ToObject(cx, &args.thisv());

Generic stuff again, and obj.

@@ +3336,5 @@
> +    /* Step 1. */
> +    JSObject *O = ToObject(cx, &args.thisv());
> +    if (!O)
> +        return false;
> +    Value Ov = ObjectValue(*O);

No need.

@@ +3339,5 @@
> +        return false;
> +    Value Ov = ObjectValue(*O);
> +
> +    /* Step 2 & 3. */
> +    jsuint len;

uint32

@@ +3351,5 @@
> +    }
> +    JSObject *callable = js_ValueToCallableObject(cx, &args[0], JSV2F_SEARCH_STACK);
> +    if (!callable)
> +        return false;
> +    Value callbackfn = ObjectValue(*callable);

No need.

@@ +3354,5 @@
> +        return false;
> +    Value callbackfn = ObjectValue(*callable);
> +
> +    /* Step 5. */
> +    Value T = args.length() > 1 ? args[1] : UndefinedValue();

thisv

@@ +3366,5 @@
> +        return false;
> +    A->setType(newtype);
> +
> +    /* Step 7. */
> +    jsuint k = 0;

uint32

@@ +3369,5 @@
> +    /* Step 7. */
> +    jsuint k = 0;
> +
> +    /* Step 8. */
> +    jsuint to = 0;

uint32

@@ +3420,5 @@
> +
> +typedef enum ArrayReduceMode {
> +    REDUCE_LEFT,
> +    REDUCE_RIGHT
> +} ArrayReduceMode;

Another candidate for behavior selection through a class with static methods.

@@ +3454,5 @@
> +        return false;
> +    }
> +
> +    /* Step 6. */
> +    jsuint k, end;

uint32

@@ +3455,5 @@
> +    }
> +
> +    /* Step 6. */
> +    jsuint k, end;
> +    jsint step;

uint32, I think.  (And note by the -1 additions that we're relying on modulo-based unsigned integer overflow, perhaps.  That bit is not exactly the clearest part of all this, alas.)

@@ +3471,5 @@
> +    }
> +
> +    Value accumulator;
> +    if (args.length() >= 2) {
> +        /* Step 7.a. */

Commenting here isn't really necessary, I think -- just clutters up, rather than illuminates.

@@ +3476,5 @@
> +        accumulator = args[1];
> +    } else {
> +        /* Step 8.a. */
> +        JSBool kNotPresent = true;
> +        /* Step b. */

Blank line before /* */ comments.
Comment 3 Terrence Cole [:terrence] 2011-10-26 16:17:55 PDT
Created attachment 569835 [details] [diff] [review]
v2: With review feedback.

Added bugs 697260, 697310, and 697322 to track the work I'm not doing in this patch.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-03 15:37:15 PDT
Comment on attachment 569835 [details] [diff] [review]
v2: With review feedback.

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

::: js/src/jsarray.cpp
@@ +3328,5 @@
>      JSObject *obj = ToObject(cx, &args.thisv());
>      if (!obj)
>          return false;
>  
> +    /* Step 2 & 3. */

Formatting for these should be consistent with that for other step-comments in other code:
/* Steps 2-3. */

@@ +3343,5 @@
>      if (!callable)
> +        return false;
> +
> +    /* Step 5. */
> +    Value thisv = args.length() > 1 ? args[1] : UndefinedValue();

Suggest |args.length() >= 2|, since having two arguments is the precondition here, and I think >= the value suggests that more clearly than more-than-one does.

@@ +3421,5 @@
> +    JSObject *obj = ToObject(cx, &args.thisv());
> +    if (!obj)
> +        return false;
> +
> +    /* Step 2 & 3. */

/* Steps 2-3. */

@@ +3436,5 @@
> +    if (!callable)
> +        return false;
> +
> +    /* Step 5. */
> +    Value thisv = args.length() > 1 ? args[1] : UndefinedValue();

>= 2

@@ +3498,5 @@
> +    JSObject *obj = ToObject(cx, &args.thisv());
> +    if (!obj)
> +        return false;
> +
> +    /* Step 2 & 3. */

/* Steps 2-3. */

@@ +3513,5 @@
> +    if (!callable)
> +        return false;
> +
> +    /* Step 5. */
> +    Value thisv = args.length() > 1 ? args[1] : UndefinedValue();

>= 2

@@ +3530,5 @@
> +
> +    /* Step 8. */
> +    uint32 to = 0;
> +
> +    /* Step 8. */

/* Step 9. */

@@ +3607,5 @@
> +    JSObject *obj = ToObject(cx, &args.thisv());
> +    if (!obj)
> +        return false;
> +
> +    /* Step 2 & 3. */

/* Steps 2-3. */
Comment 5 Terrence Cole [:terrence] 2011-11-04 11:08:55 PDT
Created attachment 572024 [details] [diff] [review]
v3: with cleanups from second review.

Posting final patch and carrying r+ forward.
Comment 6 Terrence Cole [:terrence] 2011-11-04 15:52:14 PDT
https://tbpl.mozilla.org/?tree=Try&rev=8d802c1de58b
Comment 7 Terrence Cole [:terrence] 2011-11-07 10:15:00 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/bb1945550b0f
Comment 8 Ed Morley [:emorley] 2011-11-08 01:45:52 PST
https://hg.mozilla.org/mozilla-central/rev/bb1945550b0f
Comment 9 Brendan Eich [:brendan] 2012-01-10 10:41:32 PST
    static bool shouldExit(Value &callval, Value *rval)

The various Value &callval parameters should be const references.

callval sounds like a callable, or something. The parameter is the funarg return value. Too many rvals, so "callval" has some appeal, but being explicit seems better: callback_rval.

/be

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