From 5c054df0851ddf75ca813a54d7c61f49668bcfac Mon Sep 17 00:00:00 2001 From: Shane Carr Date: Thu, 14 Dec 2017 00:47:43 +0000 Subject: [PATCH] ICU-13495 Optimizing chooseMultiplierAndApply method implementation. X-SVN-Rev: 40732 --- icu4c/source/i18n/number_rounding.cpp | 43 +++++++++------ icu4c/source/i18n/number_types.h | 11 ++++ icu4c/source/i18n/unicode/numberformatter.h | 14 +++++ .../icu/impl/number/MultiplierProducer.java | 8 +++ .../core/src/com/ibm/icu/number/Rounder.java | 53 ++++++++++++++----- 5 files changed, 99 insertions(+), 30 deletions(-) diff --git a/icu4c/source/i18n/number_rounding.cpp b/icu4c/source/i18n/number_rounding.cpp index 5c494f09544..f0cd1135f98 100644 --- a/icu4c/source/i18n/number_rounding.cpp +++ b/icu4c/source/i18n/number_rounding.cpp @@ -251,28 +251,39 @@ void Rounder::setLocaleData(const CurrencyUnit ¤cy, UErrorCode &status) { int32_t Rounder::chooseMultiplierAndApply(impl::DecimalQuantity &input, const impl::MultiplierProducer &producer, UErrorCode &status) { - // TODO: Make a better and more efficient implementation. - // TODO: Avoid the object creation here. - DecimalQuantity copy(input); - + // Do not call this method with zero. U_ASSERT(!input.isZero()); - int32_t magnitude = input.getMagnitude(); - int32_t multiplier = producer.getMultiplier(magnitude); + + // Perform the first attempt at rounding. + int magnitude = input.getMagnitude(); + int multiplier = producer.getMultiplier(magnitude); input.adjustMagnitude(multiplier); apply(input, status); - // If the number turned to zero when rounding, do not re-attempt the rounding. - if (!input.isZero() && input.getMagnitude() == magnitude + multiplier + 1) { - magnitude += 1; - input = copy; - multiplier = producer.getMultiplier(magnitude); - input.adjustMagnitude(multiplier); - U_ASSERT(input.getMagnitude() == magnitude + multiplier - 1); - apply(input, status); - U_ASSERT(input.getMagnitude() == magnitude + multiplier); + // If the number rounded to zero, exit. + if (input.isZero() || U_FAILURE(status)) { + return multiplier; + } + + // If the new magnitude after rounding is the same as it was before rounding, then we are done. + // This case applies to most numbers. + if (input.getMagnitude() == magnitude + multiplier) { + return multiplier; } - return multiplier; + // If the above case DIDN'T apply, then we have a case like 99.9 -> 100 or 999.9 -> 1000: + // The number rounded up to the next magnitude. Check if the multiplier changes; if it doesn't, + // we do not need to make any more adjustments. + int _multiplier = producer.getMultiplier(magnitude + 1); + if (multiplier == _multiplier) { + return multiplier; + } + + // We have a case like 999.9 -> 1000, where the correct output is "1K", not "1000". + // Fix the magnitude and re-apply the rounding strategy. + input.adjustMagnitude(_multiplier - multiplier); + apply(input, status); + return _multiplier; } /** This is the method that contains the actual rounding logic. */ diff --git a/icu4c/source/i18n/number_types.h b/icu4c/source/i18n/number_types.h index 2bc21bd40dc..a404ef9686d 100644 --- a/icu4c/source/i18n/number_types.h +++ b/icu4c/source/i18n/number_types.h @@ -230,10 +230,21 @@ class U_I18N_API MicroPropsGenerator { virtual void processQuantity(DecimalQuantity& quantity, MicroProps& micros, UErrorCode& status) const = 0; }; +/** + * An interface used by compact notation and scientific notation to choose a multiplier while rounding. + */ class MultiplierProducer { public: virtual ~MultiplierProducer() = default; + /** + * Maps a magnitude to a multiplier in powers of ten. For example, in compact notation in English, a magnitude of 5 + * (e.g., 100,000) should return a multiplier of -3, since the number is displayed in thousands. + * + * @param magnitude + * The power of ten of the input number. + * @return The shift in powers of ten. + */ virtual int32_t getMultiplier(int32_t magnitude) const = 0; }; diff --git a/icu4c/source/i18n/unicode/numberformatter.h b/icu4c/source/i18n/unicode/numberformatter.h index 4a11c2f9157..546b1875ff4 100644 --- a/icu4c/source/i18n/unicode/numberformatter.h +++ b/icu4c/source/i18n/unicode/numberformatter.h @@ -836,6 +836,20 @@ class U_I18N_API Rounder : public UMemory { /** Version of {@link #apply} that obeys minInt constraints. Used for scientific notation compatibility mode. */ void apply(impl::DecimalQuantity &value, int32_t minInt, UErrorCode status); + /** + * Rounding endpoint used by Engineering and Compact notation. Chooses the most appropriate multiplier (magnitude + * adjustment), applies the adjustment, rounds, and returns the chosen multiplier. + * + *

+ * In most cases, this is simple. However, when rounding the number causes it to cross a multiplier boundary, we + * need to re-do the rounding. For example, to display 999,999 in Engineering notation with 2 sigfigs, first you + * guess the multiplier to be -3. However, then you end up getting 1000E3, which is not the correct output. You then + * change your multiplier to be -6, and you get 1.0E6, which is correct. + * + * @param input The quantity to process. + * @param producer Function to call to return a multiplier based on a magnitude. + * @return The number of orders of magnitude the input was adjusted by this method. + */ int32_t chooseMultiplierAndApply(impl::DecimalQuantity &input, const impl::MultiplierProducer &producer, UErrorCode &status); diff --git a/icu4j/main/classes/core/src/com/ibm/icu/impl/number/MultiplierProducer.java b/icu4j/main/classes/core/src/com/ibm/icu/impl/number/MultiplierProducer.java index ea422465a01..e39150ae832 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/impl/number/MultiplierProducer.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/impl/number/MultiplierProducer.java @@ -6,5 +6,13 @@ package com.ibm.icu.impl.number; * An interface used by compact notation and scientific notation to choose a multiplier while rounding. */ public interface MultiplierProducer { + /** + * Maps a magnitude to a multiplier in powers of ten. For example, in compact notation in English, a magnitude of 5 + * (e.g., 100,000) should return a multiplier of -3, since the number is displayed in thousands. + * + * @param magnitude + * The power of ten of the input number. + * @return The shift in powers of ten. + */ int getMultiplier(int magnitude); } diff --git a/icu4j/main/classes/core/src/com/ibm/icu/number/Rounder.java b/icu4j/main/classes/core/src/com/ibm/icu/number/Rounder.java index e48c5585714..5624fa8a5f8 100644 --- a/icu4j/main/classes/core/src/com/ibm/icu/number/Rounder.java +++ b/icu4j/main/classes/core/src/com/ibm/icu/number/Rounder.java @@ -485,29 +485,54 @@ public abstract class Rounder implements Cloneable { } } + /** + * Rounding endpoint used by Engineering and Compact notation. Chooses the most appropriate multiplier (magnitude + * adjustment), applies the adjustment, rounds, and returns the chosen multiplier. + * + *

+ * In most cases, this is simple. However, when rounding the number causes it to cross a multiplier boundary, we + * need to re-do the rounding. For example, to display 999,999 in Engineering notation with 2 sigfigs, first you + * guess the multiplier to be -3. However, then you end up getting 1000E3, which is not the correct output. You then + * change your multiplier to be -6, and you get 1.0E6, which is correct. + * + * @param input The quantity to process. + * @param producer Function to call to return a multiplier based on a magnitude. + * @return The number of orders of magnitude the input was adjusted by this method. + */ int chooseMultiplierAndApply(DecimalQuantity input, MultiplierProducer producer) { - // TODO: Make a better and more efficient implementation. - // TODO: Avoid the object creation here. - DecimalQuantity copy = input.createCopy(); - + // Do not call this method with zero. assert !input.isZero(); + + // Perform the first attempt at rounding. int magnitude = input.getMagnitude(); int multiplier = producer.getMultiplier(magnitude); input.adjustMagnitude(multiplier); apply(input); - // If the number turned to zero when rounding, do not re-attempt the rounding. - if (!input.isZero() && input.getMagnitude() == magnitude + multiplier + 1) { - magnitude += 1; - input.copyFrom(copy); - multiplier = producer.getMultiplier(magnitude); - input.adjustMagnitude(multiplier); - assert input.getMagnitude() == magnitude + multiplier - 1; - apply(input); - assert input.getMagnitude() == magnitude + multiplier; + // If the number rounded to zero, exit. + if (input.isZero()) { + return multiplier; + } + + // If the new magnitude after rounding is the same as it was before rounding, then we are done. + // This case applies to most numbers. + if (input.getMagnitude() == magnitude + multiplier) { + return multiplier; } - return multiplier; + // If the above case DIDN'T apply, then we have a case like 99.9 -> 100 or 999.9 -> 1000: + // The number rounded up to the next magnitude. Check if the multiplier changes; if it doesn't, + // we do not need to make any more adjustments. + int _multiplier = producer.getMultiplier(magnitude + 1); + if (multiplier == _multiplier) { + return multiplier; + } + + // We have a case like 999.9 -> 1000, where the correct output is "1K", not "1000". + // Fix the magnitude and re-apply the rounding strategy. + input.adjustMagnitude(_multiplier - multiplier); + apply(input); + return _multiplier; } /////////////// -- 2.40.0