Array.prototype.sort() should sort in place. #335

dbatyai · 2015-07-08T12:27:15Z

Array.prototype.sort() should sort in place instead of creating a new Array with the sorted elements.
JerryScript-DCO-1.0-Signed-off-by: Dániel Bátyai dbatyai.u-szeged@partner.samsung.com

egavrin · 2015-07-08T12:29:40Z

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

+
+    if (ecma_is_value_undefined (values_buffer[index]))
+    {
+      ECMA_TRY_CATCH (del_value, ecma_op_object_delete (obj_p, index_string_p, true), ret_value);


What is the reason to have empty TRY/FINALIZE block?

In my opinion this look cleaner than using if/else to check/free.

Also, this is current common style.
Maybe, this could be encoded in a more convenient way. I think, in the case it is better to change all the places at once.

egavrin · 2015-07-08T13:29:04Z

No other questions. Good to me.

ruben-ayrapetyan · 2015-07-08T13:37:42Z

tests/jerry/array-prototype-sort.js

+
+// Sorting sparse array
+var array = [1,,2,,3,,4,undefined,5];
+var expected = [1,2,3,4,5,,,,];


Seems that expected.length is 8, and array.length is 9. Maybe, it is incorrect.

Could you, please, check whether conditions in the following loop should be true?

for (i = 0; i < array.length; i++) { assert (expected.hasOwnProperty (i) == array.hasOwnProperty (i)); }

Currently, some of them are not true.

expected has 5 numbers, and then 4 empty places after those, that should be 9. Nevertheless, there's an assert for this in line 57 below.

The following test case print 8:

var expected = [1,2,3,4,5,,,,]; print (expected.length);

By the way, assertion at line 57 is incorrect, as it performs assignment, instead of comparison.

The issue with hasOwnProperty is related with #319, in particular, the problem is that we currently can't distinguish between "empty" values that are unset, and explicitly defined undefined values, currently both are undefined. If we do it by the standard, then empty values should be deleted, but currently that also deletes the set undefined value.

My mistake, let me re-check.

To distinguish between undefined properties and properties with undefined values, we can use ecma_op_object_get_property (...) != NULL.

Yes, initially. But we copy the values into a local array, so that we can sort them. When we do this both will become undefined, since we don't have a way to represent them differently.

I think, this is because values are copied to simple array of values without additional information.

It seems that the way also can lead to out-of-memory in case of sparse arrays. For example:

a = new Array (); a[100000] = 1; a[1000000] = 2; a.sort ();

Is it correct? If so, maybe we should use some another way of intermediate value storage during operation of sort.

dbatyai · 2015-07-09T09:17:24Z

@ruben-ayrapetyan , I've updated the PR. We no longer copy undefined properties.

ruben-ayrapetyan · 2015-07-09T21:30:43Z

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

  uint32_t copied_num = 0;

  /* Copy unsorted array into a native c array. */
  for (uint32_t index = 0; index < len && ecma_is_completion_value_empty (ret_value); index++)
  {
    ecma_string_t *index_string_p = ecma_new_ecma_string_from_uint32 (index);
-    ECMA_TRY_CATCH (index_value, ecma_op_object_get (obj_p, index_string_p), ret_value);
+    if (ecma_op_object_get_property (obj_p, index_string_p) != NULL)


The test would be executed too long, because of iteration over all values in [0.. len):

var a = new Array (); a[100000] = 1; a[1000000] = 2; a.sort ();

There are also another places with the issue. For example, ecma_op_array_object_define_own_property.
Could you, please, raise an issue about this, with references to known places with the issue, and add FIXME to the code part, referencing the issue?

ruben-ayrapetyan · 2015-07-09T21:34:56Z

After raising the issue and adding FIXMEs, the changes would look good to me.

ruben-ayrapetyan · 2015-07-10T11:17:19Z

@dbatyai, could you, please, rebase to master?

dbatyai · 2015-07-10T11:38:31Z

@ruben-ayrapetyan, rebased.

ruben-ayrapetyan · 2015-07-10T12:15:20Z

@dbatyai, thank you!

zherczeg · 2015-07-10T13:22:31Z

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

@@ -962,6 +962,9 @@ ecma_builtin_array_prototype_object_index_of (ecma_value_t this_arg, /**< this a
    {
      JERRY_ASSERT (from_idx < len);

+        /* FIXME: Iterating over the whole lenght is unnecessary in case of sparse arrays,
+         *        and could take a long time if len is large. We should only iterate
+         *        over defined array indexes. */


I think there is a TODO () or similar macro somewhere.

zherczeg · 2015-07-10T13:26:58Z

I agree that the real index based sorting should be replaced by a virtual index based sorting. What is the sorting algorithm? Quick-sort?

ruben-ayrapetyan · 2015-07-10T13:29:07Z

After fixing FIXME comments and moving them to separate commit, looks good to me.

JerryScript-DCO-1.0-Signed-off-by: Dániel Bátyai dbatyai.u-szeged@partner.samsung.com

dbatyai · 2015-07-13T09:21:12Z

@zherczeg, current algorithm is heapsort.

dbatyai · 2015-07-14T11:45:52Z

@ruben-ayrapetyan, please re-check
After talking with @zherczeg we decided that we should fix #362 in this patch, instead of adding FIXMEs.
After some investigation it appears that sort is actually the only candidate for #362, because other functions are required to also check the prototype chain, so iterating over their defined properties is not enough.

ruben-ayrapetyan · 2015-07-14T12:30:18Z

@dbatyai, I'll check the changes today.

I think that fix of #362 in several other parts of engine (including, ecma_op_array_object_define_own_property) can significantly improve performance of sparse arrays handling. However, maybe, sort can be optimized even more, by skipping prototype chain.

dbatyai · 2015-07-15T14:43:55Z

@ruben-ayrapetyan, @zherczeg, I've updated the PR.

ruben-ayrapetyan · 2015-07-15T14:53:52Z

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

+      ECMA_FINALIZE (del_value);
+    }
+
+    property_p = next_prop;


The code infitely loops for the following example:

Array.prototype.sort();

Reason is that property_p is not updated for internal properties.

It's better to put property_p = next_prop into for's increment section.

ruben-ayrapetyan · 2015-07-15T14:56:47Z

After fixing, the changes would look good to me.

JerryScript-DCO-1.0-Signed-off-by: Dániel Bátyai dbatyai.u-szeged@partner.samsung.com

ruben-ayrapetyan · 2015-07-16T15:54:00Z

Looks good to me

zherczeg · 2015-07-20T09:05:44Z

LGTM

egavrin · 2015-07-22T11:08:32Z

I'll merge.

egavrin · 2015-07-22T11:31:48Z

Merged a2c6663

dbatyai added bug Undesired behaviour ecma builtins Related to ECMA built-in routines labels Jul 8, 2015

dbatyai added this to the ECMA builtins milestone Jul 8, 2015

egavrin reviewed Jul 8, 2015
View reviewed changes

ruben-ayrapetyan reviewed Jul 8, 2015
View reviewed changes

dbatyai force-pushed the array_sort_in_place branch from 1d1fb9f to 0231028 Compare July 9, 2015 08:53

ruben-ayrapetyan self-assigned this Jul 9, 2015

ruben-ayrapetyan reviewed Jul 9, 2015
View reviewed changes

ruben-ayrapetyan assigned dbatyai and unassigned ruben-ayrapetyan Jul 9, 2015

dbatyai force-pushed the array_sort_in_place branch from 0231028 to 5fb673e Compare July 10, 2015 11:34

ruben-ayrapetyan assigned ruben-ayrapetyan and unassigned dbatyai Jul 10, 2015

zherczeg reviewed Jul 10, 2015
View reviewed changes

ruben-ayrapetyan assigned zherczeg and unassigned ruben-ayrapetyan Jul 10, 2015

Array.prototype.sort() should sort in place.

9f6bded

JerryScript-DCO-1.0-Signed-off-by: Dániel Bátyai dbatyai.u-szeged@partner.samsung.com

dbatyai force-pushed the array_sort_in_place branch from 5fb673e to 65b2361 Compare July 13, 2015 09:17

dbatyai force-pushed the array_sort_in_place branch from 65b2361 to 6331555 Compare July 14, 2015 11:28

dbatyai force-pushed the array_sort_in_place branch from 6331555 to 92cde9d Compare July 15, 2015 14:42

ruben-ayrapetyan reviewed Jul 15, 2015
View reviewed changes

Array.prototype.sort() now handles defined properties only.

eca1ec4

JerryScript-DCO-1.0-Signed-off-by: Dániel Bátyai dbatyai.u-szeged@partner.samsung.com

dbatyai force-pushed the array_sort_in_place branch from 92cde9d to eca1ec4 Compare July 15, 2015 15:05

dbatyai assigned egavrin and unassigned zherczeg Jul 21, 2015

egavrin closed this Jul 22, 2015

dbatyai deleted the array_sort_in_place branch July 31, 2015 10:57

zherczeg mentioned this pull request Jul 20, 2016

Nominating Dániel Bátyai (dbatyai) for JerryScript Maintainer status #1220

Closed

somang-park unassigned egavrin Nov 25, 2016

Array.prototype.sort() should sort in place. #335

Array.prototype.sort() should sort in place. #335

Uh oh!

Conversation

dbatyai commented Jul 8, 2015

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!

egavrin commented Jul 8, 2015

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!

dbatyai commented Jul 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ruben-ayrapetyan commented Jul 9, 2015

Uh oh!

ruben-ayrapetyan commented Jul 10, 2015

Uh oh!

dbatyai commented Jul 10, 2015

Uh oh!

ruben-ayrapetyan commented Jul 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg commented Jul 10, 2015

Uh oh!

ruben-ayrapetyan commented Jul 10, 2015

Uh oh!

dbatyai commented Jul 13, 2015

Uh oh!

dbatyai commented Jul 14, 2015

Uh oh!

ruben-ayrapetyan commented Jul 14, 2015

Uh oh!

dbatyai commented Jul 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ruben-ayrapetyan commented Jul 15, 2015

Uh oh!

ruben-ayrapetyan commented Jul 16, 2015

Uh oh!

zherczeg commented Jul 20, 2015

Uh oh!

egavrin commented Jul 22, 2015

Uh oh!

egavrin commented Jul 22, 2015

Uh oh!

Uh oh!