[ES2015 profile]add TypedArray intrinsic object #1507

jiangzidong · 2016-12-25T11:44:20Z

add %TypedArray% intrinsic object
implement Int8Array
will implement other types and prototype functions
in the following patches.

Each typedarray object has following struct

ecma_object_t
extended_part (it is class type, with class id and array length)
typedarray_info_t (contains the internal arraybuffer object, byteoffset and element size)

Also it fixed a bug for arraybuffer. Each bytes in the buffer should be initialized to 0.

JerryScript-DCO-1.0-Signed-off-by: Zidong Jiang [email protected]

zherczeg · 2016-12-27T04:43:19Z

Thank you for the hard work. It is Christmas time so we will be slower to respond.

zherczeg

Nice work. I have some design questions though as first step. To my understanding, typed arrays behave the same way as normal arrays except their values are limited.

zherczeg · 2016-12-27T04:44:01Z

jerry-core/CMakeLists.txt

@@ -165,7 +165,8 @@ endif()

 # Profile modes
 set(CONFIG_DISABLE_ES2015
-    CONFIG_DISABLE_ARRAYBUFFER_BUILTIN)
+    CONFIG_DISABLE_ARRAYBUFFER_BUILTIN
+    CONFIG_DISABLE_TYPEDARRAY_BUILTIN)


This will be reworked in #1497

#1497 is just landed. Please check the changes.

zherczeg · 2016-12-27T04:52:57Z

jerry-core/ecma/base/ecma-globals.h

