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.

Sign up for GitHub

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

Jump to bottom

Remove unnecessary borrows in function params for Copy types. #265

Merged
merged 1 commit into from
Jun 16, 2018
Merged

Remove unnecessary borrows in function params for Copy types. #265

merged 1 commit into from
Jun 16, 2018

Conversation

frewsxcv
Copy link
Member

@frewsxcv frewsxcv commented Jun 9, 2018 •
edited

Breaking changes for geo and geo-types.

@frewsxcv frewsxcv changed the title Remove references in function params for Copy types. Remove unnecessary references in function params for Copy types. Jun 9, 2018
@frewsxcv frewsxcv changed the title Remove unnecessary references in function params for Copy types. Remove unnecessary borrows in function params for Copy types. Jun 9, 2018
@frewsxcv
Copy link
Member Author

frewsxcv commented Jun 9, 2018

I need to benchmark this...

@frewsxcv
Copy link
Member Author

Okay, just benchmarked this with our current set of benchmarks. With these changes, convex hull f32 and convex hull f64 speed up ~17% and Polygon Euclidean distance RTree f64 speeds up ~4%. Everything else that we have benchmarks for there's not much of a performance change. I'm gonna approve...

bors r+

bors bot added a commit that referenced this pull request Jun 16, 2018
@bors @frewsxcv
Merge #265
8e26788 
265: Remove unnecessary borrows in function params for `Copy` types. r=frewsxcv a=frewsxcv

Breaking changes for `geo` and `geo-types`.

Co-authored-by: Corey Farwell <coreyf@rwell.org>
@urschrei
Copy link
Member

How are we getting 17% improvement for this?! (I mean, that's amazing, but I thought it was largely a semantic change, why is there such a perf difference?)

@bors
Copy link
Contributor

bors bot commented Jun 16, 2018

Build succeeded

@bors bors bot merged commit b7d8063 into master Jun 16, 2018
@frewsxcv frewsxcv deleted the frewsxcv-references branch June 16, 2018 14:40
@frewsxcv
Copy link
Member Author

How are we getting 17% improvement for this?! (I mean, that's amazing, but I thought it was largely a semantic change, why is there such a perf difference?)

Not sure! It looks like convex hull is a recursive algorithm and maybe by in-lining the points/coordinates instead of references, it's able to do something like tail call optimization? 🤷‍♂️ If I had some more time, it could be interesting to dig into the compiled LLVM to see what's going on behind the scenes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@frewsxcv @urschrei