Add %TypedArray%.prototype.sort([ compareFunction ]) support. #2437

AnthonyCalandra · 2018-07-25T15:42:04Z

Add %TypedArray%.prototype.sort([ compareFunction ]) support.

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

AnthonyCalandra · 2018-07-25T15:45:44Z

While testing the sorting functionality, I've noticed that if I do something like the following, I get unexpected output:

jerry> [0, 1, 0].sort(function() { return NaN; })
0,0,1
jerry> [0, 1, 0].sort(function() { return 0; })
1,0,0

When I execute the above in V8, the arrays remain as they were originally, but JerryScript seems to incorrectly "sort" them. Since this PR reuses the heap sort implementation, this behavior is also seen when sorting typedarrays.

AnthonyCalandra · 2018-07-28T03:24:24Z

jerry-core/ecma/base/ecma-helpers-sort.c

+  }
+
+  return ret_value;
+} /* ecma_helpers_array_heap_sort_helper */


Forgot to mention, really this code was just moved (almost) verbatim from ecma-builtin-array-prototype.c (see below). I made one slight modification, and that is a ecma_sort_compare_helper_t which is a function pointer that points to either the typedarray sort routine or the array routine. The reason for choosing not to reuse the array implementation for typedarrays is because there is unnecessary overhead (such as converting to a string, comparing, etc.) while a typedarray comparison is strictly Number based.

zherczeg · 2018-07-31T10:11:56Z

jerry-core/ecma/base/ecma-helpers-sort.c