+typedef struct
+{
+  /** The arraybuffer object behind the typedarray */
+  ecma_object_t *viewed_arraybuffer_p;


Cannot we use ECMA_GET_INTERNAL_VALUE_POINTER for the value part of the external object?

Do you mean use ecma_value_t to store the value? or use extend_object_t.class_prop.u.value? I already use extend_object_t.class_prop.u.length to store the array length

You said below: It contains another object instance arraybuffer inside, and multiple typedarray may share the same arraybuffer

In this case the length comes from the viewed arraybuffer doesn't it? Why do we need to duplicate it?

the length is the typedarray's length, not the arraybuffer's length
for example,

var a = new Int8Array(10); //it will contain an arraybuffer whose length is 10, and its own length is also 10 var b = new int8Array(a.buffer, 2, 5); //b and a will share the same arraybuffer, and b' length is 5

Ah, so a data view can be a sub-array. Interesting. Then what about this layout:

Use the normal extended object when offset = 0, and length = buffer.length. I think this is the most common case, and we can save space.

When this is not true, allocate an extra 8 byte, where the offset and length are stored.

I think the default array-length should buffer-length / bytes_per_element

Hm, I have an idea which could solve two issues:

Add a byte to extended object, which is

0 for non-typed-arrays

log2(item_length)+1 for typed-arrays

This would:

heavily simplify ecma_is_typedarray

length could be computed as (buffer-length) >> (this byte-1)

zherczeg · 2016-12-27T04:55:32Z

jerry-core/ecma/base/ecma-globals.h

+  /** the byteoffset of the above arraybuffer */
+  ecma_length_t byte_offset;
+  /** the size of each element in the typedarray */
+  ecma_length_t bytes_per_element;


There are two unused bytes in the class object. One could be used to store this number (is it a type btw?), and the other the byte offset if it is less than 255 (value zero has high chance). This could reduce memory consumption.

byteoffset can be more than 255.

I suspect this is a read-only property, so at construction time we can decide whether the offset is >= 255, and allocate extra space for it. But I think 0 is the most common value so we don''t need to allocate extra space in most cases.

Yes, we can do this.

About the 2 unused byte, do you mean the following part because of the 4-byte alignment?

struct { uint16_t class_id; // 2 byte union { ecma_value_t value; uint32_t length; } u; //4 byte uint16_t unused_2_bytes; } class_prop;

Yes, but the unused_2_bytes are right after the class_id.

zherczeg · 2016-12-27T04:57:24Z

jerry-core/ecma/operations/ecma-typedarray-object.c

+      TYPEDARRAY_SET_INDEXED_ELEMENT (int16_t);
+      break;
+    }
+    case LIT_MAGIC_STRING_UINT16_ARRAY_UL:


If I understand correctly, little endian and big endian access are supproted, so this should be more sophisticated. Anyway, I think we should only focus on int8 arrays in this patch, and just create the infrastructure.

Yes, and that's the reason why I implement int8array first. I thought about the endian problem before, and i think the C compiler will do this for us. The convert in TypedArray is not binary reinterpreter, but type-cast, e.g. 36.7(double) -> 36 (int8) 255(uint32) -> -1(int8)

Endian problem is not a type cast problem, it is writing the bytes of a number in different order. This patch is big enough, and this is not needed at all for now.

I mean, native compiler will handle the Endian problem, and for us, it is only type-cast.
For example, we want to set 2 uint16_t number 0x1122 and 0x3344 in the Uint16Array, and convert to Uint8Array

var a = new Uint16Array(2); a[0]=0x1122; a[1]=0x3344; var b = new Uint8Array(a); //[0x22, 0x44]

In little endian, the data in the arraybuffer of a, is (hex) 22 11 44 33
after the convertion-to-uint8, the data in the arraybuffer of b, is (hex) 22 44

In big endian, the data in the arraybuffer of a, is (hex) 11 22 33 44
after the convertion-to-uint8, the data in the arraybuffer of b, is (hex) 22 44

How uint16_t value (0x1122, and 0x3344) stored in the memory, is handled by compiler. And how to change a uint16_t value to uint8_t, is also handled by compiler.

Am I correct about that?

zherczeg · 2016-12-27T05:02:42Z

jerry-core/ecma/operations/ecma-objects.c

+      if (ecma_is_typedarray (ecma_make_object_value (object_p)))
+      {
+        ecma_number_t num = ecma_string_to_number (property_name_p);
+        ecma_string_t *num_to_str = ecma_new_ecma_string_from_number (num);


var a = new Int8Array(5); a[3.1] = 65535; a[3.1] // yields 65535

It seems array these arrays only capture array indicies (UINT32 values except UINT32_MAX).

I dont understand this comments.
Actually

var a = new Int8Array(5); a[3.1] = 65535; a[3.1] // yield undefined

according to the spec
If the prop name is a number but not integer, the set or get is invalid

In firefox it definitely returned with 65535. In which browser do you test it?

chrome v55 and nodejs v7.3.0

9.4.5.5.a: the prop name "3.1" can be converted to number

9.4.5.5 2.c.i, in IntegerIndexedElementSet the number 3.1 is not integer, return false

So I think V8's implementation conforms to the spec.

A suggestion here:
Check that string type is ECMA_STRING_CONTAINER_UINT32_IN_DESC, and just simply read the uint32_number field of the string. In this case there is no need to call ecma_string_to_number and ecma_new_ecma_string_from_number. These are quite costly functions.

A question: does this should return 6 or not? If not, what is its return value and why?

var a = new Int8Array(100); a[40] = 6; a["4e1"]

this is a very interesting part... and your case should return undefined

the chrome result:

var a = new Int8Array(10); >undefined a[5]=50 >50 a[5.0] >50 a["5"] >50 a["5.0"] >undefined a["5e0"] >undefined

Because the spec 7.1.16 use the string-number-string conversion to check if the prop name is CanonicalNumericIndexString. "5e1", "5.0" are not CanonicalNumericIndexString according to the criteria, and prop will be searched as normal way. e.g.

Int8Array.prototype["5e1"]="proto 5e1"; var a = new Int8Array(10); a[5]=50 a["5e1"] >"proto 5e1"

Hm. so a[5.0] is converted to a[5] during parsing. That makes sense.

And we basically needs the double conversion to return with undefined for "canonical numbers". (ECMA_STRING_CONTAINER_UINT32_IN_DESC handles all normal cases).

sorry, I dont understand what does "ECMA_STRING_CONTAINER_UINT32_IN_DESC handles all normal cases" mean in your last reply.

In JerryScript, strings which are also valid array indicies (e.g. "34", "0", "64567") are stored as uint32 numbers to reduce space and improve array operations. For example, in case of a[i] the 'i' must be converted to string first according to the standard. This "conversion" is cheap if i is an uint32 number or a string (these two are the most frequent cases).

The uint32 "strings" are also represent the canonical forms of each decimal integer number between 0 and UINT32_MAX. Since we will not support array buffers bigger than UINT32_MAX, only these uint32 "strings" can be valid array indicies. All other numbers are non integer numbers or not in canonical form, so they can be rejected (return with undefined) without decoding their number values.

So the pseudo code would look like this:

if (ECMA_STRING_GET_CONTAINER (string_p) == ECMA_STRING_CONTAINER_UINT32_IN_DESC) return get_value(string_p->u.uint32_number) else if (is_string_is_number(string_p)) return undefined else normal_code_path

The only question is what happens with "-0.0"

zherczeg · 2016-12-27T05:04:24Z

jerry-core/ecma/base/ecma-globals.h

@@ -79,6 +79,7 @@ typedef enum
  ECMA_SIMPLE_VALUE_UNDEFINED, /**< undefined value */
  ECMA_SIMPLE_VALUE_NULL, /**< null value */
  ECMA_SIMPLE_VALUE_NOT_FOUND, /**< a special value returned by ecma_op_object_find */
+  ECMA_SIMPLE_VALUE_NOT_FOUND_AND_STOP, /** not found and not search the proto chain */


Why don't we return with (virtual) undefined in this case? Is there a situation where we need to know that the value is not found?

ecma_op_object_find use "ECMA_SIMPLE_VALUE_NOT_FOUND" to indicate the prop is not in the object. While for typedarray, when the prop name is a number, and the prop is not in the own object, it should not search in the prototype chain any more. So I add the "NOT_FOUND_AND_STOP"

I understand what it does, the question is: do we really need it? What about simply return with undefined virtual data?

Oh, I didn't notice that. I will fix it soon

jiangzidong · 2016-12-27T05:44:07Z

@zherczeg Merry Christmas and Happy new year :) Totally understand, and just review it after your vacation

I think TypedArray has much diffs with normal Array.

length is fixed after instantiation, and all elements are default zeros
It contains another object instance arraybuffer inside, and multiple typedarray may share the same arraybuffer
the set/get/define prop operation is special from any other JS type. Integer Indexed Exotic Objects

zherczeg · 2016-12-28T04:57:25Z

jerry-core/ecma/operations/ecma-typedarray-object.c

+ *         False: it is not object or it is not a typedarray
+ */
+bool
+ecma_is_typedarray (ecma_value_t target) /**< the target need to be checked */


It seems this costly check called on hot-paths. We should avoid it to get a fast engine. Perhaps we need to invent something clever instead of it.

what about this:

we add extra uint16_t after class_id

struct { uint16_t class_id; uint16_t extra; union { ecma_value_t value; uint32_t length; } u; } class_prop;

some bits of extra indicates the class group the object belongs to. in our case, the group is typedarray, and we can check if an object is typedarray by checking those bits

some bits of the extra indicates if the typedarray allocates extra space to store the offset and array_length. the remaining bits can indicate the element size. all those definition of those bits are depends on the class of the object

the class_prop.u.value still store the internal arraybuffer object

for any other CLASS_TYPE objects, we should set extra to some proper initial value when allocate the object.

Hm your idea is even better. We could use one byte as generic flags: (e.g. typed array, etc.) and another byte for type specific (extra) data. This byte could store the log2(item_length) (without the +1). This sounds good.

yes, and I think just use one byte can do the all things for typedarray for now

first 4 bits indicate the class group of the object (e.g. typedarray) and it can support 16 different group for future extension

1 bit indicate whether the object need allocate other space for length and offset

3 bits (0-7) show the item-length (1 - 64)

In order to maintain the readability of the code, I think your proposal is better: one byte as generic flag and another byte for specific data :)

