Support for %TypedArray%.prototype.set(typedArray [, offset]). #2405

AnthonyCalandra · 2018-06-19T15:11:37Z

This patch allows developers to set a typedarray given a source typedarray. This patch
attempts to follow section 22.2.3.22.2 in the ECMAScript spec as closely as possible.

JerryScript-DCO-1.0-Signed-off-by: AnthonyCalandra anthony@anthony-calandra.com

zherczeg

Quite good patch.

zherczeg · 2018-06-20T06:16:44Z

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

+  /* 6.~ 8. targetOffset */
+  ecma_value_t ret_val = ECMA_VALUE_EMPTY;
+  ECMA_OP_TO_NUMBER_TRY_CATCH (target_offset_num, offset_val, ret_val);
+  if (ecma_number_is_nan (target_offset_num))


Please add a newline before this if and another below the second if.

zherczeg · 2018-06-20T06:21:44Z

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

+
+  /* 23. */
+  uint32_t target_offset_uint32 = ecma_number_to_uint32 (target_offset_num);
+  if ((int64_t) src_length_uint32 + target_offset_uint32 > target_length)


Could you explain the purpose of this if?

I want to make sure that the offset is within range of the target typedarray. For example, if the target has length 3 and the offset is 1000, it should throw a RangeError. I cast it to int64_t to account for overflow.

Hm, I checked that partial copy is not allowed. That is strange. Anyway this is an ok check then.

zherczeg · 2018-06-20T06:24:13Z

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

+  ecma_object_t *src_arraybuffer_clone_p = NULL;
+  bool src_target_same_value = ecma_op_same_value (ecma_make_object_value (src_arraybuffer_p),
+                                                   ecma_make_object_value (target_arraybuffer_p));
+  if (src_target_same_value)


If this is for "in-place-copy" just use memmove(), that works with any buffers, and you don'y need src_arraybuffer_clone_p.

That's true, I could probably just copy the buffer and operate on that instead of cloning an entire typedarray. I'll give it a try.

I've tried to do this but I'm running into an assertion error. The code I have looks like:

size_t target_clone_heap_size = (sizeof (lit_utf8_byte_t)) * target_element_size * target_length; lit_utf8_byte_t *target_clone_buffer_p = (lit_utf8_byte_t *) jmem_heap_alloc_block (target_clone_heap_size); memmove (target_clone_buffer_p, ecma_arraybuffer_get_buffer (target_arraybuffer_p), target_length); src_buffer_p = target_clone_buffer_p + src_byte_offset; // ...

And the assertion error is:

ICE: Assertion '(uintptr_t) data_space_p % JMEM_ALIGNMENT == 0' failed at /Users/acalandra/jerryscript/jerry-core/jmem/jmem-heap.c(jmem_heap_alloc_block_internal):335. Error: ERR_FAILED_INTERNAL_ASSERTION

Any idea how I can resolve this?

I would almost prefer the clone approach because it's closer to the spec and the code definitely looks cleaner.

Isn't this a nop? The check ensures above that only a.set(a,0) is valid, and that is effectively do nothing.

I suspect the error is caused by memory overwrite, asan or valgrind can tell you cause. The size of memove is definitely wrong, it should be target_clone_heap_size.

Regardless for in-place copy never clone a buffer. Allocating memory may increase peak memory consumption, CPU consuming, definitely against the aims of an engine for low-end systems.

I agree that this seems like a nop. When I read the spec it added the cloning step because of side-effects. I've been trying to think of a case where this would be an issue but I haven't come up with an example.

AnthonyCalandra · 2018-06-21T15:49:18Z

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

+
+  return object_p;
+} /* ecma_arraybuffer_clone_object */
+


Waiting to see if cloning an ArrayBuffer is necessary -- will remove if not.

AnthonyCalandra · 2018-06-22T19:49:05Z

I've added the memmove optimization and added a test to account for overlapping setting of elements.

zherczeg · 2018-06-23T03:35:05Z

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

