-
Notifications
You must be signed in to change notification settings - Fork 319
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
Memory efficient replay route #6636
Conversation
427788f
to
99a6929
Compare
99a6929
to
f1f6cca
Compare
Codecov Report
@@ Coverage Diff @@
## main #6636 +/- ##
============================================
+ Coverage 72.31% 72.35% +0.03%
- Complexity 5323 5369 +46
============================================
Files 753 755 +2
Lines 29089 29250 +161
Branches 3448 3476 +28
============================================
+ Hits 21037 21164 +127
- Misses 6667 6681 +14
- Partials 1385 1405 +20
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kmadsen , lazy initialisation is a great idea for replay events! It happened to me that I wasn't able to simulate 10Hz location updates for Gdansk-Lisbon route, app crashed with OOM
override fun next(): Point { | ||
var result = 1 | ||
var shift = 0 | ||
var temp: Int | ||
do { | ||
temp = encodedPath[index++].code - 63 - 1 | ||
result += temp shl shift | ||
shift += 5 | ||
} while (temp >= 0x1f) | ||
lat += if (result and 1 != 0) (result shr 1).inv() else result shr 1 | ||
|
||
result = 1 | ||
shift = 0 | ||
do { | ||
temp = encodedPath[index++].code - 63 - 1 | ||
result += temp shl shift | ||
shift += 5 | ||
} while (temp >= 0x1f) | ||
lng += if (result and 1 != 0) (result shr 1).inv() else result shr 1 | ||
|
||
return Point.fromLngLat(lng / factor, lat / factor).also { next -> | ||
current = next | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RouteLine
decodes full route anyway and put it into cache. Maybe instead of decoding geometry by small pieces you can get full decoded from the cache? Or in case route simulation was launched earlier than route line, you can decode full route on background thread and put it into the cache so that route line will reuse decoded geometry later.
What do you think about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are a few options. Another option is putting the geometry string into a file (also can be a memory cache). We can decode with an InputStream mapbox/mapbox-java#1518.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now this ReplayPolylineDecodeStream
is internal and considered temporary. I still want to decode it as a Iterable stream because that makes the ReplayRouteSession
usage simpler. But i'm okay if geometry is stored in a file, cache, or memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good enough for the first iteration 👍
/** | ||
* Take a list of [Point] and map it to events that can be replayed by the [MapboxReplayer]. | ||
* | ||
* @param points containing location coordinates to be replayed. | ||
* @return [ReplayEventBase] [List] | ||
*/ | ||
fun mapPointList(points: List<Point>): List<ReplayEventBase> { | ||
return replayRouteDriver.drivePointList(options, points) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like current implementation 👍
But I think the same result could be achieved easier using Sequence
/** | |
* Take a list of [Point] and map it to events that can be replayed by the [MapboxReplayer]. | |
* | |
* @param points containing location coordinates to be replayed. | |
* @return [ReplayEventBase] [List] | |
*/ | |
fun mapPointList(points: List<Point>): List<ReplayEventBase> { | |
return replayRouteDriver.drivePointList(options, points) | |
/** | |
* Simulate a driver navigating a route | |
* | |
* @param geometry is a [DirectionRoute] | |
* @return [ReplayEventBase] [Sequence] | |
*/ | |
suspend fun mapPointList(route: DirectionRoute): Sequence<ReplayEventBase> { | |
val decodeGeometry = withContext(Dispatchers.Default) { | |
route.completeGeometryToPoints() | |
} | |
return replayRouteDriver.drivePointList(options, points) |
And if you replace lists by sequences in replayRouteDriver.drivePointList
and all algorithms under the hood, every element of the sequence will be calculated on demand, i.e. lazily.
What do you think about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the functions in the ReplayRouteMapper
return a List
. And the replayer still takes events as a list
fun pushEvents(events: List<ReplayEventBase>)
So I don't think it is easier to convert back and forth at this time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The algorithms under the covers are still using lists, because they dedup locations on the route and they estimate the route curvatures. I agree that iterable sequence can be better, but in this case we're also sharing algorithms and moving code back and forth between mapbox java https://github.com/mapbox/mapbox-java/blob/1b867fe8e68b299f37e5591c8431df89881ca9c2/services-geojson/src/main/java/com/mapbox/geojson/utils/PolylineUtils.java#L39. Some of the decisions were based on the tools that exist in mapbox-java
It will be a big change to turn everything into a sequence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that we are trying to make replay "streamable", I also agree Sequence/Iterable/InputStream is where we should be going. But the ReplayRouteMapper
is going to continue to be a List
creator.
Take a look at ReplayRouteDriver
, that is what we want to work on making into a Sequence
. And then the mapper will convert the sequence into a List
to maintain backwards compatibility.
I've been thinking about this too, so am just sharing the thoughts on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that we are trying to make replay "streamable", I also agree Sequence/Iterable/InputStream is where we should be going. But the
ReplayRouteMapper
is going to continue to be aList
creator.Take a look at
ReplayRouteDriver
, that is what we want to work on making into aSequence
. And then the mapper will convert the sequence into aList
to maintain backwards compatibility.
I agree, we have to leave current ReplayRouteMapper
as is with List
just because we can't break this API. What if we introduce new API with Sequence
in parallel by copy pasting ReplayRouteMapper
and replacing lists by sequences?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is essentially the approach I was taking here #6582
Instead of a Sequence
it's an Iterator
. I'm not opposed to introducing it, but it still requires bug fixing
public interface ReplayEventStream extends Closeable, Iterator<ReplayEventBase>
* The replay session will be enabled when [MapboxNavigation] is attached. | ||
*/ | ||
@ExperimentalPreviewMapboxNavigationAPI | ||
class ReplayRouteSession : MapboxNavigationObserver { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have users who still create MapboxNavigaiton
using constructor. Will they be able to use this lazy mechanism without migrating to MapboxNavigationApp
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. The MapboxNavigationObserver
can be used without MapboxNavigationApp
. The interface exposes the functions.
interface MapboxNavigationOberver {
fun onAttached(mapboxNavigation: MapboxNavigation)
fun onDetached(mapboxNavigation: MapboxNavigation)
}
This means you can use ReplayRouteSession
in this way. Call start and stop replay whenever you have an instance of MapboxNavigation
. But this is NOT RECOMMENDED because we want to share this object with android auto.
val replayRouteSession = ReplayRouteSession()
fun startReplay() {
replayRouteSession.onAttached(mapboxNavigation)(
}
fun stopReplay() {
replayRouteSession.onDetached(mapboxNavigation)(
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested using the route Gdańsk(Poland)-Ho Chi Minh City(Vietnam) with 10Hz location update. On main branch the test app crashes with OOM. But with new optimisation simulation works fine, java heap doesn't grow more the 0.2 Gb 👍
Well done! 💪
Let's merge and iterate improving this solution 🚀
Improvements I recommend:
- Try using
Sequience
, that would allow driver not to stop every X km because we don't process routes by chunks - Try reusing existing decoded route geometry from cache
Description
This adds a
ReplayRouteSession
that is able to replay super large routes at high frequencies, without running out of memory. It also integrates it into drop-in-ui, so that we can all use it!How it works. The developer pushes events into a replayer, and the replayer simulates the events in real time. The current replayer will take all events from a route and hold them in memory. The new
ReplayRouteSession
will only replaydecodeMinDistance
events at a time. For example, if you create a 1000 kilometers route and set the decodeMinDistance to 10 kilometers. You can expectReplayRouteSession
to only hold ~10km of events in memory at a time.Api explanation
This object is experimental but for the intents and purposes it will become permanent. The details under the cover are subject to change, but the top interface is going to be set in stone.
Known issue
The
ReplayRouteMapper
was designed to simulate events from A to B, where the starting speed and the ending speed is 0.0 meters per second.ReplayRouteSession
is using theReplayRouteMapper
to simulate each section of the route, so the driver will appear to stop and speed up again. The larger thedecodeMinDistance
, the less often this will happen.Also note that this issue is fixable but will require more time and effort.
Here are memory approximations to help you pick
frequency
anddecodeMinDistance
values. Duration is more accurate for determining memory sizes, but duration requires more resources to calculate than distance. The parameters may be removed after the internal improvements are completed.What kind of memory improvement to expect
Old formulas to determine memory requirements
New formulas
In other words: When duration becomes 40 hours. The memory requirement can be clamped to 30 minutes.