Array sort update #6849
Array sort update #6849
Conversation
| @@ -134,6 +134,35 @@ const tests = [ | |||
| } | |||
| }, | |||
| { | |||
| name : "Array.prototype.sort with edited prototypes and compare function side effects", | |||
There was a problem hiding this comment.
This test had to be revised as the behaviour it was testing was wrong after the spec change.
ALSO had to move it up above other tests that edit the prototype.
rhuanjl
force-pushed
the
array_sort_update
branch
from
fbe13a2
to
ca8dc9b
Compare
- cloning the array and using Has property on every element - sorting the clone - writing back over the top of the original
rhuanjl
force-pushed
the
array_sort_update
branch
from
ca8dc9b
to
e3d3a36
Compare
|
@ppenzin I may scrap this and do a native C++ version - unsure - and interested if you have any thoughts first. |
There was a problem hiding this comment.
Do you think this variant of sort is slower in JS than it can be in C++? Updated sort spec should come with at least some performance and memory penalty over the old variant because of need for temp and copy back. Functionally I think this solution is OK, however C++ implementation may be worth to trying, though I am not sure if that would bring big perf gains.
In theory the change brings a performance penalty due to the requirement to copy etc BUT I think if I took the whole thing into C++ I could actually optimise quite a bit. As the spec says to copy into a list, I could use a simple row of Vars rather than a JS data structure - read/write into that could be a lot more efficient than the current mechanism. The potential downside would be the loss of the ability to inline the compare functions BUT considering all the read/write gains I think it would be minor. Decided to merge this anyway for now - it works and the penalty isn't a disaster, though will think about following up with the C++ version. |
|
Maybe we should file an issue to change this later to C++. Edit: now I see #6852, was going through notifications one by one. |
The Array.prototype.sort specification was revised in 2021, this PR attempts to update CC's implementation for these changes.
Details of the changes can be reviewed here: tc39/ecma262#1585
I've also attempted to make a short cut that skips several checks if certain conditions are met - I'm unsure how worthwhile this is - removing it would simplify the logic and I'm not sure it would lose that much.
EDIT: the short cut/fast path didn't work - pushing updated PR without it, also wondering if with the new algorithm it may be more efficient to revert to a C++ implementation rather than this self-hosted JS - the reasons for the self-hosted JS were a) the possibility of compare functions being inlined AND b) ease of implementation. BUT with the new algorithm I think C++ would probably perform better.
Fix: #6843