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

Performance improvements for Comparators #5687

Merged
merged 14 commits into from
Feb 21, 2023

Conversation

Akirathan
Copy link
Member

@Akirathan Akirathan commented Feb 17, 2023

Pull Request Description

Critical performance improvements after #4067

Important Notes

  • Replace if-then-else expressions in Any.== with case expressions.
  • Fix caching in EqualsNode.
    • This includes fixing specializations, along with fallback guard.

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@Akirathan Akirathan linked an issue Feb 17, 2023 that may be closed by this pull request
@Akirathan Akirathan force-pushed the wip/akirathan/perf-regress-comparator-5636 branch from 30a1ad8 to 48498ac Compare February 20, 2023 12:00
@Akirathan Akirathan marked this pull request as ready for review February 20, 2023 12:02
@Akirathan Akirathan self-assigned this Feb 20, 2023
@Akirathan Akirathan added the CI: No changelog needed Do not require a changelog entry for this PR. label Feb 20, 2023
@Akirathan
Copy link
Member Author

Akirathan commented Feb 20, 2023

State of benchmarks from test/Benchmarks/src/Sum.enso:

engine version SumTCO Corecursive SumTCO Decimal SumTCO SumTCO Java State Co-State
engine-2023.1.27 55.05 100.80 77.72 57.24 77.55 231.13
engine-2023.2.17 5000 ?? ?? ?? ?? ??
engine-develop 487 99 147 138 141.80 519.18

engine-2023.1.27 is version before #4067, engine-2023.2.17 is version after #4067, I was not patient enough to let all the benchmarks finish.

There are still some regressions, but they will be tackled in more follow-up PRs.

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.

Let's get this important improvement in.

@Akirathan
Copy link
Member Author

Follow-up: #5709

@Akirathan Akirathan added the CI: Ready to merge This PR is eligible for automatic merge label Feb 20, 2023
@Akirathan Akirathan added p-highest Should be completed ASAP --low-performance labels Feb 20, 2023
@mergify mergify bot merged commit 58c7ca5 into develop Feb 21, 2023
@mergify mergify bot deleted the wip/akirathan/perf-regress-comparator-5636 branch February 21, 2023 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
--low-performance CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge p-highest Should be completed ASAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Severe performance regression of the Engine after Comparator API
4 participants