-
Notifications
You must be signed in to change notification settings - Fork 77
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
feat: array sort(quick sort), swap, reverse #72
Conversation
- fallback to heapsort - skip same elements - replace heap allocated List with recursive calls + loop
pub fn sort[T : Compare](self : Array[T]) -> Unit { | ||
quick_sort( | ||
{ array: self, start: 0, end: self.length() }, | ||
T::compare, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里有个问题就是 MoonBit 不会像 Rust 那样做 higher-order function 的 specialization,所以这里传函数指针的话会有一些间接调用造成的运行时开销
/cc @bobzhang
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这种额外开销很高吗?还有什么写法能够比较通用的?如果几种不同的 sort 方式如果都要硬编码 inline 的话有点太丑了
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overhead comes from passing the function T::compare
, which leads to indirect calls at the call site. So I'd recommend you just stick with generic functions, ie. using [T: Compare]
. The moonbit compiler implements monomorphization so the runtime cost of generic functions is low.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it means we cannot reuse the same code for sort_by
and sort_by_key
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do a benchmark to see the performance impact
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing the bench. Right now you have to manually copy the code if you want both the efficient sort
and the versatile sort_by
. We will try to work out a solution for both code reusability and high performance. Let us know if you have any suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for Moonbit to have specializations of high-order functions in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to discuss about it in our team, and we will let you know soon. /cc @bobzhang
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will perform more advanced optimizations in the future, however, it is recommended to write obviously fast code for perf critical code instead of relying on black box advanced optimizations, you may encounter perf cliff from time to time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it
This PR adds the following methods to Array:
pub fn sort[T : Compare](self : Array[T])
pub fn sort_by_key[T, K : Compare](self : Array[T], map : (T) -> K)
pub fn sort_by[T](self : Array[T], cmp : (T, T) -> Int)
pub fn reverse[T](self : Array[T])
pub fn swap[T](self : Array[T], i : Int, j : Int)
The implementation uses a heuristic algorithm to choose the pivot value to avoid the worst case of quick sort, as Rust std does. It will also try to use bubble sort instead when the array is likely sorted or when it is small. The time complexity is O(n) if the array is sorted ascendingly or descendingly.