730880 - (memset) ArrayBufferView.prototype.fill

Status: RESOLVED DUPLICATE of bug 1113722

(Core :: JavaScript Engine, defect)

Description

[Dave Herman [:dherman]]

We are considering adding a fill method to typed arrays for doing efficient memsets. We'd like to see how efficient a built-in method would be over a pure JS version.

First cut at behavior:

    a.fill(start, end, val)

is equivalent to:

    start = start >>> 0; // ToUint32
    end = end >>> 0;     // ToUint32
    if (typeof val === "undefined")
        val = 0;
    val = [[ThisType]](val);
    for (let i = start; i < end; i++) {
        this[i] = val;
    }

Dave

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Implement Arraybuffer.prototype.fill

Not ready for review yet. Applied on top of DataView + Transferable + move(). Seems to be working.

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Implement Arraybuffer.prototype.fill

memset equivalent

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 630322 [details] [diff] [review]
Implement Arraybuffer.prototype.fill

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

r- for the zero bits in particular, otherwise seems about on a right track.

::: js/src/jstypedarray.cpp
@@ +799,5 @@
> +    JS_ASSERT(byteStart <= getByteLength(tarray));
> +    JS_ASSERT(byteEnd <= getByteLength(tarray));
> +    JS_ASSERT(byteEnd % valueSize == 0);
> +
> +    uint8_t *data = static_cast<uint8_t*>(tarray->getPrivate());

Shouldn't this be getDataOffset or something?

@@ +802,5 @@
> +
> +    uint8_t *data = static_cast<uint8_t*>(tarray->getPrivate());
> +
> +    if (valueSize == 1 || zero) {
> +        memset(&data[byteStart], *value, byteEnd - byteStart);

Haha, I win!  Most hilarious bug ever.

If you have a Float32Array or a Float64Array, and the value you pass in to fill the array is -0, the leading byte of that value on big-endian systems will have the sign bit set.  But it'll appear to be zero!  So you'd end up copying 0x80 into every byte of the elements you're filling, and I don't think you'd be setting the elements to 0, or even to -0.  Lulz.

I think you should pass in the value to set by value here and do the is-zero test here as well.  And of course to fix this issue, you'll have to have an IsZero method specialized for floats and doubles that passes +0 only.

And of course this stuff should have tests.  :-)

@@ +808,5 @@
> +        for (uint8_t *p = &data[byteStart]; p < &data[byteEnd]; p += valueSize)
> +            memcpy(p, value, valueSize);
> +    }
> +
> +    return true;

Another uselessly bool method.

@@ +1688,5 @@
> +    {
> +        CallArgs args = CallArgsFromVp(argc, vp);
> +
> +        bool ok;
> +        JSObject *obj = NonGenericMethodGuard(cx, args, fun_move, fastClass(), &ok);

Hem, hem.

@@ +1694,5 @@
> +            return ok;
> +
> +        JSObject *tarray = getTypedArray(obj);
> +        if (!tarray)
> +            return JS_TRUE;

This also seems impossible.

@@ +1696,5 @@
> +        JSObject *tarray = getTypedArray(obj);
> +        if (!tarray)
> +            return JS_TRUE;
> +
> +        if (args.length() < 3) {

This doesn't agree with comment 0, but it seems better to me.

@@ +1714,5 @@
> +            return JS_FALSE;
> +        }
> +
> +        NativeType value = nativeFromValue(cx, args[2]);
> +        if (!memsetData(cx, tarray, begin * sizeof(NativeType), end * sizeof(NativeType), reinterpret_cast<uint8_t*>(&value), sizeof(NativeType), value == 0))

Same bit about passing indexes as element-based, not byte-based, for typed array-centric methods.  Then you can also get rid of the sizeof(NativeType) method too.

@@ +1716,5 @@
> +
> +        NativeType value = nativeFromValue(cx, args[2]);
> +        if (!memsetData(cx, tarray, begin * sizeof(NativeType), end * sizeof(NativeType), reinterpret_cast<uint8_t*>(&value), sizeof(NativeType), value == 0))
> +            return JS_FALSE;
> +        args.rval().setNull();

setUndefined()

@@ +3025,5 @@
>  JSFunctionSpec _typedArray::jsfuncs[] = {                                      \
>      JS_FN("subarray", _typedArray::fun_subarray, 2, JSFUN_GENERIC_NATIVE),     \
>      JS_FN("set", _typedArray::fun_set, 2, JSFUN_GENERIC_NATIVE),               \
>      JS_FN("move", _typedArray::fun_move, 3, JSFUN_GENERIC_NATIVE),             \
> +    JS_FN("fill", _typedArray::fun_fill, 3, JSFUN_GENERIC_NATIVE),             \

Erm, JSFUN_GENERIC_NATIVE makes no sense whatsoever for any of these, because these methods all type-check |this| to be exactly one class (or a proxy to an object of that class).  KILL IT WITH FIRE.  :-D