Njs: ArrayBuffer and TypedArray support.

Created on 29 Nov 2019  路  14Comments  路  Source: nginx/njs

These patches add the following global constructors:

- ArrayBuffer([length])
- %TypedArray%([length])
- %TypedArray%([buffer, [start, [end]]])
- %TypedArray%([array])
- %TypedArray%([typedarray])

For %TypedArray% the constructors are: Uint8Array, Uint16Array, Uint32Array, Int8Array, Int16Array, Int32Array, Float32Array, Float64Array and Uint8ClampedArray, BigUint64Array and BigInt64Array.

The following properties were implemented:

- ArrayBuffer.isView(obj)
- ArrayBuffer[Symbol.species]
- ArrayBuffer.prototype.constructor
- ArrayBuffer.prototype.byteLength
- ArrayBuffer.prototype.slice([start, [end]])
- ArrayBuffer.prototype[Symbol.toStringTag]
- ArrayBuffer,.prototype.toString()

- %TypedArray%.prototype.constructor
- %TypedArray%.prototype.set(array [, start])
- %TypedArray%.prototype.slice([start, [end]])
- %TypedArray%.prototype.toString()
- %TypedArray%.prototype.buffer
- %TypedArray%.prototype.byteLength
- %TypedArray%.prototype.byteOffset
- %TypedArray%.prototype.length

Patches: https://gist.github.com/tiago4orion/d03e7660518739d5fae7e1e87e806c10

Test262:

 === Summary ===
  - Ran 32306 tests
- - Passed 10688 tests (33.1%)
- - Failed 21618 tests (66.9%)
+ - Passed 11085 tests (34.3%)
+ - Failed 21221 tests (65.7%)
ES6 feature

Most helpful comment

No. I was planning to contribute this in a future patch.

Yes, I think we can leave this as is for now.

All 14 comments

@tiago4orion

Hi!

Thank you for the patches!

Looks unneeded:
https://gist.github.com/tiago4orion/d03e7660518739d5fae7e1e87e806c10#file-njs_array_buffer-patch-L246
https://gist.github.com/tiago4orion/d03e7660518739d5fae7e1e87e806c10#file-njs_array_buffer-patch-L294

https://gist.github.com/tiago4orion/d03e7660518739d5fae7e1e87e806c10#file-njs_typed_array-patch-L497
and below
maybe '&args[1] -> value' ?
Style: new line before else, please.

https://gist.github.com/tiago4orion/d03e7660518739d5fae7e1e87e806c10#file-njs_typed_array-patch-L1000
njs_is_null_or_undefined -> njs_is_undefined

https://gist.github.com/tiago4orion/d03e7660518739d5fae7e1e87e806c10#file-njs_typed_array-patch-L529
https://gist.github.com/tiago4orion/d03e7660518739d5fae7e1e87e806c10#file-njs_typed_array-patch-L540
return NJS_ERROR

https://gist.github.com/tiago4orion/d03e7660518739d5fae7e1e87e806c10#file-njs_array_buffer-patch-L107
There's no need to set size to 0 here, as it's already initialized to 0 before.

https://gist.github.com/tiago4orion/d03e7660518739d5fae7e1e87e806c10#file-njs_typed_array-patch-L901
It's better to add a comment here describing why the return value of njs_value_to_number() isn't checked.

Thanks, I'll update it soon with improvements.

For the function TypedArray.prototype.toString, I was lazy and just copied the typedarray content into a newly allocated array and used its own toString method... but I'll improve by properly implementing the TypedArray.prototype.join and just point the toString to Array.prototype.toString builtin function as the spec says. The current implementation also does not free the temporary array allocated in it.

I updated the patches.

I implemented TypedArray.prototype.join and pointed TypedArray.prototype.toString to njs_array_prototype_to_string, as says the spec.

I also implemented the TypedArray.prototype.copyWithin function.

@tiago4orion

An actual [proto]type hierarchy defined by the spec is Uint8Array -> TypedArray -> Object :

