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.

Sign up for GitHub

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

ICU-21684 Fix performance issue in NumberRangeFormatter #1863

Merged
merged 1 commit into from
Sep 15, 2021
Merged

ICU-21684 Fix performance issue in NumberRangeFormatter #1863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion icu4c/source/i18n/numrange_impl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,10 @@ void NumberRangeFormatterImpl::formatRange(UFormattedNumberRangeData& data,
}

length1 += NumberFormatterImpl::writeNumber(micros1, data.quantity1, string, UPRV_INDEX_0, status);
length2 += NumberFormatterImpl::writeNumber(micros2, data.quantity2, string, UPRV_INDEX_2, status);
// ICU-21684: Write the second number to a temp string to avoid repeated insert operations
FormattedStringBuilder tempString;
NumberFormatterImpl::writeNumber(micros2, data.quantity2, tempString, 0, status);
length2 += string.insert(UPRV_INDEX_2, tempString, status);

// TODO: Support padding?

Expand Down
1 change: 1 addition & 0 deletions icu4c/source/test/intltest/numbertest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,7 @@ class NumberRangeFormatterTest : public IntlTestWithFieldPosition {
void testCopyMove();
void toObject();
void testGetDecimalNumbers();
void test21684_Performance();
void test21358_SignPosition();

void runIndexedTest(int32_t index, UBool exec, const char *&name, char *par = 0) override;
Expand Down
9 changes: 9 additions & 0 deletions icu4c/source/test/intltest/numbertest_range.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ void NumberRangeFormatterTest::runIndexedTest(int32_t index, UBool exec, const c
TESTCASE_AUTO(testCopyMove);
TESTCASE_AUTO(toObject);
TESTCASE_AUTO(testGetDecimalNumbers);
TESTCASE_AUTO(test21684_Performance);
TESTCASE_AUTO(test21358_SignPosition);
TESTCASE_AUTO_END;
}
Expand Down Expand Up @@ -910,6 +911,14 @@ void NumberRangeFormatterTest::testGetDecimalNumbers() {
}
}

void NumberRangeFormatterTest::test21684_Performance() {
IcuTestErrorCode status(*this, "test21684_Performance");
LocalizedNumberRangeFormatter lnf = NumberRangeFormatter::withLocale("en");
// The following two lines of code should finish quickly.
lnf.formatFormattableRange({"-1e99999", status}, {"0", status}, status);
lnf.formatFormattableRange({"0", status}, {"1e99999", status}, status);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?)

Copy link
Member Author

Choose a reason for hiding this comment

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

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:

  1. Remove the test, since the bug is fixed
  2. Keep the test and add a flaky clock time checker
  3. Keep the test as proposed

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I guess #3 seems best.


void NumberRangeFormatterTest::test21358_SignPosition() {
IcuTestErrorCode status(*this, "test21358_SignPosition");

Expand Down
5 changes: 4 additions & 1 deletion icu4j/main/classes/core/src/com/ibm/icu/number/NumberRangeFormatterImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,10 @@ private void formatRange(DecimalQuantity quantity1, DecimalQuantity quantity2, F
}

h.length1 += NumberFormatterImpl.writeNumber(micros1, quantity1, string, h.index0());
h.length2 += NumberFormatterImpl.writeNumber(micros2, quantity2, string, h.index2());
// ICU-21684: Write the second number to a temp string to avoid repeated insert operations
FormattedStringBuilder tempString = new FormattedStringBuilder();
NumberFormatterImpl.writeNumber(micros2, quantity2, tempString, 0);
h.length2 += string.insert(h.index2(), tempString);

// TODO: Support padding?

Expand Down
9 changes: 9 additions & 0 deletions icu4j/main/tests/core/src/com/ibm/icu/dev/test/number/NumberRangeFormatterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// License & terms of use: http://www.unicode.org/copyright.html
package com.ibm.icu.dev.test.number;

import java.math.BigDecimal;
import java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
Expand Down Expand Up @@ -854,6 +855,14 @@ public void testNumberingSystemRangeData() {
}
}

@Test
public void test21684_Performance() {
LocalizedNumberRangeFormatter lnf = NumberRangeFormatter.withLocale(ULocale.ENGLISH);
// 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"));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same idea as above about generating an error if execution too long...


@Test
public void test21358_SignPosition() {
// de-CH has currency pattern "¤ #,##0.00;¤-#,##0.00"
Expand Down