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

Within Vector, use Array.Copy wherever possible #3236

Merged
merged 22 commits into from
Mar 3, 2022

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Jan 26, 2022

Pull Request Description

Following the Slice and Array.Copy experiment, took just the Array.Copy parts out and built into the Vector class.

This gives big performance wins in common operations:

Test Ref New
New Vector 41.5 41.4
Append Single 26.6 4.2
Append Large 26.6 4.2
Sum 230.1 99.1
Drop First 20 and Sum 343.5 96.9
Drop Last 20 and Sum 311.7 96.9
Filter 240.2 92.5
Filter With Index 364.9 237.2
Partition 772.6 280.4
Partition With Index 912.3 427.9
Each 110.2 113.3

Benchmarks run on an AWS EC2 r5a.xlarge with 1,000,000 item count, 100 iteration size run 10 times.

Important Notes

Have generally tried to push the @Tail_Call down from the Vector class and move to calling functions on the range class.

  • Expanded benchmarks on Vector
  • Added take method to Vector
  • Added each_with_index method to Vector
  • Added filter_with_index method to Vector

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.

@jdunkerley jdunkerley marked this pull request as draft January 26, 2022 15:59
@jdunkerley jdunkerley changed the title Use Array.Copy wherever possible Within Vector, use Array.Copy wherever possible Jan 26, 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.

Apart from reconsidering the slice name, the rest looks good to me.

Hurray for the performance improvements! 🎉

Copy link
Member

@wdanilo wdanilo left a comment

Choose a reason for hiding this comment

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

Absolutely amazing results ❤️

@jdunkerley
Copy link
Member Author

jdunkerley commented Jan 28, 2022

Discovered an issue with polyglot arrays and Array.Copy.

from Standard.Base import all

main =
    vector = "ABCDEF".utf_8
    IO.println <| vector == [65,66,67,68,69,70]

    new_array = Array.new 6
    Array.copy vector.to_array 0 new_array 0 6
    new_vector = Vector.Vector new_array
    IO.println <| new_vector == [65,66,67,68,69,70]

Will print True for the first check and False for the second.

Raised a ticket to look at the way polyglot arrays are used in vector and solve this issue:
https://www.pivotaltracker.com/story/show/181122542

@wdanilo
Copy link
Member

wdanilo commented Feb 2, 2022

Discovered an issue with polyglot arrays and Array.Copy. Can't merge until solved.

In such a case, James, please always do the following steps:

  1. Change the PR to Draft.
  2. Add description in the comment what is the issue. Telling only Discovered an issue with polyglot arrays and Array.Copy does not allow threader to understand what it is about.
  3. Paste in the comment a link to the issue, so everyone can track whether it was resolved or not.

@jdunkerley jdunkerley marked this pull request as draft February 2, 2022 08:49
@jdunkerley jdunkerley mentioned this pull request Feb 2, 2022
4 tasks
@jdunkerley jdunkerley marked this pull request as ready for review March 2, 2022 16:29
@jdunkerley jdunkerley requested a review from radeusgd March 2, 2022 16:29
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.

Enso code looks good, I don't understand how the workaround fixes the described Array.copy issue though. But I'm told cases that didn't work, work now.

I think @kustosz should take a look at this before merge too.

@jdunkerley
Copy link
Member Author

Revised stats:

Test Ref New
New Vector 36.1 36.5
Append Single 19.4 4.5
Append Large 13.4 5.3
Sum 208.6 123.1
Drop First 20 and Sum 489.1 116.4
Drop Last 20 and Sum 474.7 115.8
Filter 228.9 103.6
Filter With Index 380.7 188.7
Partition 714.4 307.7
Partition With Index 870.2 423.8
Each 111.0 113.6

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.

LGTM, though I hope my proposed fix for Array.copy works and un-breaks all of this :)

@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Mar 3, 2022
@mergify mergify bot merged commit fb68f18 into develop Mar 3, 2022
@mergify mergify bot deleted the wip/jd/array-copy-181034165 branch March 3, 2022 15: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.

None yet

5 participants