DecimalFormat rewrite, Java

DecimalFormat rewrite, Java

[sffc, 2017-01-21 11:13:25]

See the email entitled "Initial DecimalFormat Code Review Ready" for
instructions on how I recommend going about this code review.  Thanks!

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/.sett...
File icu4j/main/classes/core/.settings/org.eclipse.jdt.core.prefs (right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/.sett...
icu4j/main/classes/core/.settings/org.eclipse.jdt.core.prefs:1:
eclipse.preferences.version=1
****

You can ignore this file.  They are local changes to my workspace preferences. 
It got caught by rietveld-upload but these changes aren't being committed to
SVN.

****

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/.sett...
File icu4j/main/classes/core/.settings/org.eclipse.jdt.ui.prefs (right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/.sett...
icu4j/main/classes/core/.settings/org.eclipse.jdt.ui.prefs:1:
eclipse.preferences.version=1
****

You can ignore this file.  They are local changes to my workspace preferences. 
It got caught by rietveld-upload but these changes aren't being committed to
SVN.

****

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
File
icu4j/main/classes/core/src/com/ibm/icu/impl/number/DoubleSidedStringBuilder.java
(right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/DoubleSidedStringBuilder.java:5:
public class DoubleSidedStringBuilder implements CharSequence {
This is a version of StringBuilder optimized for my number formatting pipeline. 
It supports fast prepend as well as fast append operations.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
File icu4j/main/classes/core/src/com/ibm/icu/impl/number/Endpoint.java (right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Endpoint.java:76: public
static SingularFormat fromBTA(Properties properties, DecimalFormatSymbols
symbols)
This is the first of the two main entrypoints into my code.  This entrypoint
will construct a formatter object that can be used over and over again.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Endpoint.java:163:
FormatQuantity input, Properties properties, DecimalFormatSymbols symbols)
This is the other entrypoint into my code.  It tries to avoid object creations. 
It is not completely finished yet and still falls back to creating normal
instances for most of the pipeline pieces.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Endpoint.java:211: private
static final ThreadLocal<Map<ULocale, DecimalFormatSymbols>>
threadLocalSymbolsCache =
You'll see a lot of ThreadLocal stuff in my code.  I like ThreadLocals a lot
because they give you a way to share objects without the overhead of obtaining a
lock and potentially blocking more than one thread.

In this case, I'm making a thread-local cache of DecimalFormatSymbols objects. 
Perhaps this could use a global cache instead; I will need to set up the right
benchmarks.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Endpoint.java:239: private
static final ThreadLocal<Map<String, Properties>> threadLocalPropertiesCache =
This is new: a cache from pattern strings to property bags.  So if you try
making a formatter with the same pattern strings multiple times, we can save the
step of parsing the pattern string.  We can do this here since we are treating
the property bags as immutable once they are created for the first time.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
File icu4j/main/classes/core/src/com/ibm/icu/impl/number/Format.java (right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Format.java:14: protected
static final ThreadLocal<DoubleSidedStringBuilder> threadLocalStringBuilder =
A thread-local string builder and modifier holder, to reduce object creations. 
The same string builder can be shared not only by the same formatter, but by all
formatters on the same thread.  In single-threaded environments like Android,
this virtually eliminates object creations.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Format.java:53: public void
format(FormatQuantity input, StringBuffer output) {
I added this StringBuffer API for compatibility with the old DecimalFormat. 
Note that StringBuffer has been discouraged in Java for a long time in favor of
StringBuilder.  (StringBuffer is slow because it is thread-safe.)

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Format.java:84: public
static class BeforeTargetAfterFormat extends SingularFormat {
This is the main formatter frontend.  Instances of BeforeTargetAfterFormat are
constructed in Endpoint.java.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Format.java:192: public
static class PositiveNegativeRounderTargetFormat extends SingularFormat {
This class is a special case of BeforeTargetAfterFormat, but slightly more
efficient (~10 ns).  It has less polymorphism and fewer internal properties to
iterate through when formatting.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
File icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java
(right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java:329: //
 public boolean hasNextFraction() {
Old methods that I should remove.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java:432: //
 public byte nextInteger() {
Old method that I should remove.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java:529:
private void readDoubleToBcd(double n) {
This here is the method that Andy won't feel comfortable with.  It's the one
that uses heuristics to convert from a double to decimal digits.  I'm sure there
are edge cases that will make this method fail, but we're talking about a huge
speedup here, 30-40% overall improvement on small numbers by avoiding the cost
of calling Double.toString().

When I get back from my trip, I'll investigate more into how to efficiently
detect when the heuristics fail, and fall back to Double.toString() in those
cases.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java:546:
int exponent = (int) ((ieeeBits & 0x7ff0000000000000L) >> 52) - 0x3ff;
exponent is the power of 2 to which the mantissa is to be raised.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java:547:
int fracLength = (int) ((52 - exponent) / 3.32192809489);
3.32192809489 is log2(10)

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java:559:
readLongToBcd(Math.round(n));
At this point, n has approximately 15 digits to the left of the decimal point. 
We call Math.round() to get rid of the out-of-range trailing digits, and then
let the long-to-bcd algorithm take care of the rest.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java:729: //
 private byte popDigit(int position) {
This is an old method that I don't need any more.  I should delete it.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
File icu4j/main/classes/core/src/com/ibm/icu/impl/number/Parse.java (right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Parse.java:475: String
input, ParsePosition ppos, IProperties properties, DecimalFormatSymbols symbols)
START HERE: This is the main entrypoint to the parser.  I should probably move
this method to the top.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
File icu4j/main/classes/core/src/com/ibm/icu/impl/number/Properties.java
(right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Properties.java:42: 
A bag of properties.  One simple, unified interface.  No bells and whistles.  No
distractions.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Properties.java:129: //
There are so many properties that the likelihood of this method becoming stale
is high.
I should probably just implement the clone method by manually copying over all
of the 30-40 properties, one by one.  Code  reviews should help prevent it from
becoming stale.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Properties.java:285: * @see
com.ibm.icu.impl.number.Parse.IProperties#getParseIgnoreExponent()
I don't know why these messages showed up for some but not others.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Properties.java:579: for
(Field field : fields) {
The string is built by looking for all of the properties in this bag that are
different from the default.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/tests/core/src/com...
File
icu4j/main/tests/core/src/com/ibm/icu/dev/data/numberformattestspecification.txt
(right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/tests/core/src/com...
icu4j/main/tests/core/src/com/ibm/icu/dev/data/numberformattestspecification.txt:21:
-6	\u061C-\u0666	JK
The change in character is due to a data change.

"J" means ICU4J 58 DecimalFormat, "K" means JDK DecimalFormat, "C" means ICU4C
58 DecimalFormat, and "S" means my new DecimalFormat.  If the letter is in the
"breaks" column, it means that the corresponding implementation does not pass
that test.  Note that the test passes S and C but not J or K.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/tests/core/src/com...
icu4j/main/tests/core/src/com/ibm/icu/dev/data/numberformattestspecification.txt:243:
0	0	0	0	2.99792458E8	KS
This is the first of a handful of things that my implementation does differently
than the existing implementation(s).  I'll write these all up in a doc.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/tests/core/src/com...
icu4j/main/tests/core/src/com/ibm/icu/dev/data/numberformattestspecification.txt:566:
**####0,000	**#,##0,000	KS
In all of the "toPattern" tests cases that S is "failing", it is because S
produces a pattern that is functionally equivalent to the expected pattern.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/tests/core/src/com...
icu4j/main/tests/core/src/com/ibm/icu/dev/data/numberformattestspecification.txt:630:
// S doesn't accept a grouping separator without any leading digits
Another difference in behavior that we can discuss.  This is responsible for the
next 7 "failures".

https://codereview.appspot.com/315420043/diff/1/icu4j/main/tests/core/src/com...
icu4j/main/tests/core/src/com/ibm/icu/dev/data/numberformattestspecification.txt:696:
// S adopts JDK's behavior here
Another behavior difference.  We can discuss.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/tests/core/src/com...
icu4j/main/tests/core/src/com/ibm/icu/dev/data/numberformattestspecification.txt:794:
// S is the only implementation that passes these cases.
This is my new parser at its best.  All of the other parsers fail because they
are greedy and consume the 9 as a digit. :)

https://codereview.appspot.com/315420043/diff/1/icu4j/main/tests/core/src/com...
icu4j/main/tests/core/src/com/ibm/icu/dev/data/numberformattestspecification.txt:819:
// J and K stop parsing at the apostrophe
I was surprised when I wrote these cases and found that J and K weren't passing,
but you can try it for yourself!  (Note: I haven't run this data file against C
so I don't know how C's behavior compares.)

[andy.heninger, 2017-01-27 05:50:55]

The code, so far, makes sense. I've been stepping through formatting ops in the
debugger, to better understand the flow. Not a lot of specific comments yet.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
File
icu4j/main/classes/core/src/com/ibm/icu/impl/number/DoubleSidedStringBuilder.java
(right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/DoubleSidedStringBuilder.java:64:
public int insert(int index, CharSequence sequence) {
Tracing some formatting, this is getting called with empty sequences. Churns a
fair amount doing nothing.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
File icu4j/main/classes/core/src/com/ibm/icu/impl/number/Format.java (right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Format.java:224: if (mod !=
null) {
Formatting 10.0, at this point have a non-null mod, with both  prefix & suffix
empty. Possibly optimizable.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
File icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity.java
(right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity.java:18: *
<p>Implementations of this interface are free to use any internal storage
mechanism.
It might be useful to note the implementing classes. How many do you ultimately
expect to have?

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
File icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java
(right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java:21: *
The BCD of the 16 digits of the number represented by this object. Every 4 bits
of the long map
How are numbers with more than 16 digits handled?

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java:483:
int i = 16;
Number of digits can be up to 19 for a 64 bit int.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java:529:
private void readDoubleToBcd(double n) {
On 2017/01/21 11:13:24, sffc wrote:
> This here is the method that Andy won't feel comfortable with.  It's the one
> that uses heuristics to convert from a double to decimal digits.  I'm sure
there
> are edge cases that will make this method fail, but we're talking about a huge
> speedup here, 30-40% overall improvement on small numbers by avoiding the cost
> of calling Double.toString().
> 
> When I get back from my trip, I'll investigate more into how to efficiently
> detect when the heuristics fail, and fall back to Double.toString() in those
> cases.

You are right, I'm not comfortable with this conversion.

Guaranteed safe: The number is an integer and will fit in a long. Just assign
and proceed with the long.

Probably safe enough, although some more analysis is worth doing. Covers a lot
of frequently occurring cases:
The number * 1000 is an integer.
Multiply it, then treat as a scaled int.

(What should happen we format 0.1 with enough digits to show that the double
value was not exactly 0.1? As long as we are reworking things, we should make it
do the right thing, whatever that is).

Anything that we don't expect to be common should go to Double.toString(). Give
someone else the hard problems.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java:559:
readLongToBcd(Math.round(n));
On 2017/01/21 11:13:24, sffc wrote:
> At this point, n has approximately 15 digits to the left of the decimal point.

> We call Math.round() to get rid of the out-of-range trailing digits, and then
> let the long-to-bcd algorithm take care of the rest.

It would be faster to screen for integer values first, avoids the multiplies in
this functions, and then avoids a bunch (for small numbers) of divides and mods
in readLongToBcd().

[andy.heninger, 2017-02-01 23:55:17]

Comments pending from last week.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
File icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java
(right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java:34: *
digits after the decimal place, which is the negative of our definition of
scale.
Is there any reason for scale to be different from BidDecimal? Being similar
might be a convenience for people who are already used to it. Also,  there will
need to be a FormatQuantity class that handles a BigDecimal. Also, thinking of
the eventual C++ port, the C decNumber library conventions could be worth a
look.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java:582:
for (; n.signum() != 0; i--) {
I suspect it might be faster to just do toString() on the bigInteger, and
iterate the digits.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
File icu4j/main/classes/core/src/com/ibm/icu/impl/number/Properties.java
(right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Properties.java:131:
Properties copy = new Properties();
It looks like your clone does the same thing as Object.clone(), a shallow copy
of all fields. So you could just implement Cloneable and return super.clone();

Are all of the properties either primitive types or immutable?

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
File icu4j/main/classes/core/src/com/ibm/icu/impl/number/Rounder.java (right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Rounder.java:21: * "999" as
"999".)
Why is this?

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Rounder.java:146: * Sets the
maximium number of significant digits to display. The number of significant
digits
 sp maximium => maximum

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Rounder.java:154: * <p>If
both maximum significant digits and maximum integer/fraction digits are set at
the same
I think we should define some behavior, to avoid user surprise when the
undefined behavior changes. Any reason not to just apply both?

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
File icu4j/main/classes/core/src/com/ibm/icu/text/DecimalFormat.java (right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/text/DecimalFormat.java:91:
formatter.format(new FormatQuantity2(number), result);
Could you sqeeze out another allocation by having DecimalFormat keep a thread
local FormatQuantity, and reuse it?

[sffc, 2017-02-17 22:42:49]

Responding to Andy's initial feedback.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
File
icu4j/main/classes/core/src/com/ibm/icu/impl/number/DoubleSidedStringBuilder.java
(right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/DoubleSidedStringBuilder.java:64:
public int insert(int index, CharSequence sequence) {
On 2017/01/27 05:50:55, andy.heninger wrote:
> Tracing some formatting, this is getting called with empty sequences. Churns a
> fair amount doing nothing.

I added a check for if (sequence.length() == 0).

Note that this file has been renamed NumberStringBuilder.java instead of
DoubleSidedStringBuilder.java.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
File icu4j/main/classes/core/src/com/ibm/icu/impl/number/Format.java (right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Format.java:224: if (mod !=
null) {
On 2017/01/27 05:50:55, andy.heninger wrote:
> Formatting 10.0, at this point have a non-null mod, with both  prefix & suffix
> empty. Possibly optimizable.

Acknowledged. I added a TODO to the code in ConstantAffixModifier.java.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
File icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity.java
(right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity.java:18: *
<p>Implementations of this interface are free to use any internal storage
mechanism.
On 2017/01/27 05:50:55, andy.heninger wrote:
> It might be useful to note the implementing classes. How many do you
ultimately
> expect to have?

In practice, although I've written four different implementations, there is only
one that is actually being used, FormatQuantity4, which has the fast long-based
BCD storage mechanism with an internal fallback to a byte array.  However, I
still like the idea of having an interface.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
File icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java
(right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java:21: *
The BCD of the 16 digits of the number represented by this object. Every 4 bits
of the long map
On 2017/01/27 05:50:55, andy.heninger wrote:
> How are numbers with more than 16 digits handled?

I added FormatQuantity4, which is used by default and supports more than 16
digits.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java:34: *
digits after the decimal place, which is the negative of our definition of
scale.
On 2017/02/01 23:55:17, andy.heninger wrote:
> Is there any reason for scale to be different from BidDecimal? Being similar
> might be a convenience for people who are already used to it. Also,  there
will
> need to be a FormatQuantity class that handles a BigDecimal. Also, thinking of
> the eventual C++ port, the C decNumber library conventions could be worth a
> look.

I would have made the definitions of "scale" the same had I noticed it sooner. 
It was only after I had already written most of the code that I noticed that
they were defined to be the negative of each other.  Personally I prefer my
definition of scale because it corresponds directly with powers of ten.  I can
change it though if you feel strongly.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java:483:
int i = 16;
On 2017/01/27 05:50:55, andy.heninger wrote:
> Number of digits can be up to 19 for a 64 bit int.

FormatQuantity4 falls back to a byte array here.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java:529:
private void readDoubleToBcd(double n) {
On 2017/01/27 05:50:55, andy.heninger wrote:
> On 2017/01/21 11:13:24, sffc wrote:
> > This here is the method that Andy won't feel comfortable with.  It's the one
> > that uses heuristics to convert from a double to decimal digits.  I'm sure
> there
> > are edge cases that will make this method fail, but we're talking about a
huge
> > speedup here, 30-40% overall improvement on small numbers by avoiding the
cost
> > of calling Double.toString().
> > 
> > When I get back from my trip, I'll investigate more into how to efficiently
> > detect when the heuristics fail, and fall back to Double.toString() in those
> > cases.
> 
> You are right, I'm not comfortable with this conversion.
> 
> Guaranteed safe: The number is an integer and will fit in a long. Just assign
> and proceed with the long.
> 
> Probably safe enough, although some more analysis is worth doing. Covers a lot
> of frequently occurring cases:
> The number * 1000 is an integer.
> Multiply it, then treat as a scaled int.
> 
> (What should happen we format 0.1 with enough digits to show that the double
> value was not exactly 0.1? As long as we are reworking things, we should make
it
> do the right thing, whatever that is).
> 
> Anything that we don't expect to be common should go to Double.toString().
Give
> someone else the hard problems.

Discussed offline and started a discussion with Florian.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java:559:
readLongToBcd(Math.round(n));
On 2017/01/27 05:50:55, andy.heninger wrote:
> On 2017/01/21 11:13:24, sffc wrote:
> > At this point, n has approximately 15 digits to the left of the decimal
point.
> 
> > We call Math.round() to get rid of the out-of-range trailing digits, and
then
> > let the long-to-bcd algorithm take care of the rest.
> 
> It would be faster to screen for integer values first, avoids the multiplies
in
> this functions, and then avoids a bunch (for small numbers) of divides and
mods
> in readLongToBcd().

Acknowledged.  This function is likely to be overhauled after consulting with
Florian.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/FormatQuantity2.java:582:
for (; n.signum() != 0; i--) {
On 2017/02/01 23:55:17, andy.heninger wrote:
> I suspect it might be faster to just do toString() on the bigInteger, and
> iterate the digits.

I used a toString() trick in the BigInteger output method.  However, I'm not
keen on using that trick here, because it takes a lot of code to parse a
BigInteger string, and BigIntegers are expected to be slow anyway.

Also, FormatQuantity4 converts BigIntegers to longs whenever possible.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
File icu4j/main/classes/core/src/com/ibm/icu/impl/number/Properties.java
(right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Properties.java:131:
Properties copy = new Properties();
On 2017/02/01 23:55:17, andy.heninger wrote:
> It looks like your clone does the same thing as Object.clone(), a shallow copy
> of all fields. So you could just implement Cloneable and return super.clone();
> 
> Are all of the properties either primitive types or immutable?

Yes, actually Object.clone() does exactly what I want in this case.

Right now, the individual properties are *not* required to be immutable.  The
property bag takes everything by reference and doesn't perform any copies or
clones.  This is by design.  It is the formatter's job to perform such
copies/clones during its own construction process.

However, I have been thinking about this a bit over the last few weeks, and I'm
thinking now that it might be beneficial if we required that the properties be
immutable.  This is something we can discuss over the next few months as we
settle on the new public API.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
File icu4j/main/classes/core/src/com/ibm/icu/impl/number/Rounder.java (right):

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Rounder.java:21: * "999" as
"999".)
On 2017/02/01 23:55:17, andy.heninger wrote:
> Why is this?

Consider the case when you are handed the number 999.  If you round it to 1000,
then CompactDecimalFormat will say that the number should instead be divided by
10^3 first.  But then, if the rounder divides the number by 10^3 and returns
0.999 instead of 1.0, that's also an incorrect result from
CompactDecimalFormat's point of view.  By requiring that the number of digits
won't increase as you decrease the scale, this problem is solved.

We can discuss whether there is a good way to relax this precondition.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Rounder.java:146: * Sets the
maximium number of significant digits to display. The number of significant
digits
On 2017/02/01 23:55:17, andy.heninger wrote:
>  sp maximium => maximum

Done.

https://codereview.appspot.com/315420043/diff/1/icu4j/main/classes/core/src/c...
icu4j/main/classes/core/src/com/ibm/icu/impl/number/Rounder.java:154: * <p>If
both maximum significant digits and maximum integer/fraction digits are set at
the same
On 2017/02/01 23:55:17, andy.heninger wrote:
> I think we should define some behavior, to avoid user surprise when the
> undefined behavior changes. Any reason not to just apply both?

I added a new setting, "significant digits override", that lets you define what
the behavior should be.

[sffc, 2017-03-14 02:19:40]

r39789: All tests are passing except for backwards-compatible serialization.

[sffc, 2017-03-15 11:06:57]

r39817: Documentation, name changes, technical previews, and more tests.

[sffc, 2017-03-20 21:51:32]

r39857: Last commit before merge to trunk.