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

Move logic calculating the index in Vector.at to a builtin method to make the performance of Vector to be on par with Array #3811

Merged
merged 8 commits into from
Oct 20, 2022

Conversation

hubertp
Copy link
Contributor

@hubertp hubertp commented Oct 19, 2022

Pull Request Description

The main culprit of a Vector slowdown (when compared to Array) was the normalization of the index when accessing the elements. Turns out that the Graal was very persistent on not inlining that particular fragment and that was degrading the results in benchmarks.

Being unable to force it to do it (looks like a combination of thunk execution and another layer of indirection) we resorted to just moving the normalization to the builtin method. That makes Array and Vector perform roughly the same.

Moved all handling of invalid index into the builtin as well, simplifying the Enso implementation. This also meant that Vector.unsafe_at is now obsolete.
Additionally, added support for negative indices in Array, to behave in the same way as for Vector.

Important Notes

Note that this workaround only addresses this particular perf issue. I'm pretty sure we will have more of such scenarios.
Before the change averageOverVector benchmark averaged around 0.033 ms/op now it does consistently 0.016 ms/op, similarly to averageOverArray.

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.

@hubertp hubertp force-pushed the wip/hubert/vector-perf-improvements-183249415 branch from e147277 to c974eb4 Compare October 19, 2022 10:00
@JaroslavTulach
Copy link
Member

I'd suggest to remove Vector.unsafe_at, now when Vector.at is fast enough.

@JaroslavTulach
Copy link
Member

Please make sure Array.at behaves the same as Vector.at - e.g. it can also handle negative indexes.

@hubertp hubertp force-pushed the wip/hubert/vector-perf-improvements-183249415 branch from 8d80034 to 2f4c3d9 Compare October 19, 2022 15:37
@hubertp hubertp changed the title Normalize index in Vector.at in builtin method Move logic calculating the index in Vector.at to a builtin method to make the performance of Vector to be on par with Array Oct 19, 2022
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 like that Arrays now fail with dataflow errors as vectors - the consistency is cool.

Also really appreciating how easy and clean it is now to add a builtin type!

@hubertp hubertp force-pushed the wip/hubert/vector-perf-improvements-183249415 branch from 423272c to 5c0a7a9 Compare October 19, 2022 16:32
@hubertp
Copy link
Contributor Author

hubertp commented Oct 19, 2022

I also added some documentation to explain how I figured that (and others) out.

The main culprit of a Vector slowdown (when compared to Array) was the
normalization of the index when accessing the elements.
Turns out that the Graal was very persistent on **not** inlining that
particular fragment and that was degrading the results in benchmarks.

Being unable to force it to do it (looks like a combination of thunk
execution and another layer of indirection) we resorted to just moving
the normalization to the builtin method. That makes Array and Vector
perform roughly the same.
Vector.at and Array.at should both support negative indices and both
return a dataflow error on invalid index.
@hubertp hubertp force-pushed the wip/hubert/vector-perf-improvements-183249415 branch from 5c0a7a9 to baee63c Compare October 20, 2022 08:28
@@ -94,6 +94,29 @@ order: 7
`truffle-api`, you've done something wrong. Similarly, if you ever import
anything from `org.graalvm.polyglot` in the language code, you're doing
something wrong.
10. Avoid
[deoptimizations](https://www.graalvm.org/22.2/graalvm-as-a-platform/language-implementation-framework/Optimizing/#debugging-deoptimizations).
Understanding IGV graphs can be a very time-consuming and complex process.
Copy link
Member

Choose a reason for hiding this comment

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

;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To put it mildly.

@hubertp hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Oct 20, 2022
@mergify mergify bot merged commit 6c440be into develop Oct 20, 2022
@mergify mergify bot deleted the wip/hubert/vector-perf-improvements-183249415 branch October 20, 2022 12:50
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.

5 participants