+
+  if ((int64_t) src_length_uint32 + target_offset_uint32 > target_length)
+  {
+    ret_val = ecma_raise_range_error (ECMA_ERR_MSG ("Invalid range of index"));


You could return here as well.

zherczeg · 2018-06-23T03:35:59Z

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

+  /* 27. limit */
+  uint32_t limit = target_byte_index + target_element_size * src_length_uint32;
+
+  if (src_class_id == target_class_id && target_byte_index < limit && ecma_is_value_empty (ret_val))


No need |ecma_is_value_empty (ret_val).
What is the purpose of target_byte_index < limit ? It is used in the loop.

You're right, checking target_byte_index < limit is not needed here.

zherczeg · 2018-06-23T03:36:20Z

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

+  }
+  else
+  {
+    while (target_byte_index < limit && ecma_is_value_empty (ret_val))


Add an assert that src and target buffers cannot be the same.

Why is this required? The assertion fails if I want to do the following:

var ab = new ArrayBuffer(8); var a1 = new Uint8Array(ab); var a2 = new Uint32Array(ab, 4); a1.set(a2, 2);

a1 and a2 have different class names/IDs so the while loop gets executed even though they have the same buffer. Seems reasonable to me.

zherczeg · 2018-06-23T03:40:31Z

Ok, I get now where the buffer cloning comes from. They wanted to simplify the pseudo code in the standard so they only need forward direction data copy. This is how you write a standard. But not how you write code. Fortunately for in-buffer copy the type of src and target must match obviously, and memmove handles the forward/backward copy. This is its key difference from memcpy, where the result is unspecified for in-buffer copy.

AnthonyCalandra · 2018-06-25T02:31:00Z

Addressed all comments and I added a unit test for the case of forward/backward copying when the two buffers have different class IDs (thus using the while-loop algorithm).

AnthonyCalandra · 2018-06-28T01:24:44Z

Sorry to pester, but can I get some looks at this? Our goal at work is to have this available to us by the end of the week.

zherczeg · 2018-06-28T06:27:43Z

Who is 'our'?

zherczeg

It looks like there is a code duplication for computing offset. Please merge the two (keep yours, that is better).

zherczeg · 2018-06-28T06:31:21Z

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

+{
+  /* 6.~ 8. targetOffset */
+  ecma_number_t target_offset_num;
+  if (!ecma_is_value_empty (ecma_get_number (offset_val, &target_offset_num)))


This is not correct. Put the result into a temporary value, and if it is an error, return with it. Do not create another error. Otherwise do nothing, so no need to free ECMA_VALUE_xxx values.

zherczeg · 2018-06-28T06:34:45Z

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

+    target_offset_num = 0;
+  }
+
+  if (target_offset_num <= -1.0 || target_offset_num >= (ecma_number_t) UINT32_MAX + 0.5)


Rounding question: you check <= -1 and MAX+0.5. For consistency, it should be:

if (target_offset_num <= -0.5 || target_offset_num >= (ecma_number_t) UINT32_MAX + 0.5)

or

if (target_offset_num <= -1.0 || target_offset_num >= (ecma_number_t) UINT32_MAX + 1.0)

It depends on the rounding rules specified by the standard. I think C rounding is truncating by default.

This is the same rounding check as https://github.com/jerryscript-project/jerryscript/pull/2405/files#diff-f81c66825ff663e22d61a8e1fb0fa8a9R797, so I just copied it over. I don't know much about rounding rules so I left it alone.

I tried with latest clang and gcc on godbolt to try out the rounding rules and I think if (target_offset_num <= -1.0 || target_offset_num >= (ecma_number_t) UINT32_MAX + 1.0) is what we want. Not sure why the original author used 0.5 for the latter condition though.

zherczeg · 2018-06-28T06:37:07Z

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

+
+  /* 21. srcLength */
+  ecma_length_t src_length = ecma_typedarray_get_length (src_typedarray_p);
+  uint32_t src_length_uint32 = ecma_number_to_uint32 (src_length);


I think ecma_length_t is uint32_t. No need a conversion here. Definitely not a to double and back to int, which is rather costly.

zherczeg · 2018-06-28T06:37:28Z

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

+
+  if ((ecma_number_t) src_length_uint32 != src_length)
+  {
+    return ecma_raise_range_error (ECMA_ERR_MSG ("Invalid source length"));


This is impossible.

zherczeg · 2018-06-28T06:46:34Z

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

+  }
+
+  /* 22. srcByteOffset */
+  ecma_length_t src_byte_offset = ecma_typedarray_get_offset (src_typedarray_p);


This is used later, please move it before use.

zherczeg · 2018-06-28T06:48:20Z

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

+  /* 24.d, 25. srcByteIndex */
+  ecma_length_t src_byte_index = 0;
+
+  if (src_arraybuffer_p != target_arraybuffer_p)


Is src_byte_index is always 0 if the two array buffers are the same? I don't think this is true because of the subarray mechanics. This is probably not covered by a test, so please test it.

~~This is true according to the spec: https://www.ecma-international.org/ecma-262/6.0/#sec-%typedarray%.prototype.set-typedarray-offset.~~

~~Also covered by this test: https://github.com/jerryscript-project/jerryscript/pull/2405/files#diff-8618fbaec5e60cd450468b18a0d0b1ecR64~~

Actually, I'm not so sure. I'll look at the code again.

OK, so what happened was when we decided to go with the memmove approach and ditch the cloning, the spec sets srcByteIndex to zero because it clones the buffer starting at srcByteOffset. Since we don't do the cloning, we always need to start at srcByteOffset bytes in the buffer, and the tests didn't cover that edge-case.

zherczeg

Only minor changes remained.

zherczeg · 2018-06-29T07:52:23Z

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

+  lit_utf8_byte_t *target_buffer_p = ecma_arraybuffer_get_buffer (target_arraybuffer_p);
+
+  /* 11. targetLength */
+  ecma_length_t target_length = ecma_typedarray_get_length (target_typedarray_p);


Move target_length after target_length definition.

After which definition? I'll assume you meant src_length and move it there.

zherczeg · 2018-06-29T07:53:02Z

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

+  ecma_length_t src_byte_offset = ecma_typedarray_get_offset (src_typedarray_p);
+
+  /* 24.d, 25. srcByteIndex */
+  ecma_length_t src_byte_index = src_byte_offset;


I think src_byte_offset is unnecessary.

Do you mean src_byte_index? I was on the fence about that one because it gets mutated in the while loop but I guess it doesn't matter at this point.

AnthonyCalandra · 2018-06-29T14:29:40Z

Addressed all remaining comments.

zherczeg

LGTM

LaszloLango

Please fix the comment, good to me after that.

LaszloLango · 2018-07-02T06:05:22Z

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

+ * See also:
+ *          ES2015, 22.2.3.22, 22.2.3.22.2
+ *
+ * @return ecma value of undefined.


This is inaccurate. Please update.

* @return ecma value of undefined if success, error otherwise. * Returned value must be freed with ecma_free_value.

This patch allows developers to set a typedarray given a source typedarray. This patch attempts to follow section 22.2.3.22.2 in the ECMAScript spec as closely as possible. JerryScript-DCO-1.0-Signed-off-by: AnthonyCalandra anthony@anthony-calandra.com

LaszloLango

Thanks for the update, LGTM

AnthonyCalandra force-pushed the typedarray-set-support branch 3 times, most recently from d3d16c5 to 444e774 Compare June 19, 2018 17:26

zherczeg requested changes Jun 20, 2018

View reviewed changes

LaszloLango added development Feature implementation ES2015 Related to ES2015 features labels Jun 20, 2018

AnthonyCalandra force-pushed the typedarray-set-support branch from 444e774 to 023da39 Compare June 21, 2018 15:47

AnthonyCalandra commented Jun 21, 2018

View reviewed changes

AnthonyCalandra force-pushed the typedarray-set-support branch 3 times, most recently from 8ade614 to 11ea3b8 Compare June 22, 2018 19:47

zherczeg requested changes Jun 23, 2018

View reviewed changes

AnthonyCalandra force-pushed the typedarray-set-support branch from 11ea3b8 to 74c5037 Compare June 25, 2018 02:29

AnthonyCalandra force-pushed the typedarray-set-support branch from 74c5037 to 4cbe695 Compare June 25, 2018 17:05

zherczeg requested changes Jun 28, 2018

View reviewed changes

AnthonyCalandra force-pushed the typedarray-set-support branch 4 times, most recently from 75d1e07 to 09c21cf Compare June 28, 2018 17:40

zherczeg requested changes Jun 29, 2018

View reviewed changes

AnthonyCalandra force-pushed the typedarray-set-support branch from 09c21cf to 1d2137d Compare June 29, 2018 14:29

zherczeg approved these changes Jun 29, 2018

View reviewed changes

LaszloLango requested changes Jul 2, 2018

View reviewed changes

AnthonyCalandra force-pushed the typedarray-set-support branch from 1d2137d to d2b070a Compare July 3, 2018 01:39

LaszloLango approved these changes Jul 3, 2018

View reviewed changes

yichoi merged commit d5cd32b into jerryscript-project:master Jul 3, 2018

Support for %TypedArray%.prototype.set(typedArray [, offset]). #2405

Support for %TypedArray%.prototype.set(typedArray [, offset]). #2405

Uh oh!

Conversation

AnthonyCalandra commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

zherczeg Jun 20, 2018 • 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!

zherczeg Jun 21, 2018 • 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!

AnthonyCalandra commented Jun 22, 2018

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!

zherczeg commented Jun 23, 2018

Uh oh!

AnthonyCalandra commented Jun 25, 2018

Uh oh!

AnthonyCalandra commented Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zherczeg commented Jun 28, 2018

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!

AnthonyCalandra Jun 28, 2018 • 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!

AnthonyCalandra Jun 28, 2018 • 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!

AnthonyCalandra commented Jun 19, 2018 •

edited

Loading

zherczeg Jun 20, 2018 •

edited

Loading

zherczeg Jun 21, 2018 •

edited

Loading

AnthonyCalandra commented Jun 28, 2018 •

edited

Loading

AnthonyCalandra Jun 28, 2018 •

edited

Loading

AnthonyCalandra Jun 28, 2018 •

edited

Loading

AnthonyCalandra Jun 29, 2018 •

edited

Loading