Do you think we need something else for typed arrays, which will use the spare byte? If not, we can use it (that byte will always be type dependent). I would prefer to store log2(item_length), since << and >> are cheap while integer division is costly.

Furthermore we could simply use one bit for typed array, and one bit for length/offset. I wouldn't want to squeze all JS object types into object with class, only simple ones (boolean, string, etc., whose have a single type and and a single ecma-value data). Using it for typed arrays already reaching its boundaries :)

Btw loading a byte is faster (1 instruction) than loading a word, shifting and masking (at least 3 instruction), and does not create data dependency (superscalar CPUs like it).

zherczeg · 2016-12-28T05:02:48Z

Based on this lot of code typed arrays are not exactly designed for small devices. So we really need to figure out how can we keep the binary size increase and performance drop at bay. Optimization is key for that.

jiangzidong · 2016-12-29T01:50:22Z

@zherczeg I think we provide different levels of profiles to let users to choose which one is suitable for device and application. For really small devices, typedarray is not suitable, and maybe we had to choose the minimal profile (like we did in curiebsp port).

V8 needs about 10M ram to run, I think JerryScript's target device range should be xx KB ram to x MB ram, and the larger devices may need more features.

TypedArray is designed to handle the raw data in the buffer, and should be useful in embedded/iot/wearable area. For example, W3C;s sensor API and bluetooth API are using it.

