Skip to content
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

[android] Add Speed class #7955

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gpesquero
Copy link
Contributor

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 #7779 (comment).

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...

@gpesquero gpesquero marked this pull request as draft April 18, 2024 23:08
Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I like the approach to measure 👍 A few comments.

if (units == Framework.UNITS_IMPERIAL)
{
speedValue = MpsToMiph(speedInMetersPerSecond);
unitsString = context.getString(R.string.miles_per_hour);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if these strings will be loaded once (avoid calling context.getString on each call)?

unitsString = context.getString(R.string.kilometers_per_hour);
}

String formatString = (speedValue < 10.0)? "%.1f" : "%.0f";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this string be pre-cached?


String formatString = (speedValue < 10.0)? "%.1f" : "%.0f";

String speedString = String.format(Locale.getDefault(), formatString, speedValue);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does caching the locale help to speed up the method?

Are there other alternatives to String.format?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there other alternatives to String.format?

I've made some timing comparison with 2 other alternatives to String.format().

You can find the code for this comparison in the function formatMeasurements().

  • Option 1: String.format(Locale, double)
  • Option 2: DecimalFormat("#.#").format(double)
  • Option 3: Long.toString(Math.round(speedValue * 10.0)) + StringBuffer(String).insert(1, char) (inserting the decimal separator into the string)

These are the average times for each option:

  • Option 1: 250 us
  • Option 2: 55 us
  • Option 3: 15 us

Option 3 is by far the fastest one, though it requires to insert manually the decimal separator into the string (as we are doing in c++ right now).

With option 3, we even get a better performance compared to the current JNI calls. These are the new measurements:

Current / New  / Diff (x1.1f)
    210 /   27 / -184 (x0.1)
    123 /   45 /  -78 (x0.4)
    111 /   20 /  -90 (x0.2)
    112 /   21 /  -91 (x0.2)
    115 /   18 /  -97 (x0.2)
    112 /   25 /  -87 (x0.2)
    132 /   22 / -110 (x0.2)
    126 /   23 / -103 (x0.2)
    123 /   29 /  -94 (x0.2)
    120 /   22 /  -97 (x0.2)
    185 /   28 / -156 (x0.2)
    140 /   44 /  -97 (x0.3)
    123 /   22 / -101 (x0.2)
    130 /   26 / -104 (x0.2)
    111 /   22 /  -89 (x0.2)
    108 /   16 /  -92 (x0.1)
    123 /   20 / -103 (x0.2)
    120 /   29 /  -91 (x0.2)
    127 /   21 / -106 (x0.2)

This 3rd option seems to be a good solution performance-wise, so we could start to implement it in this PR, including:

  1. Delete the native call for speed formatting.
  2. Implement decimal separator change detection.
  3. Update units strings at app startup and when settings are changed.
  4. Implement speed formatting unit tests in Android

I have to check also if Android Auto also uses native calls for speed formatting, and see if this solution also applies there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about separating thousands? There are also Formatter formatter = new Formatter(sb, Locale.US); and java.text.NumberFormat

Note that there are also values displayed in the Place Page (altitude, speed) that also should be formatted. Ideally, doing it in one place and supporting only one implementation would be the best way to go than supporting C++ version and custom Java formatters for Android.

As you've already established a test environment, does it make sense to check how the current C++ implementation can be sped up? Is there any overhead there? Can JNI calls be optimized?

@RedAuburn
Copy link
Sponsor Member

RedAuburn commented Apr 19, 2024

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 #3676 with your knowledge of the units implementation though!)

@gpesquero
Copy link
Contributor Author

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.

Signed-off-by: Gonzalo Pesquero <gpesquero@yahoo.es>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants