<i18n dev> Review Request for CR : 7050528 Improve performance of java.text.DecimalFormat.format() call stack
Olivier Lagneau
olivier.lagneau at oracle.com
Wed Sep 14 05:20:39 PDT 2011
Thanks for the review and the quick reply Joe !
I agree with all your suggestions and will integrate them.
For sure I did not pay enough attention to the serialization part of it.
The on demand holder class idiom for 6k of "The Fast Path for double
Constants" is something
I had in mind but forgot to do.
Thanks a lot for the patch you provided that fixes the coding style
rules too.
That will quicken a lot the integration of your suggestions.
I will check the perf result of the new code too.
Olivier.
Joe Darcy said on date 9/14/2011 8:04 AM:
> Hello Olivier.
>
> Olivier Lagneau wrote:
>> Please review The implementation of a fast-path algorithm for
>> optimizing the
>> DecimalFormat(double, ...) call stack.
>>
>> The webrev is here :
>> http://cr.openjdk.java.net/~alanb/7050528/webrev.01
>>
>> As described in the CR evaluation and suggested fix,
>> the speed-up on a micro-benchmark is ~x11 on Sparc-USIV arch and ~x10
>> on Sparc-T4.
>> The footprint cost is ~6kbytes of static char array constant data to
>> collect digits from integer values.
>>
>> New test dedicated at checking fast-path algorithm, as well as
>> micro-benchmark,
>> are coming soon in a subsequent webrev containing them.
>>
>
> I have some initial comments on your changes.
>
> First, since the DecimalFormat class is serializable all the new
> fields should be transient and the readObject method should be updated
> to initialize them accordingly. However, I suggest putting the
> fast-path specific data into a package private static nested class of
> DecimalFormat and having a single transient pointer in DecimalFormat
> to the helper object.
>
> Since the general contract of NumberFormat is for external
> synchronization, it it not incorrect that the various new mutable
> fields are not declared to be volatile (or final).
>
> Also in terms of organizing data, I suggest using the initialize on
> demand holder class idiom for 6k of "The Fast Path for double
> Constants" info. That is, create a static nested class to store the
> data. This pattern ensures that the data is not loaded unnecessary
> and avoids the need for synchronization when the data is accessed.
>
> I've attached a patch below which changes the code to more closely
> follow JDK whitespace conventions. It also changes a number of
> instances of
>
> if (condition)
> foo = true;
> else
> foo = false;
>
> to
>
> foo = condition;
>
> as well as making the new fields transient (no readObject update) and
> using a sun.mis.FpUtils.isFinite method. Diff below.
>
> Cheers,
>
> -Joe
>
> 523c523,524
> < // If fieldPosition is a DontCareFieldPosition instance we
> can try to go to fast-path code.
> ---
> > // If fieldPosition is a DontCareFieldPosition instance we can
> > // try to go to fast-path code.
> 525c526,527
> < if (fieldPosition == DontCareFieldPosition.INSTANCE)
> tryFastPath = true;
> ---
> > if (fieldPosition == DontCareFieldPosition.INSTANCE)
> > tryFastPath = true;
> 892c894,895
> < else if (fractionalPartAsInt != 0) boundIndex =
> nbFractionalDigits;
> ---
> > else if (fractionalPartAsInt != 0)
> > boundIndex = nbFractionalDigits;
> 909c912
> < // Applies healf-even rounding rule.
> ---
> > // Applies half-even rounding rule.
> 912c915,916
> < else return (number > perfectHalf);
> ---
> > else
> > return (number > perfectHalf);
> 916,917c920,922
> < // Returns the exact number of digits (radix 10) representing
> < // the passed paramater i. Sets utility formating fields used
> in the algorithm
> ---
> > // Returns the exact number of digits (radix 10) representing the
> > // passed paramater i. Sets utility formating fields used in the
> > // algorithm
> 927,930c932,933
> < }
> < else {
> < if (i == 99) isAllNines = true;
> < else isAllNines = false;
> ---
> > } else {
> > isAllNines = (i == 99);
> 934,937c937,938
> < }
> < else {
> < if (i == 999) isAllNines = true;
> < else isAllNines = false;
> ---
> > } else {
> > isAllNines = (i == 999);
> 943,944c944
> < if (i == 9999) isAllNines = true;
> < else isAllNines = false;
> ---
> > isAllNines = (i == 9999);
> 947,950c947,948
> < }
> < else {
> < if (i == 99999) isAllNines = true;
> < else isAllNines = false;
> ---
> > } else {
> > isAllNines = (i == 99999);
> 954,955c952
> < }
> < else {
> ---
> > } else {
> 958,959c955
> < if (i == 999999) isAllNines = true;
> < else isAllNines = false;
> ---
> > isAllNines = (i == 999999);
> 962,965c958,959
> < }
> < else if (i <= 9999999) {
> < if (i == 9999999) isAllNines = true;
> < else isAllNines = false;
> ---
> > } else if (i <= 9999999) {
> > isAllNines = (i == 9999999);
> 968,971c962,963
> < }
> < else {
> < if (i == 99999999) isAllNines = true;
> < else isAllNines = false;
> ---
> > } else {
> > isAllNines = (i == 99999999);
> 975,978c967,968
> < }
> < else if (i <= 999999999) {
> < if (i == 999999999) isAllNines = true;
> < else isAllNines = false;
> ---
> > } else if (i <= 999999999) {
> > isAllNines = (i == 999999999);
> 981,982c971
> < }
> < else {
> ---
> > } else {
> 994c983,984
> < if (i == 999) isAllNines = true;
> ---
> > if (i == 999)
> > isAllNines = true;
> 996,998c986,988
> < }
> < else if (i < 10) {
> < if (i == 9) isAllNines = true;
> ---
> > } else if (i < 10) {
> > if (i == 9)
> > isAllNines = true;
> 1000,1002c990,992
> < }
> < else {
> < if (i == 99) isAllNines = true;
> ---
> > } else {
> > if (i == 99)
> > isAllNines = true;
> 1008,1009c998,1000
> < // Extracts and returns the 2 (Currency pattern) or 3 (Decimal
> pattern)
> < // digits int representing the fractional part of passed double.
> ---
> > // Extracts and returns the 2 (Currency pattern) or 3 (Decimal
> > // pattern) digits int representing the fractional part of passed
> > // double.
> 1017,1018c1008
> < }
> < else {
> ---
> > } else {
> 1030d1019
> <
> 1058,1060c1047,1048
> < if ((minimumIntegerDigits == 1) &&
> < (maximumIntegerDigits >= 10)) isFastPath = true;
> < else isFastPath = false;
> ---
> > isFastPath = ((minimumIntegerDigits == 1) &&
> > (maximumIntegerDigits >= 10));
> 1067,1070c1055,1059
> < (maximumFractionDigits != 2)) isFastPath =
> false;
> < }
> < else if ((minimumFractionDigits != 0) ||
> < (maximumFractionDigits != 3)) isFastPath =
> false;
> ---
> > (maximumFractionDigits != 2))
> > isFastPath = false;
> > } else if ((minimumFractionDigits != 0) ||
> > (maximumFractionDigits != 3))
> > isFastPath = false;
> 1072,1073c1061,1062
> < }
> < else isFastPath = false;
> ---
> > } else
> > isFastPath = false;
> 1121,1122c1110
> < }
> < else {
> ---
> > } else {
> 1126,1127c1114
> < }
> < else if (fastPathWasOn) {
> ---
> > } else if (fastPathWasOn) {
> 1145c1132,1133
> < if (len == 1) digitsBuffer[lowerIndex] = digit;
> ---
> > if (len == 1)
> > digitsBuffer[lowerIndex] = digit;
> 1154c1142,1143
> < if (--upperIndex > lowerIndex)
> digitsBuffer[upperIndex] = digit;
> ---
> > if (--upperIndex > lowerIndex)
> > digitsBuffer[upperIndex] = digit;
> 1165c1154,1155
> < else suffix = charsPositiveSuffix;
> ---
> > else
> > suffix = charsPositiveSuffix;
> 1169c1159,1160
> < if (len == 0) return;
> ---
> > if (len == 0)
> > return;
> 1181,1184c1172,1177
> < if (len > 2) internalChars[dstLower] = suffix[1];
> < if (len == 4) internalChars[dstUpper] = suffix[2];
> < }
> < else System.arraycopy(suffix, 0, internalChars, startIndex,
> len);
> ---
> > if (len > 2)
> > internalChars[dstLower] = suffix[1];
> > if (len == 4)
> > internalChars[dstUpper] = suffix[2];
> > } else
> > System.arraycopy(suffix, 0, internalChars, startIndex,
> len);
> 1192c1185,1186
> < // We localize digits only, taking into account
> currency/decimal fractional case
> ---
> > // We localize digits only, taking into account
> > // currency/decimal fractional case
> 1199,1201c1193,1195
> < }
> < else {
> < // Cursor char is decimal separator or grouping
> char. Just reinit counter.
> ---
> > } else {
> > // Cursor char is decimal separator or grouping
> > // char. Just reinit counter.
> 1212c1206,1207
> < if (!toBeRounded) fillDigit = '9';
> ---
> > if (!toBeRounded)
> > fillDigit = '9';
> 1234,1235c1229,1230
> < }
> < else break;
> ---
> > } else
> > break;
> 1239,1240c1234,1237
> < if (!toBeRounded || isCurrencyFormat) lastUsedIndex +=
> fractionalLength;
> < else lastUsedIndex--; // fractional part absent
> ---
> > if (!toBeRounded || isCurrencyFormat)
> > lastUsedIndex += fractionalLength;
> > else
> > lastUsedIndex--; // fractional part absent
> 1269,1270c1266,1267
> < }
> < else digitsBuffer[index] = DigitTens1000[number];
> ---
> > } else
> > digitsBuffer[index] = DigitTens1000[number];
> 1289,1290c1286
> < }
> < else {
> ---
> > } else {
> 1292c1288,1289
> < if (number == 0) index -= 4;
> ---
> > if (number == 0)
> > index -= 4;
> 1302,1303c1299
> < }
> < else {
> ---
> > } else {
> 1352,1353c1348
> < }
> < else {
> ---
> > } else {
> 1368,1369c1363
> < }
> < else {
> ---
> > } else {
> 1373,1374c1367
> < }
> < else {
> ---
> > } else {
> 1386c1379,1380
> < else formatAllNinesCase(internalChars, startIndex,
> ---
> > else {
> > formatAllNinesCase(internalChars, startIndex,
> 1388a1383
> > }
> 1391c1386,1387
> < if (zeroDelta != 0) localizeDigits(internalChars, startIndex);
> ---
> > if (zeroDelta != 0)
> > localizeDigits(internalChars, startIndex);
> 1394c1390,1391
> < if (isSuffixPresent) appendSuffix(negative, internalChars);
> ---
> > if (isSuffixPresent)
> > appendSuffix(negative, internalChars);
> 1411c1408,1409
> < if (fastPathCheckNeeded) checkAndSetFastPathStatus();
> ---
> > if (fastPathCheckNeeded)
> > checkAndSetFastPathStatus();
> 1414,1416c1412,1413
> < if (!Double.isNaN(number) &&
> < !Double.isInfinite(number)) {
> <
> ---
> > // If finite
> > if (sun.misc.FpUtils.isFinite(number)) {
> 1427,1428c1424
> < }
> < else if (number < 0.0) { // 2 tests for negative
> doubles
> ---
> > } else if (number < 0.0) { // 2 tests for negative
> doubles
> 1436,1437c1432
> < }
> < else if ( 1/number > 0.0) { // 3 tests for positive
> zero. Not frequent.
> ---
> > } else if ( 1/number > 0.0) { // 3 tests for
> positive zero. Not frequent.
> 1442,1443c1437
> < }
> < else { // 4 tests for negative zero (hopefully very
> rare)
> ---
> > } else { // 4 tests for negative zero (hopefully
> very rare)
> 3828c3822
> < private boolean isAllNines = false;
> ---
> > private transient boolean isAllNines = false;
> 3831c3825
> < private double remainingFractionalPart = 0.0d;
> ---
> > private transient double remainingFractionalPart = 0.0d;
> 3834c3828
> < private int lastUsedIndex = 0;
> ---
> > private transient int lastUsedIndex = 0;
> 3837c3831
> < private int nbGroupsNeeded = 0;
> ---
> > private transient int nbGroupsNeeded = 0;
> 3842c3836
> < private boolean isFastPath = false;
> ---
> > private transient boolean isFastPath = false;
> 3845c3839
> < private boolean fastPathCheckNeeded = true;
> ---
> > private transient boolean fastPathCheckNeeded = true;
> 3848,3849c3842,3843
> < private char[] positiveFastPathContainer;
> < private char[] negativeFastPathContainer;
> ---
> > private transient char[] positiveFastPathContainer;
> > private transient char[] negativeFastPathContainer;
> 3852,3853c3846,3847
> < private double[] safeMinusRoundingBounds = null;
> < private double[] safeMajorRoundingBounds = null;
> ---
> > private transient double[] safeMinusRoundingBounds = null;
> > private transient double[] safeMajorRoundingBounds = null;
> 3856,3858c3850,3852
> < private boolean isSuffixPresent;
> < private char[] charsPositiveSuffix;
> < private char[] charsNegativeSuffix;
> ---
> > private transient boolean isSuffixPresent;
> > private transient char[] charsPositiveSuffix;
> > private transient char[] charsNegativeSuffix;
> 3861c3855
> < private int zeroDelta;
> ---
> > private transient int zeroDelta;
> 3864c3858
> < private char groupingChar;
> ---
> > private transient char groupingChar;
> 3867c3861
> < private char decimalSeparatorChar;
> ---
> > private transient char decimalSeparatorChar;
> 3921,3922c3915,3918
> < if (digitOne == '9') digitOne = '0';
> < else digitOne++;
> ---
> > if (digitOne == '9')
> > digitOne = '0';
> > else
> > digitOne++;
> 3927,3928c3923,3926
> < if (digitTen == '9') digitTen = '0';
> < else digitTen++;
> ---
> > if (digitTen == '9')
> > digitTen = '0';
> > else
> > digitTen++;
>
More information about the i18n-dev
mailing list