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++;
