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

Improve Vector #3232

Merged
merged 15 commits into from
Jan 25, 2022
Merged

Improve Vector #3232

merged 15 commits into from
Jan 25, 2022

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented Jan 21, 2022

  • Add error handling and negative indices to at
  • Improve each performance ~2-3x
  • Improve filter performance ~50-60x

Pull Request Description

Important Notes

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 documentation and configuration conforms to the markdown and YAML style guides.
  • All code has been tested where possible.

@radeusgd radeusgd self-assigned this Jan 21, 2022
@radeusgd
Copy link
Member Author

Added benchmarks for filter and each. Their results from before changes:

Filter/iteration:0: 766,51ms
Filter/iteration:1: 759,77ms
Filter/iteration:2: 758,00ms
Filter/iteration:3: 760,46ms
Filter/iteration:4: 768,10ms
Filter/iteration:5: 777,35ms
Filter/iteration:6: 783,51ms
Filter/iteration:7: 781,18ms
Filter/iteration:8: 783,44ms
Filter/iteration:9: 769,84ms
Each/iteration:0: 110,84ms
Each/iteration:1: 93,60ms
Each/iteration:2: 94,65ms
Each/iteration:3: 91,88ms
Each/iteration:4: 101,19ms
Each/iteration:5: 98,19ms
Each/iteration:6: 96,54ms
Each/iteration:7: 93,94ms
Each/iteration:8: 96,97ms
Each/iteration:9: 96,00ms

@radeusgd
Copy link
Member Author

The same benchmarks AFTER the changes:

Filter/iteration:0: 157,70ms
Filter/iteration:1: 33,16ms
Filter/iteration:2: 14,49ms
Filter/iteration:3: 12,44ms
Filter/iteration:4: 13,07ms
Filter/iteration:5: 13,04ms
Filter/iteration:6: 11,58ms
Filter/iteration:7: 10,90ms
Filter/iteration:8: 10,01ms
Filter/iteration:9: 2,18ms
Each/iteration:0: 28,42ms
Each/iteration:1: 10,42ms
Each/iteration:2: 10,72ms
Each/iteration:3: 10,46ms
Each/iteration:4: 10,50ms
Each/iteration:5: 11,12ms
Each/iteration:6: 10,23ms
Each/iteration:7: 10,18ms
Each/iteration:8: 10,23ms
Each/iteration:9: 10,16ms

We can see roughly 9x improvement for each and a sure 70x improvement for filter (last step suggests it may JIT to be even faster) - filter just got improved from being N^2 to N, so depending on the input size the difference will also differ, quite significantly.

@@ -303,8 +324,15 @@ type Vector
[1, 2, 3, 4, 5].filter (> 3)
filter : (Any -> Boolean) -> Vector Any
filter predicate =
check acc ix = if predicate ix then acc + [ix] else acc
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

My exact face expression after seeing it.

@radeusgd
Copy link
Member Author

The benchmarks were deliberately smaller so that the old super-slow version of filter would finish in a sane amount of time.
Since we now have a fast implementation, in the last commit I made the benchmarks run on slightly bigger numbers.

Below are the results after this change.
Obviously they will be slower as they are running on bigger data - they cannot be compared with the results from above.
They are listed mostly as a reference for future measurements (but this is only a general reference, to measure this well we obviously need to ensure both old and new version run on the same machine etc.)

Filter/iteration:0: 181,82ms
Filter/iteration:1: 52,25ms
Filter/iteration:2: 49,06ms
Filter/iteration:3: 48,53ms
Filter/iteration:4: 52,87ms
Filter/iteration:5: 50,44ms
Filter/iteration:6: 48,64ms
Filter/iteration:7: 46,07ms
Filter/iteration:8: 48,40ms
Filter/iteration:9: 46,88ms
Each/iteration:0: 25,31ms
Each/iteration:1: 12,99ms
Each/iteration:2: 12,64ms
Each/iteration:3: 12,77ms
Each/iteration:4: 12,63ms
Each/iteration:5: 12,48ms
Each/iteration:6: 12,77ms
Each/iteration:7: 12,60ms
Each/iteration:8: 12,71ms
Each/iteration:9: 12,64ms

@radeusgd radeusgd marked this pull request as ready for review January 21, 2022 17:17
@wdanilo
Copy link
Member

wdanilo commented Jan 21, 2022

Regarding the times, this is still super slow, but I understand that making it performant is not part of this PR and that there would be separate discussion with @ekmett and @kustosz about it, so its a green light from me :)

@radeusgd
Copy link
Member Author

radeusgd commented Jan 21, 2022

Regarding the times, this is still super slow, but I understand that making it performant is not part of this PR and that there would be separate discussion with @ekmett and @kustosz about it, so its a green light from me :)

Why is this super-slow? In comparison to what?

Asking because it may indeed be worth comparing, for example with pure-Java code - and there will probably be a noticeable difference.

But fair, maybe filtering 1M elements should indeed take less than 40ms.

Copy link
Member

@jdunkerley jdunkerley left a comment

Choose a reason for hiding this comment

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

Some decent improvements in here - that help should help with the high level functions.

@radeusgd
Copy link
Member Author

Just out of curiosity - and to get a general feeling of what should I expect of these functions, I wrote a very small benchmark in Java to have a baseline comparison (of course it is not a fully-fledged comparison as I didn't want to waste time on it, just get a general feeling of what would be the expected performance).

So using the same parameters, I also changed the callback in each to compute (sum + x) % 1000 so that we avoid entering BigIntegers (as then Java will just go with fast modulo and Enso will go the slower but precise BigInt computation).

In Java unboxed iteration takes about ~3ms, boxed iteration is about 5ms (boxed is a better baseline because our Vector is boxed to allow mixed types, theoretically we could also have an unboxed Vector for integers, but it could have some issues with overflows as normally we'd want to switch to BigInts). In Enso each takes about 14ms. So yeah it is indeed slower, but I wouldn't say super-slow.

Filter is a bit worse, since our implementation takes ~50ms and simple Java code seems to run in ~10ms.

Java code for reference:
java-bench.zip

Copy link
Contributor

@kustosz kustosz left a comment

Choose a reason for hiding this comment

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

There's a useful pattern emerging, you should formalize it.

@radeusgd radeusgd merged commit cfdb33b into develop Jan 25, 2022
@radeusgd radeusgd deleted the wip/radeusgd/improve-vector branch January 25, 2022 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants