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

[dbatyai]

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]

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

[dbatyai]

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

[ruben-ayrapetyan]

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]

I see

[egavrin]

No other questions. Good to me.

[ruben-ayrapetyan]

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

[ruben-ayrapetyan]

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.

[dbatyai]

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.

[ruben-ayrapetyan]

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.

[dbatyai]

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.

[dbatyai]

My mistake, let me re-check.

[ruben-ayrapetyan]

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

[dbatyai]

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.

[ruben-ayrapetyan]

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]

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

[ruben-ayrapetyan]

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]

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

[ruben-ayrapetyan]

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

[dbatyai]

@ruben-ayrapetyan, rebased.

[ruben-ayrapetyan]

@dbatyai, thank you!

[zherczeg]

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

[zherczeg]

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]

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

[dbatyai]

@zherczeg, current algorithm is heapsort.

[dbatyai]

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

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

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

[ruben-ayrapetyan]

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]

After fixing, the changes would look good to me.

[ruben-ayrapetyan]

Looks good to me

[zherczeg]

LGTM

[egavrin]

I'll merge.

[egavrin]

Merged a2c6663