+ *         Returned value must be freed with ecma_free_value.
+ */
+static ecma_value_t
+ecma_helpers_array_to_heap (ecma_value_t array[], /**< heap data array */


This looks like a builtin specific function, keep it in ecma/builtins. You can create a new file for them if needed though.

zherczeg · 2018-08-02T07:53:21Z

jerry-core/ecma/builtin-objects/ecma-builtin-helpers-sort.c

+ *         Returned value must be freed with ecma_free_value.
+ */
+static ecma_value_t
+ecma_builtin_helper_array_to_heap (ecma_value_t array[], /**< heap data array */


Please ecma_value_t *array_p style.

I just copied this over, but sure, I'll make the change.

zherczeg · 2018-08-02T07:55:13Z

jerry-core/ecma/builtin-objects/ecma-builtin-helpers-sort.c

+                                   int index, /**< current item index */
+                                   int right, /**< right index is a maximum index */
+                                   ecma_value_t comparefn, /**< compare function */
+                                   const ecma_sort_compare_helper_t sortfn) /**< sorting function */


Use sort_compare_cb.

zherczeg · 2018-08-02T07:58:30Z

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

+
+  ecma_value_t ret_value = ECMA_VALUE_EMPTY;
+
+  JMEM_DEFINE_LOCAL_ARRAY (values_buffer, typedarray_length, ecma_value_t);


Do we actually need allocating memory? I think the heap-sort is capable of using the arraybuffer buffer for sorting (in-place sorting). This cannot be done for normal arrays since they don't have a buffer.

I don't think it would work if I just gave the heap-sort helper an underlying buffer without having to change the implementation of the helper -- then that means my helper isn't generic anymore. The heap-sort seems to work on ecma_value_ts and I don't think I can just map variable-sized (ie. 8-bit integers, 32-bit floats, etc.) arraybuffer elements to it?

Ok, at least this is good for binary size. We can optimize the code later if needed, since heapsort is not exactly a complicated algorithm.

AnthonyCalandra · 2018-08-07T14:19:55Z

I've addressed your remaining comments.

zherczeg · 2018-08-08T10:31:47Z

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

  /* Sorting. */
  if (copied_num > 1 && ecma_is_value_empty (ret_value))
  {
+    const sort_compare_cb sortfn = &ecma_builtin_array_prototype_object_sort_compare_helper;


I might wasn't clear. Ok, here is an example from jerry-core.h:

/** * Function type for allocating buffer for JerryScript instance. */ typedef void *(*jerry_instance_alloc_t) (size_t size, void *cb_data_p); /** * Type information of a native pointer. */ typedef struct { jerry_object_native_free_callback_t free_cb; /**< the free callback of the native pointer */ } jerry_object_native_info_t;

So we put _t after types, including callbacks. However, for the names of the variables we put _cb for callbacks, _p for pointers. This was the rule way before I joined to the project. So the correct form looks like this:

const sort_compare_t sort_cb = &ecma_builtin_array_prototype_object_sort_compare_helper;

I understand now. Made the change.

zherczeg · 2018-08-09T06:42:21Z

jerry-core/ecma/builtin-objects/ecma-builtin-helpers-sort.c

+ *         Returned value must be freed with ecma_free_value.
+ */
+static ecma_value_t
+ecma_builtin_helper_array_to_heap (ecma_value_t *array, /**< heap data array */


the _p postfix is still missing. All pointers require a _p in JerryScript. I know the original style is wrong, but lets fix it if we change it.

Sorry, not used to this style.

zherczeg · 2018-08-09T06:42:36Z

jerry-core/ecma/builtin-objects/ecma-builtin-helpers-sort.c

+ecma_builtin_helper_array_to_heap (ecma_value_t *array, /**< heap data array */
+                                   int index, /**< current item index */
+                                   int right, /**< right index is a maximum index */
+                                   ecma_value_t comparefn, /**< compare function */


compare_func

zherczeg · 2018-08-09T06:43:00Z

jerry-core/ecma/builtin-objects/ecma-builtin-helpers-sort.c

+ *         Returned value must be freed with ecma_free_value.
+ */
+ecma_value_t
+ecma_builtin_helper_array_heap_sort_helper (ecma_value_t *array, /**< array to sort */


These two needs to be fixed as well.

zherczeg · 2018-08-09T06:43:41Z

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

+static ecma_value_t
+ecma_builtin_typedarray_prototype_sort_compare_helper (ecma_value_t j, /**< left value */
+                                                       ecma_value_t k, /**< right value */
+                                                       ecma_value_t comparefn) /**< compare function */


Fix all instances :)

zherczeg · 2018-08-09T06:45:00Z

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

+  ecma_value_t ret_value = ECMA_VALUE_EMPTY;
+  ecma_number_t result = ECMA_NUMBER_ZERO;
+  bool j_is_undef = ecma_is_value_undefined (j);
+  bool k_is_undef = ecma_is_value_undefined (k);


How can this be undefined for a typed array?

I don't know either... I thought I had left this in for a reason but I guess I just forgot to remove it.

zherczeg · 2018-08-09T06:47:01Z

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

+  {
+    if (k_is_undef)
+    {
+      result = ECMA_NUMBER_ZERO;


You can just use return ecma_make_integer_value (0);

If you use return, less else blocks and indentation is needed, which makes the code more readable.

I've cleaned up the code a little bit since I don't need to check if j and k are undefined, see if that helps.

zherczeg

This patch gradually becomes better.

zherczeg · 2018-08-13T12:36:35Z

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

+static ecma_value_t
+ecma_builtin_typedarray_prototype_sort_compare_helper (ecma_value_t lhs, /**< left value */
+                                                       ecma_value_t rhs, /**< right value */
+                                                       ecma_value_t comparefn) /**< compare function */


Still there are some comparefn in the code. Please search trough the patch.

zherczeg · 2018-08-13T12:38:45Z

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

-                                                                                (int)(copied_num - 1),
-                                                                                arg1),
+                    ecma_builtin_helper_array_heap_sort_helper (values_buffer,
+                                                                (int)(copied_num - 1),


There should be a space after casting. It is strange that style checkers don't catch it. See your own (double) castings below. There are multiple instances of this style issue, please fix all.

I am thinking about the int type here. ES allows the full uint32 range for array indicies. However we cannot allocate a local buffer that big, so it might be not possible to use that big numbers anyway. What is your opinion?

Wouldn't hurt to be consistent with ES and "allow" the full range even if we have hardware limitations, IMHO.

zherczeg · 2018-08-13T12:42:30Z

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

+  JERRY_ASSERT (ecma_op_is_callable (comparefn));
+  ecma_object_t *comparefn_obj_p = ecma_get_object_from_value (comparefn);
+
+  ecma_value_t compare_args[] = {lhs, rhs};


Space before lfs, and after rhs.

AnthonyCalandra · 2018-08-14T20:40:26Z

jerry-core/ecma/builtin-objects/ecma-builtin-helpers-sort.c

+  ecma_value_t ret_value = ECMA_VALUE_EMPTY;
+
+  /* First, construct the ordered binary tree from the array. */
+  for (uint32_t i = (right / 2) + 1; i > 0 && ecma_is_value_empty (ret_value); i--)


To allow having the type of the index a uint32_t I made a small change here where I added one more to i and changed the condition from i >= 0 to i > 0 because before you would decrement when i = 0 (which would previously terminate the loop) but after making the value unsigned this would wrap to the max unsigned value and so the loop would never terminate.

AnthonyCalandra · 2018-08-14T20:40:46Z

jerry-core/ecma/builtin-objects/ecma-builtin-helpers-sort.c

+  for (uint32_t i = (right / 2) + 1; i > 0 && ecma_is_value_empty (ret_value); i--)
+  {
+    ECMA_TRY_CATCH (value,
+                    ecma_builtin_helper_array_to_heap (array_p, i - 1, right, compare_func, sort_cb),


Also changed i to i - 1; see reason above.

zherczeg

LGTM

zherczeg · 2018-08-15T06:38:28Z

jerry-core/ecma/builtin-objects/ecma-builtin-helpers-sort.c

+  for (uint32_t i = (right / 2) + 1; i > 0 && ecma_is_value_empty (ret_value); i--)
+  {
+    ECMA_TRY_CATCH (value,
+                    ecma_builtin_helper_array_to_heap (array_p, i - 1, right, compare_func, sort_cb),


LaszloLango · 2018-08-21T07:35:10Z

jerry-core/ecma/builtin-objects/ecma-builtin-helpers.h

+/**
+ * Comparison callback function header for sorting helper routines.
+ */
+typedef ecma_value_t (*sort_compare_t)(ecma_value_t lhs, /**< left value */


Please use the ecma_builtin_helper_ prefix: ecma_builtin_helper_sort_compare_fn_t

@LaszloLango Done.

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

LaszloLango

Thanks for the update. LGTM

AnthonyCalandra force-pushed the typedarray-sort-support branch from dad959b to 6e08652 Compare July 25, 2018 17:41

AnthonyCalandra commented Jul 28, 2018

View reviewed changes

zherczeg requested changes Jul 31, 2018

View reviewed changes

AnthonyCalandra force-pushed the typedarray-sort-support branch from 6e08652 to 5aa0470 Compare July 31, 2018 19:57

zherczeg requested changes Aug 2, 2018

View reviewed changes

AnthonyCalandra force-pushed the typedarray-sort-support branch from 5aa0470 to b3edf88 Compare August 7, 2018 14:19

zherczeg reviewed Aug 8, 2018

View reviewed changes

AnthonyCalandra force-pushed the typedarray-sort-support branch from b3edf88 to e8b4f7b Compare August 8, 2018 17:53

zherczeg requested changes Aug 9, 2018

View reviewed changes

AnthonyCalandra force-pushed the typedarray-sort-support branch from e8b4f7b to 3a01f25 Compare August 10, 2018 18:27

zherczeg requested changes Aug 13, 2018

View reviewed changes

AnthonyCalandra force-pushed the typedarray-sort-support branch 3 times, most recently from adead3d to 5a834dc Compare August 14, 2018 19:44

AnthonyCalandra commented Aug 14, 2018

View reviewed changes

zherczeg approved these changes Aug 15, 2018

View reviewed changes

akosthekiss added ecma builtins Related to ECMA built-in routines ES2015 Related to ES2015 features labels Aug 17, 2018

LaszloLango requested changes Aug 21, 2018

View reviewed changes

AnthonyCalandra force-pushed the typedarray-sort-support branch from 5a834dc to 9f0f09c Compare August 21, 2018 20:05

Add %TypedArray%.prototype.sort([ compareFunction ]) support.

35afa52

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

AnthonyCalandra force-pushed the typedarray-sort-support branch from 9f0f09c to 35afa52 Compare August 21, 2018 20:59

LaszloLango approved these changes Aug 22, 2018

View reviewed changes

LaszloLango merged commit b2cf7eb into jerryscript-project:master Aug 22, 2018


		ecma_value_t ret_value = ECMA_VALUE_EMPTY;

		JMEM_DEFINE_LOCAL_ARRAY (values_buffer, typedarray_length, ecma_value_t);

Add %TypedArray%.prototype.sort([ compareFunction ]) support. #2437

Add %TypedArray%.prototype.sort([ compareFunction ]) support. #2437

Uh oh!

Conversation

AnthonyCalandra commented Jul 25, 2018

Uh oh!

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

Uh oh!

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 Aug 2, 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!

AnthonyCalandra commented Aug 7, 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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaszloLango left a comment

Choose a reason for hiding this comment

Uh oh!

AnthonyCalandra commented Jul 25, 2018 •

edited

Loading

AnthonyCalandra Aug 2, 2018 •

edited

Loading