ICU-21684 Fix performance issue in NumberRangeFormatter #1863
Conversation
| // The following two lines of code should finish quickly. | ||
| lnf.formatFormattableRange({"-1e99999", status}, {"0", status}, status); | ||
| lnf.formatFormattableRange({"0", status}, {"1e99999", status}, status); | ||
| } |
There was a problem hiding this comment.
This does not actually generate an error if the execution time is long. Is the idea for anyone running the tests to just look at the logged execution time and do something if it seems too long? Maybe we should at least have a infoln stating that; but that would not affect CI tests. Maybe better to call uprv_getRawUTCtime() at the beginning and end of the test and generate an error if the difference is significantly greater than expected (i.e. 20 seconds?)
There was a problem hiding this comment.
The problem with checking clock time is that the clock time really differs a lot depending on the environment. Not only CPU speed, but also whether the library was built with ASAN, run with Valgrind, etc. So as of now I basically use this as a manual qualitative runtime test.
I can either:
- Remove the test, since the bug is fixed
- Keep the test and add a flaky clock time checker
- Keep the test as proposed
There was a problem hiding this comment.
OK, I guess #3 seems best.
| // The following two lines of code should finish quickly. | ||
| lnf.formatRange(new BigDecimal("-1e99999"), new BigDecimal("0")); | ||
| lnf.formatRange(new BigDecimal("0"), new BigDecimal("1e99999")); | ||
| } |
There was a problem hiding this comment.
Same idea as above about generating an error if execution too long...
Checklist