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

A very fast sortUsingDescriptors: #1560

Merged

Conversation

mrcarlberg
Copy link
Member

I needed to sort a lot of objects and was not impressed by the speed.
Sharing my improvements...

I needed to sort a lot of objects and was not impressed by the speed.
Sharing my improvements...
@primalmotion
Copy link
Member

Do you have any performance improvement ratio?

@cappbot
Copy link

cappbot commented Jun 7, 2012

Milestone: Someday. Label: #new. What's next? A reviewer should examine this issue.

@mrcarlberg
Copy link
Member Author

It's around factor 4 when running the performance test cases from command line or Safari. Running a table view with 21000+ objects and sorting by clicking on column header it is a factor 2. But I guess that the table view might do other stuff then sorting.....

@aparajita
Copy link
Contributor

We really need a unit test for this.

@mrcarlberg
Copy link
Member Author

Please let me know what you need for type of unit tests? There are already some in CPArrayPerformanceTest.j and CPArrayControllerPerformance.j

@aparajita
Copy link
Contributor

Then it would be nice if you could make an app that shows the performance figures for old and new code.

@mrcarlberg
Copy link
Member Author

Ok, I created an ugly test Application :)

Source Code (.tgz)

@cacaodev
Copy link
Contributor

cacaodev commented Jun 8, 2012

It's definitely faster ! While testing with the current ojunit test, i noticed the native sort testing was wrong because it was not using sort descriptors. I fixed this and added tests for random numeric and text arrays.

#1562

@primalmotion
Copy link
Member

Awesome work! Any objection before merging it in?

@aparajita
Copy link
Contributor

It is definitely way faster than the current Cappuccino sort, but overall not faster than native sort. Is there a reason native sorting can't be used?

@mrcarlberg
Copy link
Member Author

I think native javascript sorting is not stable. It will not preserve order if elements are the same. I guess Cocoa has stable sorting. This can be a reason not to use native sorting in Cappuccino.

@mrcarlberg
Copy link
Member Author

I found a bug. I'm not using the selector in the sort descriptor. Using compare: all the time....
I'll make some modifications and add a test case for that.....

@aparajita
Copy link
Contributor

Please be sure you are testing all possible permutations of CPSortDescriptor.

var i = [descriptors count],
jsDescriptors = [];
// Revert the order of the descriptors
while (i--)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and everywhere else, please separate all control structures (and their bracketed code blocks) from surrounding code with a blank line.

@cacaodev
Copy link
Contributor

cacaodev commented Jun 8, 2012

@aparajita
In the current test, native sort is using js code instead of capp code for sort descriptors and it appears faster than it is. see my modified CPArrayPerformanceTest above.

@aljungberg
Copy link
Member

I'm going to merge #1562 first, then we can look into this in more detail.

milestone=0.9.6
+feature
+Foundation
+#acknowledged

@cappbot
Copy link

cappbot commented Jun 9, 2012

Milestone: 0.9.6. Labels: #acknowledged, Foundation, feature. What's next? A reviewer should examine this issue.

@aljungberg
Copy link
Member

So in my testing this is much faster.

Interestingly sort using selector also became faster. I did some testing and this seems to be caused by the minor change of declaring sortArrayUsingFunction as an independent function instead of declaring it in the scope of sortUsingFunction:. I'm guessing that the independent function might be avoiding the with() penalty.

In all cases except "TEXT RANDOM" with selector, the non-native version is faster than the native version in ojtest. (This is likely to stop being the case when we don't use with() anymore. Native should be faster, at least if we also cache the valueForKey: responses in the native version like this hand coded algorithm does.)

aljungberg added a commit that referenced this pull request Jun 10, 2012
It looks like issue #1560 will cause a different sorting function to be used for selectors vs descriptors so performance of both should be monitored for regressions.
aljungberg added a commit that referenced this pull request Jun 10, 2012
---

I needed to sort a lot of objects and was not impressed by the speed.
Sharing my improvements...
aljungberg added a commit that referenced this pull request Jun 10, 2012
@aljungberg aljungberg merged commit 0696b22 into cappuccino:master Jun 10, 2012
@aljungberg
Copy link
Member

It also seems like the Safari based application is showing pretty random results. If I click on Run I can get some results randomly 10X larger than other results. ojtest based testing seems to give much more reliable results. Maybe this is because Safari is interrupting the non-native sort more often to do other things when you have many tabs open, and those interruptions are random in frequency and length.

@aljungberg
Copy link
Member

Great work, thanks!

+#fixed

@cappbot
Copy link

cappbot commented Jun 10, 2012

Milestone: 0.9.6. Labels: #fixed, Foundation, feature. What's next? This issue is considered successfully resolved.

@aljungberg
Copy link
Member

For future reference, here are my timing results using ojtest on a 2011 MacBook Pro. It's an average of 3 runs before and after, testing both sort using descriptors and sort using selectors while comparing them against native sort using the code in #1562.

Statistics

@@ -425,3 +440,114 @@ var compareObjectsUsingDescriptors= function compareObjectsUsingDescriptors(lhs,

return result;
};

// Observe that the sort descriptors has the reversed order by the caller
var sortArrayUsingJSDescriptors = function(a, d)
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this has been merged already, but we might want to make this function more readable. All the one letter variable names are very hard to translate to a meaning and this function will be very hard to maintain for everybody that hasn't written it and even for the original author 3 months from now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree completely. Maybe the original author would be kind enough to make it more readable.

Copy link
Contributor

Choose a reason for hiding this comment

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

i'm the one who submitted the merge-sort function and it's a port from C. I can't find the original source right now but i'll keep searching.

Copy link
Contributor

Choose a reason for hiding this comment

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

it's a port from C

That was obvious. ;-)

@mrcarlberg mrcarlberg deleted the very_fast_sort_using_descriptors branch February 5, 2016 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants