The default bug view has changed. See this FAQ.

Split up array_extra

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
--
minor
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

(Blocks: 1 bug)

Trunk
mozilla10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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.
Assignee: general → tcole
Status: NEW → ASSIGNED
Attachment #565662 - Flags: review?(jwalden+bmo)
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.
Attachment #565662 - Flags: review?(jwalden+bmo) → review-
(Assignee)

Updated

6 years ago
Blocks: 697310
(Assignee)

Comment 3

6 years ago
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.
Attachment #565662 - Attachment is obsolete: true
Attachment #569835 - Flags: review?(jwalden+bmo)
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. */
Attachment #569835 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 5

6 years ago
Created attachment 572024 [details] [diff] [review]
v3: with cleanups from second review.

Posting final patch and carrying r+ forward.
Attachment #569835 - Attachment is obsolete: true
Attachment #572024 - Flags: review+
(Assignee)

Comment 6

6 years ago
https://tbpl.mozilla.org/?tree=Try&rev=8d802c1de58b
(Assignee)

Comment 7

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/bb1945550b0f
https://hg.mozilla.org/mozilla-central/rev/bb1945550b0f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
    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
(Assignee)

Updated

5 years ago
Blocks: 716955
You need to log in before you can comment on or make changes to this bug.