Yes, Optimization is very important for those feature enhancement, and I will pay more attention on it. Your suggestions/experiments are very valuable for me. Thanks very much!

zherczeg · 2016-12-29T06:20:27Z

tests/jerry-test-suite/es2015/22/22.02/22.02.06/22.02.06-001.js

+ * limitations under the License.
+ */
+
+assert(Int8Array.prototype.BYTES_PER_ELEMENT === 1);


Please add newlines to these tests.

zherczeg · 2016-12-29T07:27:13Z

I have been thinking on another idea. Non-strict arguments (ECMA_OBJECT_TYPE_ARGUMENTS) could also be a grouping for typed arrays, since they are both pseudo arrays. Unless it makes things more complicated.

jiangzidong · 2016-12-29T07:59:46Z

@zherczeg arguments object is defined in (spec 9.4.4) [http://www.ecma-international.org/ecma-262/6.0/#sec-arguments-exotic-objects], while typedarray is 9.4.5. The details has some differents. e.g

function f(){return arguments}
var a = f(1,2,3)
a[5]=100;
a[5]
>100

while to typedarray obj, the set for index which is exceed the length is invalid operation

But they maybe can share some common functions.

zherczeg · 2016-12-30T14:17:37Z

jerry-core/ecma/operations/ecma-typedarray-object.c

+  else
+  {
+    object_p = ecma_create_object (proto_p,
+                                   sizeof (ecma_extended_object_t) + sizeof (ecma_typedarray_info_t),


This can be incorrect because of padding. Let me show an example:

typedef struct { uint8_t a; } struct1; typedef struct { double b; } struct2;

sizeof(struct1)+sizeof(struct2) == 1+8 == 9 bytes

However:

typedef struct { uint8_t a; double b; } struct3;

sizeof(struct3) == 16 bytes, because the compiler inserts 7 padding bytes after the a member to align the double to 8 bytes.

The correct form for struct2 looks like the following:

typedef struct { struct1 header; double b; } struct2;

There is an example for this in JerryScript when extended and builtin objects are connected.

I can use another struct which contains the extended_object_t and typedarray_info_t, but I don't think it is necessary in this case.

If there is a container struct, let's say extended_typedarray_t , we cannot say sizeof (extended_typedarray_t) === sizeof(extended_object_t) + sizeof(typedarray_info_t) because of the padding.

Now, it didn't allocate a container struct , but allocate a section of memory. We define the former sizeof(ecma_extended_object_t) length part is to store ecma_extended_object_t, and the remaining part (whose length is sizeof(ecma_typedarray_info_t)) is to store ecma_typedarray_info_t.

Please correct me if I missed anything.

I think I understand your explanation. We always allocate a section of memory. The question is how we interpret the memory. The combined stucture is only used when the extra memory is needed.

Example:

bool is_extended = compute_whether_extended_is_needed (); ptr_p = (base_struct_t *) malloc(is_extended ? sizeof (base_struct_t) : sizeof (extended_struct_t)); ptr_p->something = 5; /* Fill base members. */ if (is_extended) { extended_struct_t *ext_ptr_p = ((extended_struct_t *) ptr_p) ext_ptr_p->something = 6; /* Fill extended members. */ }

Ok, I will use the container struct soon

zherczeg · 2016-12-31T05:40:20Z

Regarding the arguments object, it has two members, a scope pointer and a length. The length is 32 bit long atm. However, the maximum number of arguments is 255, and I think it will never be bigger than 65535. So it could be reduced to uint16_t, and we would get two extra bytes for the other stuff. Only one bit is needed to tell whether we use an arguments or an exotic array object. This way all pseudo arrays would go into one group.

jiangzidong · 2016-12-31T07:48:21Z

@zherczeg Do you mean add a new class_id for arguments, and implement its set/get/define/list prop operation which may be similar with typedarray (but not same) ?

zherczeg · 2017-01-01T04:43:05Z

Yes, rename it to something else (pseudo array or exotic array). This way we don't need to overload objects with classes.

jiangzidong · 2017-01-01T12:46:39Z

Do you mean that we define a new member of ecma_object_type_t. e.g ECMA_OBJECT_TYPE_EXOTIC_ARRAY, so that typedarray will no longer belong to "ECMA_OBJECT_TYPE_CLASS"?

if so, the bits for ecma_object_type_t is now 3, and we need more bits to support more than 7 types

zherczeg · 2017-01-01T15:50:37Z

No. Change ECMA_OBJECT_TYPE_ARGUMENTS to something more generic, e.g. ECMA_OBJECT_TYPE_PSEUDO_ARRAY and this new type should handle both typed arrays and non-strict arguments.

jiangzidong · 2017-01-03T02:30:53Z

@zherczeg I found it is difficult to design the pseudo array struct to store both typedarray or arguments.

Arguments need ecma_value_t for lev, and uint16_t for length
typedarray need ecma_value_t for araybuffer, uint16_t for specific typedarray class id (e.g Int8Array), and uint8_t for element_width_shift (the element size length)

Considering the padding thing, my proposed struct is

    struct
    {
      uint8_t type; //pseudo array type, e.g. Arguments, TypedArray or TypedArray _WITH_INFO
      uint8_t extra_info; // only for typedarray to store the element width shift
      union
     {
        uint16_t class_id:// for typedarray
        uint16_t length // for argument
     } u1;
     union
     {
       ecma_value_t lex_env_cp; // for argument
       ecma_value_t arraybuffer; // for typedarray
     } u2;
    } pseudo_array;

What do you think about it?

zherczeg · 2017-01-03T06:34:25Z

Your proposal is good for me.

jiangzidong · 2017-01-03T10:37:51Z

@zherczeg Updated the patch. Now both typedarray and arguments belong to the ecma_object_type_pseudo_array.

zherczeg

This is definitiely going in the right direction. Some comments though. Please squash everything and I will start reviewing everything again.

zherczeg · 2017-01-03T15:49:35Z

jerry-core/ecma/base/ecma-gc.c

+  #endif /* !CONFIG_DISABLE_TYPEDARRAY_BUILTIN */
+          default:
+          {
+            JERRY_UNREACHABLE ();


I think we add breaks to unreachable cases as well.

zherczeg · 2017-01-03T15:52:02Z

jerry-core/ecma/base/ecma-globals.h

-    } arguments;
+      uint8_t type; /**< pseudo array type, e.g. Arguments, TypedArray*/
+      uint8_t extra_info; /**< extra infomations of the object.
+                            *  for TypedArray, it is element_width_shift */


comment:
extra infomation about the object,
e.g. element_width_shift for typed arrays

zherczeg · 2017-01-03T15:53:01Z

jerry-core/ecma/operations/ecma-objects-arguments.c

@@ -264,7 +265,8 @@ ecma_op_arguments_object_define_own_property (ecma_object_t *object_p, /**< the

  ecma_extended_object_t *ext_object_p = (ecma_extended_object_t *) object_p;

-  if (index >= ext_object_p->u.arguments.length)
+  if (index >= ext_object_p->u.pseudo_array.u1.length
+)


Syntax error.

zherczeg · 2017-01-03T15:53:13Z

jerry-core/ecma/operations/ecma-objects-arguments.c

@@ -290,7 +292,8 @@ ecma_op_arguments_object_define_own_property (ecma_object_t *object_p, /**< the
    {
      /* emulating execution of function described by MakeArgSetter */
      ecma_object_t *lex_env_p = ECMA_GET_INTERNAL_VALUE_POINTER (ecma_object_t,
-                                                                  ext_object_p->u.arguments.lex_env_cp);
+                                                                  ext_object_p->u.pseudo_array.u2.lex_env_cp
+);


zherczeg · 2017-01-03T15:53:18Z

jerry-core/ecma/operations/ecma-objects-arguments.c

@@ -344,7 +347,8 @@ ecma_op_arguments_object_delete (ecma_object_t *object_p, /**< the object */
    {
      ecma_extended_object_t *ext_object_p = (ecma_extended_object_t *) object_p;

-      if (index < ext_object_p->u.arguments.length)
+      if (index < ext_object_p->u.pseudo_array.u1.length
+)


zherczeg · 2017-01-03T15:54:13Z

jerry-core/ecma/operations/ecma-typedarray-object.c

-    return ((ext_object_p->u.class_prop.class_type == ECMA_CLASS_TYPE_TYPEDARRAY)
-           || (ext_object_p->u.class_prop.class_type == ECMA_CLASS_TYPE_TYPEDARRAY_WITH_INFO));
+    return ((ext_object_p->u.pseudo_array.type == ECMA_PSEUDO_ARRAY_TYPEDARRAY)
+           || (ext_object_p->u.pseudo_array.type == ECMA_PSEUDO_ARRAY_TYPEDARRAY_WITH_INFO));


Again, missing space before ||

jiangzidong · 2017-01-09T00:49:15Z

@zherczeg I have squashed the commits, and solved the conflict with #1497

zherczeg

We are getting closer, thank you for the hard work. Next round of review.

zherczeg · 2017-01-09T10:28:47Z

jerry-core/ecma/base/ecma-globals.h

@@ -204,7 +204,7 @@ typedef enum

  ECMA_INTERNAL_PROPERTY_NATIVE_HANDLE, /**< native handle associated with an object */
  ECMA_INTERNAL_PROPERTY_FREE_CALLBACK, /**< object's native free callback */
-
+  ECMA_INTERNAL_PROPERTY_NOT_FOUND_AND_STOP, /**< the property is not found and no more search */


I had a question that whether we really need this? Can't we avoid it? Simply returning with virtual (ECMA_PROPERTY_TYPE_VIRTUAL) undefined as a value?

I think we need it, and the it is the same reason that why we need ECMA_INTERNAL_PROPERTY_NOT_FOUND. because, 'undefined' can be treated the value of a prop

var a = {}; a.b = undefined a.hasOwnProperty('b') // true Object.getOwnPropertyNames(a) // ['b']

We discussed that a["-1.1"] is undefined for typed arrays, but it has this property with value undefined or has not? If it has not then why it aborts the search?

in Chrome

>Int8Array.prototype['-1.1']='abc' "abc" >var a = new Int8Array(10); undefined >a['-1.1'] undefined >a['-1.1']=5 5 >a.hasOwnProperty('-1.1') false

'-1.1' will never be the property of typedarray, because it is a ' CanonicalNumeric', but it is not valid index.
because it is a 'CanonicalNumeric', the spec says it cannot search though the proto chain any more

That is convincing. Thank you for the example. Then we definitely need a way to stop searching.

zherczeg · 2017-01-09T10:29:37Z

jerry-core/ecma/base/ecma-globals.h

 } ecma_object_type_t;

 /**
+ * Types of objects with class property


Dot after sentence.

zherczeg · 2017-01-09T10:30:46Z

jerry-core/ecma/base/ecma-globals.h

+                            *  e.g. element_width_shift for typed arrays */
+      union
+      {
+        uint16_t length; /**< For arguments: length of names */


lowercase for

zherczeg · 2017-01-09T10:31:52Z

jerry-core/ecma/base/ecma-globals.h

+  /** extended object part */
+  ecma_extended_object_t extended_object;
+  /** the byteoffset of the above arraybuffer */
+  ecma_length_t byte_offset;


Put comments after type:

ecma_length_t byte_offset; /**< the byte offset of the above arraybuffer */

zherczeg · 2017-01-09T10:35:09Z

jerry-core/ecma/builtin-objects/ecma-builtin-int8array-prototype.c

+#include "ecma-typedarray-object.h"
+#include "ecma-try-catch-macro.h"
+#include "jrt.h"
+#include "jrt-libc-includes.h"


Please remove the unnecessary includes.

zherczeg · 2017-01-10T13:45:04Z

jerry-core/ecma/operations/ecma-typedarray-object.c

+  {
+    return 0;
+  }
+  else


Please just remove all these unnecessary elses.

zherczeg · 2017-01-10T13:45:22Z

jerry-core/ecma/operations/ecma-typedarray-object.c

+
+  ecma_value_t ret = ecma_make_simple_value (ECMA_SIMPLE_VALUE_EMPTY);
+
+


Only one newline.

zherczeg · 2017-01-10T13:46:37Z

jerry-core/ecma/operations/ecma-typedarray-object.c

+
+    if (num != ((ecma_number_t) length))
+    {
+      return ecma_raise_range_error (ECMA_ERR_MSG ("Invalid typedarray length."));


You cannot return in the middle of ECMA_OP_TO_NUMBER_TRY_CATCH.

Could you tell me the reason? Unlike ECMA_TRY_CATCH, ECMA_OP_TO_NUMBER_TRY_CATCH already free the number value., so I think it is ok to return

Am I right about it? or it is a coding standard?

These try-catch macros were invented before we joined the project, and the concept (coding standard) is you can never return inside a try-catch block, since the end part might free up variables. Now we use early returns, but never inside try-catch blocks. So either remove these try-catch blocks or use them as expected. I hope at some point we can delete all of them from the project.

I find it difficult to remove the return in the try-catch
for example

ECMA_OP_TO_NUMBER_TRY_CATCH(num1) if (expression1 is true) { return error1 } if (expression2 is true) { return error 2 } ECMA_OP_TO_NUMBER_TRY_CATCH(num2) if (expression3 is true) { return error3 } ret = process; ECMA_OP_TO_NUMBER_FINALIZE(num2) ECMA_OP_TO_NUMBER_FINALIZE(num1) return ret

I have to modified to

ECMA_OP_TO_NUMBER_TRY_CATCH(num1) if (expression1 is true) { ret = error1 } else if (expression2 is true) { ret = error 2 } else { ECMA_OP_TO_NUMBER_TRY_CATCH(num2) if (expression3) { ret = error 3 } else { ret = process } ECMA_OP_TO_NUMBER_FINALIZE(num2) } ECMA_OP_TO_NUMBER_FINALIZE(num1) return ret

the struct will be very complicated and hard to read.

Actually, the end part of ECMA_OP_TO_NUMBER_TRY_CATCH will not free vars, and it is safe to return in the middle of ECMA_OP_TO_NUMBER_TRY_CATCH block.

About deleting all of the macros, I think the try-catch block is needed in the project. op_to_number may throw exception. If we delete the current macros, we still have to implement the similar codes to check whether there is an exception.

This is the reason why I don't like these macros. But the point is: these macros can be changed anytime, so you cannot optimize the code that way. Perhaps you could introduce some helper functions to simplify the problem.

zherczeg · 2017-01-10T13:50:13Z

jerry-core/ecma/operations/ecma-typedarray-object.c

+      TYPEDARRAY_GET_INDEXED_ELEMENT (uint8_t);
+      break;
+    }
+    case LIT_MAGIC_STRING_INT16_ARRAY_UL:


We should not add other types to this patch. There are corner cases we need to discuss first, and they just increase binary size at the moment.

Do you mean remove all codes with is related to other typedarrays except for Int8Array?

Exactly. They are dead code now.

About the endian problem,
I mean, native compiler will handle the Endian problem, and for us, it is only type-cast.
For example, we want to set 2 uint16_t number 0x1122 and 0x3344 in the Uint16Array, and convert to Uint8Array

var a = new Uint16Array(2); a[0]=0x1122; a[1]=0x3344; var b = new Uint8Array(a); //[0x22, 0x44]

In little endian, the data in the arraybuffer of a, is (hex) 22 11 44 33
after the convertion-to-uint8, the data in the arraybuffer of b, is (hex) 22 44

In big endian, the data in the arraybuffer of a, is (hex) 11 22 33 44
after the convertion-to-uint8, the data in the arraybuffer of b, is (hex) 22 44

How uint16_t value (0x1122, and 0x3344) stored in the memory, is handled by compiler. And how to change a uint16_t value to uint8_t, is also handled by compiler.

So jerry dont need to deal with the endian problem
Am I correct about that?

Is that mean there is no little-endian read and big-endian read in typed arrays like in DataView or something? If no, then yes, the CPU handles the endianness.

Yes, DataView has to handle the endianness, and TypedArray don't need to. I plan to implement it in the following DataView patch.
It is incorrect that "jerry don't need to deal with endianness", it should add the limit : "in TypedArray area"

Ok. I don't know exactly how these arrays work, just saw somewhere that you can do reads in both alignments, and wanted to make sure.

zherczeg · 2017-01-10T13:52:30Z

jerry-core/ecma/operations/ecma-typedarray-object.c

+      TYPEDARRAY_SET_INDEXED_ELEMENT (int16_t);
+      break;
+    }
+    case LIT_MAGIC_STRING_UINT16_ARRAY_UL:


Endian problem is not a type cast problem, it is writing the bytes of a number in different order. This patch is big enough, and this is not needed at all for now.

zherczeg · 2017-01-11T09:11:37Z

jerry-core/ecma/base/ecma-globals.h

@@ -292,6 +292,11 @@ typedef enum
 #define ECMA_PROPERTY_TYPE_NOT_FOUND ECMA_PROPERTY_TYPE_DELETED

 /**
+ * Type of property not found and no more searching in the proto chain.
+ */
+#define ECMA_PROPERTY_TYPE_NOT_FOUND_AND_STOP ECMA_SPECIAL_PROPERTY_VALUE (ECMA_INTERNAL_PROPERTY_NOT_FOUND_AND_STOP)


Could we use some other property value such ECMA_PROPERTY_TYPE_HASHMAP, which cannot be a valid return value?

Yes, I think we can
#define ECMA_PROPERTY_TYPE_NOT_FOUND_AND_STOP ECMA_PROPERTY_TYPE_HASHMAP
and in this way, we can remove ECMA_INTERNAL_PROPERTY_NOT_FOUND_AND_STOP

jiangzidong · 2017-01-12T13:27:42Z

@zherczeg Thanks. Updated according to the review comments.

zherczeg

LGTM after some minor changes.

zherczeg · 2017-01-13T08:20:33Z

jerry-core/ecma/base/ecma-globals.h

@@ -204,7 +204,6 @@ typedef enum

  ECMA_INTERNAL_PROPERTY_NATIVE_HANDLE, /**< native handle associated with an object */
  ECMA_INTERNAL_PROPERTY_FREE_CALLBACK, /**< object's native free callback */
-


Please dont remove this line.

zherczeg · 2017-01-13T08:29:30Z

jerry-core/ecma/operations/ecma-typedarray-object.c

+
+#define TYPEDARRAY_GET_INDEXED_ELEMENT(type) \
+  type v = *((type *) target_p); \
+  value = ecma_make_number_value ((ecma_number_t) v);


Please undef this macro or just remove it. This is just two lines, not really worth a create a local macro.

zherczeg · 2017-01-13T08:29:45Z

jerry-core/ecma/operations/ecma-typedarray-object.c

+    lit_utf8_byte_t *target_p = ecma_arraybuffer_get_buffer (arraybuffer_p) + byte_pos;
+
+  #define TYPEDARRAY_SET_INDEXED_ELEMENT(type) \
+    *((type *) target_p) = (type) value_num;


* add %TypedArray% intrinsic object * implement Int8Array * will implement other types and prototype functions in the following patches. JerryScript-DCO-1.0-Signed-off-by: Zidong Jiang [email protected]

LaszloLango

LGTM

zherczeg requested changes Dec 27, 2016

View reviewed changes

zherczeg reviewed Dec 28, 2016

View reviewed changes

zherczeg reviewed Dec 29, 2016

View reviewed changes

jiangzidong force-pushed the typedarray1220 branch from 38de786 to c136f5e Compare December 30, 2016 05:10

zherczeg reviewed Dec 30, 2016

View reviewed changes

jiangzidong force-pushed the typedarray1220 branch from d97c6f0 to e9c7c94 Compare January 3, 2017 10:36

zherczeg reviewed Jan 3, 2017

View reviewed changes

jiangzidong force-pushed the typedarray1220 branch from e9c7c94 to 61d3fe1 Compare January 4, 2017 00:34

zherczeg reviewed Jan 10, 2017

View reviewed changes

zherczeg reviewed Jan 11, 2017

View reviewed changes

jiangzidong force-pushed the typedarray1220 branch 2 times, most recently from 2852a78 to 17c9f08 Compare January 12, 2017 12:48

zherczeg approved these changes Jan 13, 2017

View reviewed changes

jiangzidong force-pushed the typedarray1220 branch from 17c9f08 to 3c80a42 Compare January 13, 2017 08:40

[ES2015 profile]add TypedArray intrinsic object

3c80a42

* add %TypedArray% intrinsic object * implement Int8Array * will implement other types and prototype functions in the following patches. JerryScript-DCO-1.0-Signed-off-by: Zidong Jiang [email protected]

LaszloLango approved these changes Jan 13, 2017

View reviewed changes

LaszloLango merged commit adfe61b into jerryscript-project:master Jan 13, 2017

zherczeg mentioned this pull request Jan 19, 2017

Nominating Zidong Jiang (@jiangzidong) to become a JerryScript committer #1535

Closed


		ecma_value_t ret = ecma_make_simple_value (ECMA_SIMPLE_VALUE_EMPTY);

		@@ -204,7 +204,6 @@ typedef enum

		ECMA_INTERNAL_PROPERTY_NATIVE_HANDLE, /*< native handle associated with an object /
		ECMA_INTERNAL_PROPERTY_FREE_CALLBACK, /*< object's native free callback /

[ES2015 profile]add TypedArray intrinsic object #1507

[ES2015 profile]add TypedArray intrinsic object #1507

Uh oh!

Conversation

jiangzidong commented Dec 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zherczeg commented Dec 27, 2016

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiangzidong Dec 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiangzidong Dec 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiangzidong commented Dec 25, 2016 •

edited

Loading

jiangzidong Dec 28, 2016 •

edited

Loading

jiangzidong Dec 29, 2016 •

edited

Loading

jiangzidong Dec 29, 2016 •

edited

Loading

zherczeg commented Dec 29, 2016 •

edited

Loading