WIP: [android] Add Speed class #7955

gpesquero · 2024-04-18T23:07:35Z

gpesquero commented

2024-04-18 23:07:35 +00:00

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 organicmaps/organicmaps#7779.

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

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://git.omaps.dev/organicmaps/organicmaps/pulls/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...

biodranik (Migrated from github.com) reviewed 2024-04-19 07:40:23 +00:00

biodranik (Migrated from github.com) left a comment

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

android/app/src/main/java/app/organicmaps/util/Speed.java

biodranik (Migrated from github.com) commented

2024-04-19 07:39:01 +00:00

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

biodranik (Migrated from github.com) commented

2024-04-19 07:40:05 +00:00

Does caching the locale help to speed up the method?

Are there other alternatives to String.format?

Does caching the locale help to speed up the method? Are there other alternatives to String.format?

android/app/src/main/java/app/organicmaps/util/Speed.java

					
				@ -0,0 +55,4 @@

				    // Option 1: String.format()

				    long start1 = System.nanoTime();

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

biodranik (Migrated from github.com) commented

2024-04-19 07:39:30 +00:00

Can this string be pre-cached?

gpesquero reviewed 2024-04-20 12:58:00 +00:00

android/app/src/main/java/app/organicmaps/util/Speed.java

gpesquero commented

2024-04-20 12:58:00 +00:00

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:

Delete the native call for speed formatting.
Implement decimal separator change detection.
Update units strings at app startup and when settings are changed.
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.

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

biodranik (Migrated from github.com) reviewed 2024-04-20 15:44:11 +00:00

android/app/src/main/java/app/organicmaps/util/Speed.java

biodranik (Migrated from github.com) commented

2024-04-20 15:44:11 +00:00

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?

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?

Gdakgdak22 (Migrated from github.com) approved these changes 2024-07-04 14:52:01 +00:00

gpesquero reviewed 2024-08-08 19:36:58 +00:00

android/app/src/main/java/app/organicmaps/util/Speed.java

gpesquero commented

2024-08-08 19:36:58 +00:00

Hello again @biodranik !!

I've made a few more tests / benchmarks for the speed format in Java, using different implementations:

Option 1: String.format()
Option 2: DecimalFormat class
Option 3: Long.toString() + StringBuffer.insert()
Option 4: Formatter class
Option 5: NumberFormat class

And here is a sample of time values measured while running on an actual Android device:

  Opt1 /  Opt2 /  Opt3 /  Opt4 /  Opt5 (values in us)
   439 /    99 /    29 /   299 /    61
   391 /    72 /    25 /   266 /    60
   403 /   101 /    22 /   311 /    93
   476 /   104 /    45 /   649 /   229
   387 /    75 /    20 /   254 /    60
   821 /    82 /    25 /   305 /    76
   434 /    73 /    20 /   231 /    58
   480 /    76 /    20 /   275 /    65
   435 /    97 /    31 /   404 /   161
   377 /    77 /    20 /   300 /    69
   595 /   119 /    23 /   354 /    74
   399 /   130 /    35 /   467 /   121
   310 /    66 /    19 /   333 /    73
   344 /    68 /    18 /   390 /    88
   504 /   230 /    38 /   476 /    62

The fastest implementation is always Option 3: using Long.toString() and inserting the decimal separator "manually".
This "manual" approach is more or less the same as the one that we have running in c++ in function ToStringPrecisionLocale().

Note that we're dealing here with speed formatting, so only decimal separator is formatted (No thousands separator).

Hello again @biodranik !! I've made a few more tests / benchmarks for the speed format in Java, using different implementations: * Option 1: String.format() * Option 2: DecimalFormat class * Option 3: Long.toString() + StringBuffer.insert() * Option 4: Formatter class * Option 5: NumberFormat class And here is a sample of time values measured while running on an actual Android device: ``` Opt1 / Opt2 / Opt3 / Opt4 / Opt5 (values in us) 439 / 99 / 29 / 299 / 61 391 / 72 / 25 / 266 / 60 403 / 101 / 22 / 311 / 93 476 / 104 / 45 / 649 / 229 387 / 75 / 20 / 254 / 60 821 / 82 / 25 / 305 / 76 434 / 73 / 20 / 231 / 58 480 / 76 / 20 / 275 / 65 435 / 97 / 31 / 404 / 161 377 / 77 / 20 / 300 / 69 595 / 119 / 23 / 354 / 74 399 / 130 / 35 / 467 / 121 310 / 66 / 19 / 333 / 73 344 / 68 / 18 / 390 / 88 504 / 230 / 38 / 476 / 62 ``` The fastest implementation is *always* Option 3: using Long.toString() and inserting the decimal separator "manually". This "manual" approach is more or less the same as the one that we have running in c++ in function ToStringPrecisionLocale(). Note that we're dealing here with speed formatting, so only decimal separator is formatted (No thousands separator).

gpesquero reviewed 2024-08-08 19:38:06 +00:00

android/app/src/main/java/app/organicmaps/util/Speed.java

gpesquero commented

2024-08-08 19:38:06 +00:00

For the benchmarking of the current c++ implementation, I've implemented an additional JNI call for speed formatting that, instead of returning a Pair<String, String> class, returns a single String class with both the value and unit separated by a semicolon ";".

Option 1 (current): native Pair<String, String> nativeFormatSpeedAndUnits(double metersPerSecond);
Option 2 (new): native String nativeStringFormatSpeedAndUnits(double metersPerSecond);
Option 3: Java formatting implementation (Option 3 in previous benchmarking)

This new second implementation is usually faster that the current one (but always slower than the 3rd implementation: fast Java manual approach):

1=  166 / 2=  144 / 3=   37
1=  159 / 2=  412 / 3=   70
1=  159 / 2=  147 / 3=   36
1=  181 / 2=  159 / 3=   42
1=  157 / 2=  145 / 3=   39
1=  430 / 2=  669 / 3=   58
1=  141 / 2=  118 / 3=   32
1=  537 / 2=  149 / 3=   39
1=  155 / 2=  119 / 3=   32
1=  223 / 2=  137 / 3=   35
1=  142 / 2=  108 / 3=   30
1=  169 / 2=  125 / 3=   33
1=  145 / 2=  139 / 3=   37

Is it worth the implementation of this second approach? It's debatable...

Any feedback is welcomed!!

For the benchmarking of the current c++ implementation, I've implemented an additional JNI call for speed formatting that, instead of returning a Pair<String, String> class, returns a single String class with both the value and unit separated by a semicolon ";". * Option 1 (current): native Pair<String, String> nativeFormatSpeedAndUnits(double metersPerSecond); * Option 2 (new): native String nativeStringFormatSpeedAndUnits(double metersPerSecond); * Option 3: Java formatting implementation (Option 3 in previous benchmarking) This new second implementation is _usually_ faster that the current one (but always slower than the 3rd implementation: fast Java manual approach): 1= 166 / 2= 144 / 3= 37 1= 159 / 2= 412 / 3= 70 1= 159 / 2= 147 / 3= 36 1= 181 / 2= 159 / 3= 42 1= 157 / 2= 145 / 3= 39 1= 430 / 2= 669 / 3= 58 1= 141 / 2= 118 / 3= 32 1= 537 / 2= 149 / 3= 39 1= 155 / 2= 119 / 3= 32 1= 223 / 2= 137 / 3= 35 1= 142 / 2= 108 / 3= 30 1= 169 / 2= 125 / 3= 33 1= 145 / 2= 139 / 3= 37 Is it worth the implementation of this second approach? It's debatable... Any feedback is welcomed!!