Skip to content

[ES2015 profile]add TypedArray intrinsic object #1507

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 13, 2017

Conversation

jiangzidong
Copy link
Contributor

@jiangzidong jiangzidong commented Dec 25, 2016

  • 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
Copy link
Member

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

Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

# Profile modes
set(CONFIG_DISABLE_ES2015
CONFIG_DISABLE_ARRAYBUFFER_BUILTIN)
CONFIG_DISABLE_ARRAYBUFFER_BUILTIN
CONFIG_DISABLE_TYPEDARRAY_BUILTIN)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be reworked in #1497

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1497 is just landed. Please check the changes.

typedef struct
{
/** The arraybuffer object behind the typedarray */
ecma_object_t *viewed_arraybuffer_p;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

/** 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

byteoffset can be more than 255.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jiangzidong jiangzidong Dec 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but the unused_2_bytes are right after the class_id.

TYPEDARRAY_SET_INDEXED_ELEMENT (int16_t);
break;
}
case LIT_MAGIC_STRING_UINT16_ARRAY_UL:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chrome v55 and nodejs v7.3.0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"]

Copy link
Contributor Author

@jiangzidong jiangzidong Dec 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

@@ -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 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jiangzidong
Copy link
Contributor Author

@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

* 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 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jiangzidong jiangzidong Dec 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Copy link
Contributor Author

@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!

* limitations under the License.
*/

assert(Int8Array.prototype.BYTES_PER_ELEMENT === 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add newlines to these tests.

@zherczeg
Copy link
Member

zherczeg commented Dec 29, 2016

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
Copy link
Contributor Author

@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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. */
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will use the container struct soon

@zherczeg
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Member

zherczeg commented Jan 1, 2017

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

@jiangzidong
Copy link
Contributor Author

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
Copy link
Member

zherczeg commented Jan 1, 2017

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
Copy link
Contributor Author

@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
Copy link
Member

zherczeg commented Jan 3, 2017

Your proposal is good for me.

@jiangzidong
Copy link
Contributor Author

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

Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#endif /* !CONFIG_DISABLE_TYPEDARRAY_BUILTIN */
default:
{
JERRY_UNREACHABLE ();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we add breaks to unreachable cases as well.

} 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 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Syntax error.

@@ -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
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

@@ -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
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, missing space before ||

@jiangzidong
Copy link
Contributor Author

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

Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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']

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@jiangzidong jiangzidong Jan 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} ecma_object_type_t;

/**
* Types of objects with class property
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dot after sentence.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lowercase for

/** extended object part */
ecma_extended_object_t extended_object;
/** the byteoffset of the above arraybuffer */
ecma_length_t byte_offset;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put comments after type:

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

#include "ecma-typedarray-object.h"
#include "ecma-try-catch-macro.h"
#include "jrt.h"
#include "jrt-libc-includes.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the unnecessary includes.

{
return 0;
}
else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please just remove all these unnecessary elses.


ecma_value_t ret = ecma_make_simple_value (ECMA_SIMPLE_VALUE_EMPTY);


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one newline.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot return in the middle of ECMA_OP_TO_NUMBER_TRY_CATCH.

Copy link
Contributor Author

@jiangzidong jiangzidong Jan 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

TYPEDARRAY_GET_INDEXED_ELEMENT (uint8_t);
break;
}
case LIT_MAGIC_STRING_INT16_ARRAY_UL:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. They are dead code now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

TYPEDARRAY_SET_INDEXED_ELEMENT (int16_t);
break;
}
case LIT_MAGIC_STRING_UINT16_ARRAY_UL:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jiangzidong force-pushed the typedarray1220 branch 2 times, most recently from 2852a78 to 17c9f08 Compare January 12, 2017 12:48
@jiangzidong
Copy link
Contributor Author

@zherczeg Thanks. Updated according to the review comments.

Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after some minor changes.

@@ -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 */

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please dont remove this line.


#define TYPEDARRAY_GET_INDEXED_ELEMENT(type) \
type v = *((type *) target_p); \
value = ecma_make_number_value ((ecma_number_t) v);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

* 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]
Copy link
Contributor

@LaszloLango LaszloLango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants