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

sort handles incomparable values #5998

Merged
merged 56 commits into from
Apr 16, 2023

Conversation

Akirathan
Copy link
Member

@Akirathan Akirathan commented Mar 17, 2023

Fixes #5742

Pull Request Description

Vector.sort handles incomparable values. Now, it should sort basically any vector, potentially attaching a warning if sort encountered incomparable values.

Important Notes

  • Slight change of Vector.sort method signature
    • To be able to introduce Vector.sort_builtin optimized builtin for the most common case - sorting only primitive values with Default_Comparator.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@Akirathan Akirathan linked an issue Mar 17, 2023 that may be closed by this pull request
@Akirathan Akirathan force-pushed the wip/akirathan/sort-incomparable-5742 branch from 31398a0 to c6e673a Compare March 17, 2023 17:51
@Akirathan Akirathan force-pushed the wip/akirathan/sort-incomparable-5742 branch from 3fa7403 to 23c0dbf Compare March 22, 2023 18:13
@Akirathan Akirathan force-pushed the wip/akirathan/sort-incomparable-5742 branch from 23c0dbf to b5692b8 Compare March 22, 2023 18:18
@Akirathan Akirathan self-assigned this Mar 23, 2023
# Conflicts:
#	distribution/lib/Standard/Base/0.0.0-dev/src/Any.enso
#	distribution/lib/Standard/Base/0.0.0-dev/src/Data/Ordering.enso
#	test/Tests/src/Data/Numbers_Spec.enso
#	test/Tests/src/Data/Ordering_Spec.enso
@Akirathan Akirathan force-pushed the wip/akirathan/sort-incomparable-5742 branch from 432a87b to 219aa4a Compare March 28, 2023 15:38
@Akirathan Akirathan force-pushed the wip/akirathan/sort-incomparable-5742 branch from cb93987 to 4fbdc0d Compare April 3, 2023 13:47
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

I'll appreciate applying the two suggestions. Other than that - it all looks very good! I'm glad vector sort will be improved now. And I really appreciate very good unit tests.

@Akirathan Akirathan force-pushed the wip/akirathan/sort-incomparable-5742 branch 2 times, most recently from a2c6cd0 to 30109e1 Compare April 14, 2023 15:18
@Akirathan Akirathan force-pushed the wip/akirathan/sort-incomparable-5742 branch from 30109e1 to 8268e02 Compare April 14, 2023 16:03
@Akirathan
Copy link
Member Author

Akirathan commented Apr 14, 2023

Scheduled benchmarks in https://github.com/enso-org/enso/actions/runs/4702222179.

I have checked test/Benchmarks/src/Table/Sorting.enso benchmark locally, and all the benchmarks have better performance, except for "Table.order_by object", that regressed by 25%. That particular benchmark creates a lot of My atoms with a custom My_Comparator and are move into a table and order_by is called, which calls into Java and then back into Enso (in ObjectComparator). There is no simple solution how to make that faster in general, at least not now. I am not sure why there is such regression, though, and I am not going to investigate anymore now.

GitHub
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

  • Use alter in the Vector_Spec.enso
  • Don't forget to convert interop values with HostToEnsoValueNode

@@ -113,6 +113,8 @@ private Value createEnsoMethod(String source, String methodName) {
@Test
public void recursiveFactorialCall() {
final Value facFn = createEnsoMethod("""
from Standard.Base.Data.Ordering import all

Copy link
Member

Choose a reason for hiding this comment

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

Without this import we cannot use <= anymore.

test/Tests/src/Data/Vector_Spec.enso Outdated Show resolved Hide resolved
@Akirathan
Copy link
Member Author

Akirathan commented Apr 15, 2023

Benchmark results

Baseline from develop:

I had to revert 0df66b1 ("Remove the remaining comparison operator overrides for numbers"), as that caused a huge performance regressions on benchmarks dealing with vectors with primitive values. This commit was not essential for this PR, and we can deal with it later.

After the revert, I tried running and comparing some benchmarks locally. Comparison of my recent version (254d579) with the baseline is:

  • test/Benchmarks/src/Table/Sorting.enso:
    • Table.order_by ints: -5% (means -5% milliseconds, so it is better 5% better than before)
    • Vector.sort ints: -5%
    • Table.order_by_dates: -7%
    • Vector.sort dates -36%
    • Table.order_by objects +1%
    • Vector.sort objects -64%
  • Benchmarks from org.enso.interpreter.bench.benchmarks.semantic.ArrayProxyBenchmarks stayed roughly the same, with one exception of:
    • sumOverVector that got +40%. I have no idea why, the changes in this PR should not affect that benchmark. Tried to look into the Truffle compilation logs and did not discovered anything interesting. I will let this one go now.

Conclusion

Overall, the benchmark results seem pretty good. Sorting of vector with primitive values is roughly the same, but we can see huge improvements for sorting of vector with some atoms (-64%) and dates (-36%). Will merge this PR ASAP.

GitHub
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

- Output table is sorted by benchmark labels.
- Do not fail when there are different benchmark labels in both runs.
@Akirathan Akirathan force-pushed the wip/akirathan/sort-incomparable-5742 branch from eca0069 to 1f23d83 Compare April 16, 2023 10:17
@Akirathan Akirathan added the CI: Ready to merge This PR is eligible for automatic merge label Apr 16, 2023
@Akirathan Akirathan merged commit b42e910 into develop Apr 16, 2023
@Akirathan Akirathan deleted the wip/akirathan/sort-incomparable-5742 branch April 16, 2023 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sort handles Incomparable types by default
4 participants