Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Array sort update #6849

Merged
merged 2 commits into from
Oct 7, 2022
Merged

Array sort update #6849

merged 2 commits into from
Oct 7, 2022

Conversation

rhuanjl
Copy link
Collaborator

@rhuanjl rhuanjl commented Oct 5, 2022

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

Copy link
Collaborator Author

@rhuanjl rhuanjl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having serious second thoughts about my attempt at a fast path - not sure if it gains enough to be worth it.

EDIT: updated and removed it

@@ -134,6 +134,35 @@ const tests = [
}
},
{
name : "Array.prototype.sort with edited prototypes and compare function side effects",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 rhuanjl force-pushed the array_sort_update branch from fbe13a2 to ca8dc9b Compare October 6, 2022 07:37
- cloning the array and using Has property on every element
- sorting the clone
- writing back over the top of the original
@rhuanjl rhuanjl force-pushed the array_sort_update branch from ca8dc9b to e3d3a36 Compare October 6, 2022 07:39
@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Oct 6, 2022

@ppenzin I may scrap this and do a native C++ version - unsure - and interested if you have any thoughts first.

Copy link
Member

@ppenzin ppenzin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rhuanjl
Copy link
Collaborator Author

rhuanjl commented Oct 7, 2022

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.

@rhuanjl rhuanjl merged commit a880942 into chakra-core:master Oct 7, 2022
@rhuanjl rhuanjl deleted the array_sort_update branch October 7, 2022 07:08
@ppenzin
Copy link
Member

ppenzin commented Oct 13, 2022

Maybe we should file an issue to change this later to C++. Edit: now I see #6852, was going through notifications one by one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array.prototype.sort spec compliance
2 participants