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