Avoid index clone in next #1584
Conversation
There is an avoidable `clone` in `next`. This commit introduces `next_for_mut`, which is similar to `next_for`, but updates the key in-place, and uses it in `next` instead.
|
Hello! Sorry to hear about a regression. Can you provide a minimal working example of the regression? Or some additional debug information? |
|
Also, what version caused the regression? |
We are using the version 0.15.6. That said, I do not think this issue is related to the version, since the line
AFAIU, this can be problematic when there are many dimensions (probably bigger than 6, i.e. Re the minimal example, I am still curious if there are perf tests in the repo, or if you want it to have. This "cloning more or less" is not about functional correctness, so it is unclear which type of minimal example you have in your mind. |
|
Ah thank you for the clarification. A "regression" is usually meant as a drop in performance or correctness when compared to a previous version, so I thought this was behavior you were seeing after an upgrade. Unfortunately, performance is a known issue when using dynamic-dimensioned arrays. I haven't had a chance yet to look into the cause, but it's possible that this fix would help. I'd be happy to follow up and test to see if it improves speed. This is where a minimal example would help: the smallest possible code that exhibits the slow behavior that you're seeing. There are some limited benchmarks in the library, but nothing comprehensive. |
|
These changes look good, I appreciate the contribution! I'll put together a small benchmark to ensure that it has a positive impact on performance (I'm 99% sure that it will) and then I will merge the changes. |
| fn next_for(&self, index: Self) -> Option<Self> | ||
| { | ||
| let mut index = index; |
There was a problem hiding this comment.
Why can't this function calls next_for_mut now?
let mut index = index;
self.next_for_mut(&mut index)
3 participants
Hello, everyone.
My colleague @chaehhyun found that there are some regressions due to the
cloneinnextduring a profiling. We suspectix.clone()insideBaseiter::nextis the culprit, but not 100% for sure as of now. We are still experimenting its impact internally.I have a couple of questions:
nextis intentional.Thank you.