$ node --use-strict
> var u8 = new Uint8Array([1,2,3]);
undefined
> var u8p = Object.getPrototypeOf(u8);
undefined
> Object.getOwnPropertyDescriptors(u8p);
{ constructor:
   { value: [Function: Uint8Array],
     writable: true,
     enumerable: false,
     configurable: true },
  BYTES_PER_ELEMENT:
   { value: 1,
     writable: false,
     enumerable: false,
     configurable: false } }
> var u8pp = Object.getPrototypeOf(u8p);
undefined
> new u8pp.constructor([1,2,3])
Thrown:
TypeError: Abstract class TypedArray not directly constructable

But there is no TypedArray in your implementation.
Is it an intentional optimization?

No. I was planning to contribute this in a future patch.
We are very close to having that now, but I'm not really sure how to add that behavior. I think it will require to create an own prototype creator function (njs_typed_array_prototype_create) that exposes the internal TypedArray constructor, right?

No. I was planning to contribute this in a future patch.

Yes, I think we can leave this as is for now.

Patches updated: Moved the isnan change to arraybuffer patch.

@tiago4orion

Please, consider and apply the following improvements over the first patch
https://gist.github.com/2308c3f96dafc091e3f18e62f90c044d

@tiago4orion

Consider the following changes:
https://gist.github.com/xeioex/b77a5c977dc2fd2b34d383f423ea14a1

1) Introduced ToIndex() conversion primitive from the spec. (will commit to upstream soon)
2) Introduced the ArrayBuffer object. (your patch)
3) fix1.patch
4) Introduced TypedArray objects. (your patch)
5) fix2_2.patch (so far I simplified only njs_typed_array_constructor()).

Tomorrow I will continue review/improvement cycle for the second patch.

@tiago4orion Updated https://gist.github.com/xeioex/9bea539dd687b1d3ccaa2ca9c40b863e

1) Introduced ToIndex() conversion primitive from the spec.
2) Improved object type initialization structure. (to make generic constructors, so your patches can be simplified even more).
3) Introduced the ArrayBuffer object. (your patch)
4) fix1.patch
5) Introduced TypedArray objects. (your patch)
6) fix2_2.patch

I removed BigUint64Array, BigInt64Array because they require BigInt primitive which is a separate task. After it, adding them would be a trivial patch.

Will continue tomorrow.

@tiago4orion

BTW (just letting you know),

https://gist.github.com/tiago4orion/d03e7660518739d5fae7e1e87e806c10#file-njs_typed_array-patch-L2454

this code is a bad idea
1) it slows down other code passes
2) the code should be moved into njs_property_query()
because many other internal functions also need this
3) they also need a descriptor property like with NJS_ARRAY see this https://github.com/nginx/njs/blob/master/src/njs_value.c#L709

For this to work as expected

> var arr = new Int32Array(10); Object.getOwnPropertyDescriptor(arr, '3')
{ value: 0,
  writable: true,
  enumerable: true,
  configurable: false }

On it.

@tiago4orion

Changed in the committed patch:
1) Introduced proper types hierarchy (TypedArray->%TypedArray%->Object).

2) added %TypedArray%.prototype.fill().
3) %TypedArray%.prototype.join() rewritten from scratch according to spec (using njs_chb_t API).
4) TypedArray constructors rewritten according to spec.
5) njs.dump() now outputs TypedArray instances as ${TypedArray.name} [...].
6) Many other small changes (Style, test262 related).

Thank you for contributing.

- - Passed 10974 tests (33.9%)
- - Failed 21397 tests (66.1%)
+ - Passed 11421 tests (35.3%)
+ - Failed 20950 tests (64.7%)
Was this page helpful?
0 / 5 - 0 ratings

Related issues

drsm picture drsm  路  3Comments

porunov picture porunov  路  3Comments

drsm picture drsm  路  4Comments

axipo picture axipo  路  3Comments

xbb123 picture xbb123  路  4Comments