Implement GATs #231
Implement GATs #231
Conversation
I think what we do now is clearer.
|
|
||
| fn path_elements(&self, _tolerance: f64) -> Self::PathElementsIter { | ||
| self.0.clone().into_iter() |
There was a problem hiding this comment.
This is the main benefit from this PR - now we don't have to clone the whole object.
| fn path_elements(&self, _tolerance: f64) -> Self::PathElementsIter { | ||
| self.0.clone().into_iter() | ||
| fn path_elements(&self, _tolerance: f64) -> Self::PathElementsIter<'_> { | ||
| self.0.iter().copied() |
There was a problem hiding this comment.
so the question I have with all of these is basically: do we even bother with copy? We we need to?
There was a problem hiding this comment.
Do you mean "why do we copy the elements rather than returning a ref to them?" If so then the answer is because some impls of path_elements will create the path elements on-demand, and so need to pass ownership of them to the callee.
If I've misunderstood let me know.
There was a problem hiding this comment.
Right, that answers my question. LGTM!
There was a problem hiding this comment.
I think it's an important question. From a rough calculation, the size of PathEl is around 50 bytes - not insignificant!
Calc: Point = 2 x f64 = 16 bytes, PathEl = 3 x Point + u8 (discriminant) = 49 ~ 50.
Having said that if your cache-line is 64 bytes you're still in budget, even with an 8 byte discriminant.
There was a problem hiding this comment.
yea, if we could return them by reference it would be nice, but it is understandable if we can't. Unless GATs let us do a thing where the current element is owned by the iterator, and lent out? Is that now expressible? 🤔
There was a problem hiding this comment.
Measurement is important but I strongly suspect that the optimizer will eliminate these copies.
|
They can do that. I think I might experiment when I have time and get some
numbers.
…On Tue, 8 Nov 2022, 14:41 Colin Rofls, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/bezpath.rs
<#231 (comment)>:
>
- fn path_elements(&self, _tolerance: f64) -> Self::PathElementsIter {
- self.0.clone().into_iter()
+ fn path_elements(&self, _tolerance: f64) -> Self::PathElementsIter<'_> {
+ self.0.iter().copied()
yea, if we *could* return them by reference it would be nice, but it is
understandable if we can't. *Unless* GATs let us do a thing where the
current element is owned by the iterator, and lent out? Is that now
expressible? 🤔
—
Reply to this email directly, view it on GitHub
<#231 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKT4XSJTS6CKKB3JM22VI3WHJRAPANCNFSM6AAAAAARXD5HTM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
I've investigated doing benchmarks with criterion.rs, and I think it's really great! But it's quite involved setting it up, so I will merge this PR for now. |
The main gain from this is being able to iterate over
BezPaths without cloning. Makes MSRV 1.65