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

Reimplement Column.truncate, .ceil, and .floor as vectorized Java ops #6941

Merged
merged 50 commits into from
Jun 6, 2023

Conversation

GregoryTravis
Copy link
Contributor

@GregoryTravis GregoryTravis commented Jun 2, 2023

Pull Request Description

Reimplement these in Java.

Benchmarks:

Before:

Column.truncate floats average: 124.4ms
Column.ceil floats average: 121.47ms
Column.floor floats average: 120.18ms
Column.truncate ints average: 124.78ms
Column.ceil ints average: 120.41ms
Column.floor ints average: 102.35ms

After (boxed):

Column.truncate floats average: 3.75ms
Column.ceil floats average: 2.25ms
Column.floor floats average: 1.89ms
Column.truncate ints average: 2ms
Column.ceil ints average: 1.77ms
Column.floor ints average: 1.74ms

After (unboxed):
Column.truncate floats average: 3.32ms
Column.ceil floats average: 2.15ms
Column.floor floats average: 1.69ms
Column.truncate ints average: 1.74ms
Column.ceil ints average: 1.61ms
Column.floor ints average: 1.99ms

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.

@GregoryTravis GregoryTravis marked this pull request as ready for review June 2, 2023 18:56
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.

The changes look good to me, the overall 'framework' of the change is good.

However, I think we should avoid boxing and here it is certainly possible. Please create 2 classes UnaryLongToLongOp and UnaryDoubleToLongOp. You can essentially copy-paste the UnaryIntegerOp and just replace the generic T with primitive double/long. This is a pain in Java that generics don't play with primitives, but it's sometthing we cannot do about and while I hate such copy pasting, it's the only way in Java (apart from writing custom code generators - we may come there but at this moment it would be an overkill IMO) to ensure no boxing occurs.

I appreciate we are finally getting benchmarks for operations like this. Good to see the performance difference - it seems it's quite significant. This may indicate that we may need to rethink #6292 and how I implemented #6860 - since for custom types Enso is more efficient than Java-to-Enso callbacks, but apparently with primitives we are still far behind (but of course we just need separate benchmarks for the MultiValueKey, which I will create when working on #6292 - but it's good to see some first comparative results to get an idea what is roughly the difference between operations in pure Enso vs Java).

@JaroslavTulach JaroslavTulach added -compiler -libs Libraries: New libraries to be implemented labels Jun 5, 2023
@radeusgd
Copy link
Member

radeusgd commented Jun 5, 2023

Looks all good 🎉

@GregoryTravis GregoryTravis added the CI: Ready to merge This PR is eligible for automatic merge label Jun 5, 2023
@mergify mergify bot merged commit 912fbce into develop Jun 6, 2023
@mergify mergify bot deleted the wip/gmt/6861-ops branch June 6, 2023 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-compiler -libs Libraries: New libraries to be implemented CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reimplement Column.truncate, .ceil, and .floor as vectorized Java ops
4 participants