search

organicmaps / organicmaps

🍃 Organic Maps is a free Android & iOS offline maps app for travelers, tourists, hikers, and cyclists. It uses crowd-sourced OpenStreetMap data and is developed with love by MapsWithMe (MapsMe) founders and our community. No ads, no tracking, no data collection, no crapware. Please donate to support the development!
https://organicmaps.app
Apache License 2.0
8.58k stars 837 forks source link

[android] Add Speed class #7955

Open gpesquero opened 2 weeks ago

gpesquero commented 2 weeks ago

PR that adds a static Speed class for formatting speed values to strings in Android, instead of using JNI calls.

This is just a draft PR to compare the current c++/JNI string formatting approach vs speed formatting to string in Android, as discussed in https://github.com/organicmaps/organicmaps/pull/7779#discussion_r1552523638.

This PR runs both speed string formatting implementations (c++/JNI vs Android) in NavMenu.java, logging the time elapsed for each call (measurements have been made on a real Android 9 device via USB debugging):

Values are in us (microsec)
Current: c++/JNI implementation
New: Java/Android implementation
Diff = New - Current
(x1.1f) = New / Current 

Current / New / Diff (us):  180 /  837 /  657 (x4.6)
Current / New / Diff (us):  135 /  443 /  308 (x3.3)
Current / New / Diff (us):  125 /  387 /  262 (x3.1)
Current / New / Diff (us):  123 /  491 /  368 (x4.0)
Current / New / Diff (us):  124 /  366 /  243 (x3.0)
Current / New / Diff (us):  129 /  445 /  316 (x3.4)
Current / New / Diff (us):  146 /  431 /  285 (x3.0)
Current / New / Diff (us):  116 /  367 /  251 (x3.2)
Current / New / Diff (us):  110 /  324 /  214 (x2.9)
Current / New / Diff (us):  117 /  405 /  289 (x3.5)
Current / New / Diff (us):  125 /  448 /  323 (x3.6)
Current / New / Diff (us):  126 /  401 /  275 (x3.2)
Current / New / Diff (us):  167 /  416 /  249 (x2.5)

Android/Java implementation takes 3-4 times longer than the current JNI one, so actually the current implementation seems to be the best approach performance-wise.

The question here is: shall we keep the current approach?, or shall we start to implement speed and distance formatting in Android and remove JNI calls? This second option would also imply to add unit testing for Java/Android.

My opinion: to keep the current approach (string formatting in c++/JNI).

Any feedback in welcome...

RedAuburn commented 2 weeks ago

This looks nicely done, but I do think it's preferable to keep as much logic as possible in the core, otherwise every platform will end up with its own implementation & it'll be a pain to maintain

(you might fancy working on https://github.com/organicmaps/organicmaps/issues/3676 with your knowledge of the units implementation though!)

gpesquero commented 1 week ago

This looks nicely done, but I do think it's preferable to keep as much logic as possible in the core, otherwise every platform will end up with its own implementation & it'll be a pain to maintain

We're always going to have a trade-off between the different approaches. You're right that keeping the implementation in the core will be easier to maintain.

(you might fancy working on #3676 with your knowledge of the units implementation though!)

I will take note of this issue/feature request and see if I have time in the future to work on it. In any case, if we want to implement this new feature in the future (i.e. mixing miles with meters), I'd be easier if we have the distance/speed formatting in the core.