-
Notifications
You must be signed in to change notification settings - Fork 684
[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
[ES2015 profile]add TypedArray intrinsic object #1507
Conversation
Thank you for the hard work. It is Christmas time so we will be slower to respond. |
There was a problem hiding this 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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"]
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 */ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
@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.
|
* 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 */ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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).
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. |
@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); |
There was a problem hiding this comment.
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.
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. |
@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
while to typedarray obj, the set for index which is exceed the length is invalid operation But they maybe can share some common functions. |
38de786
to
c136f5e
Compare
else | ||
{ | ||
object_p = ecma_create_object (proto_p, | ||
sizeof (ecma_extended_object_t) + sizeof (ecma_typedarray_info_t), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. */
}
There was a problem hiding this comment.
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
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. |
@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) ? |
Yes, rename it to something else (pseudo array or exotic array). This way we don't need to overload objects with classes. |
Do you mean that we define a new member of if so, the bits for ecma_object_type_t is now 3, and we need more bits to support more than 7 types |
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. |
@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 Considering the padding thing, my proposed struct is
What do you think about it? |
Your proposal is good for me. |
d97c6f0
to
e9c7c94
Compare
@zherczeg Updated the patch. Now both typedarray and arguments belong to the ecma_object_type_pseudo_array. |
There was a problem hiding this 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 (); |
There was a problem hiding this comment.
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 */ |
There was a problem hiding this comment.
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 | |||
) |
There was a problem hiding this comment.
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 | |||
); |
There was a problem hiding this comment.
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 | |||
) |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, missing space before ||
e9c7c94
to
61d3fe1
Compare
There was a problem hiding this 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 */ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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']
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 */ |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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); | ||
|
||
|
There was a problem hiding this comment.
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.")); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
2852a78
to
17c9f08
Compare
@zherczeg Thanks. Updated according to the review comments. |
There was a problem hiding this 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 */ | |||
|
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
17c9f08
to
3c80a42
Compare
* 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]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
in the following patches.
Each typedarray object has following struct
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]