[RSDK-12819] Cache switching points to bound searches#9
[RSDK-12819] Cache switching points to bound searches#9acmorrow merged 2 commits intoviam-modules:mainfrom
Conversation
|
@nfranczak - This has an enormous impact on the gp12 regression files: Old: Ouch. It took 3.5 seconds! New: Oh cool, it's 45ms. |
| // Returns the first valid switching point found, or the end of the trajectory if none is found. | ||
| // | ||
| // Takes path::cursor by value (cheap copy) to seed forward search from current position. | ||
| [[gnu::pure]] switching_point find_acceleration_switching_point(path::cursor cursor, const trajectory::options& opt) { |
There was a problem hiding this comment.
It wasn't meaningful for these methods the way it was for the math
| // Takes path::cursor by value (cheap copy) to seed forward search from current position. | ||
| [[gnu::pure]] switching_point find_acceleration_switching_point(path::cursor cursor, const trajectory::options& opt) { | ||
| std::optional<switching_point> find_acceleration_switching_point(path::cursor cursor, | ||
| arc_length max_search_hint, |
There was a problem hiding this comment.
search hint caps the range of search?
.. not a huge fan of the word hint here, preference for something more explicit.
There was a problem hiding this comment.
Yes. If we have a vel sp in front of us, we can stop searching for accel sp's at it, and vice versa. The hint is the last position you need to search to. I called it hint because you are free to search further, and in some cases, we will. For instance, when finding non-diff extrema of the accel limit curve, we will do all of them in the segment we are looking at, and we won't stop at the hint: there are a finite number of non-diff switching points in the segment, and it doesn't do any harm to find them.
If we wanted to go harder at this, we could arrange things so we cached those too, but it didn't seem worth it until we have phase_plane in hand. It does mean that in circular segments we may recompute the same non-diff switching points a few times.
I think that will get fixed when we do RSDK-12891 for real, and go to the unified switching point search. Then all this hinting and caching goes away, because you don't have two search invocations to limit or two results to reconcile.
|
|
||
| // No valid switching point found before path end - return end of path as switching point | ||
| return switching_point{.point = {.s = cursor.path().length(), .s_dot = arc_velocity{0.0}}, | ||
| .kind = trajectory::switching_point_kind::k_path_end}; |
There was a problem hiding this comment.
why has this been deleted? This seems correct.
There was a problem hiding this comment.
The find_switching_point function handles that now.
| return sp1; | ||
| } | ||
|
|
||
| if (epsilon.wrap(sp1->point.s) == epsilon.wrap(sp2->point.s)) { |
There was a problem hiding this comment.
why epsilon wrapped? Seems like small differences matter here.
There was a problem hiding this comment.
Well, the bad answer is that that is how the old one did it, so I didn't change it. A better answer is that I think you are right that we probably shouldn't do this.
However, my goal with this code is that it not introduce any behavioral changes other than accelerating switching point search, so I'm a little reluctant to take it out.
2 participants
We were spending insane amounts of time repeatedly scanning for switching points. Now we cache the results of previous searches and use them to bound the search for the other type. Big win:
Old timings:
New timings:
That's an 8x improvement on that third case.