ticket:13177 NumberFormat2 from Shane's branch

ticket:13177 NumberFormat2 from Shane's branch

[andy.heninger, 2017-08-15 21:00:04]

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
File main/classes/core/src/newapi/NumberFormatter.java (right):

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/NumberFormatter.java:3: package newapi;
What is the ultimate package? com.ibm.icu.text, along with all the other
formatting APIs?

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/NumberFormatter.java:43: public final class
NumberFormatter {
Thinking about how this will look to users who aren't sure of there way around.
Probably need to get the docs in place to see how it will really look. There's a
lot of class structure to understand.

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/NumberFormatter.java:44: 
Can we move to standard 4 space indents at this time?

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/NumberFormatter.java:77: throw new
AssertionError("See NumberFormatterImpl");
Could these functions be abstract instead?

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/NumberFormatter.java:217: public static class
Internal extends LocalizedNumberFormatter {}
It'd be nice not to need these Internal classes. Maybe just leave the
constructor public, and document it as being Internal?

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
File main/classes/core/src/newapi/impl/MacroProps.java (right):

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/MacroProps.java:60: return Objects.hash(
Objects is Java 7.

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
File main/classes/core/src/newapi/impl/MicroProps.java (right):

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/MicroProps.java:43: if (frozen) {
Check who freezes, can there be races?

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/MicroProps.java:55: throw new
AssertionError();
Could throw new AssertionError(e);
to give more info should it happen.

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
File main/classes/core/src/newapi/impl/MurkyModifier.java (right):

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/MurkyModifier.java:30: final boolean isStrong;
What does 'strong' mean?

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
File main/classes/core/src/newapi/impl/NumberFormatterImpl.java (right):

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/NumberFormatterImpl.java:80: volatile
AtomicInteger callCount;
change to
  volatile long callCount;
  static final AtomicLongFieldUpdater<NumberFormatterImpl> callCountUpdater =
          AtomicLongFieldUpdater.newUpdater(NumberFormatterImpl.class,
"callCount");

Also change usage, noted below.

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/NumberFormatterImpl.java:206: if
(Objects.equals(resolve().unit, unit)) {
Objects is Java 7

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/NumberFormatterImpl.java:224: }
This lazy init is not safe, racing threads can create duplicate AtomicIntegers.
Using an AtomicLongFieldUpdater is better, and also eliminates the object
creation in this path.
Change callCount to long to head off possibility of int overflow.

Delete four lines above, change line below to
    long currentCount = callCountUpdater.incrementAndGet(this);

See other comment, above.

Web research, performance of using AtomicFieldUpdater should be similar to that
of AtomicInteger. Creation is expensive, but it's one time only, a static final.

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/NumberFormatterImpl.java:227: if (currentCount
== threshold) {
This can be called with varying threshold values. Should the check for (compiled
!= null) come first, in case some earlier call with a smaller threshold already
caused the compile to happen?

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
File main/classes/core/src/newapi/impl/Worker1.java (right):

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/Worker1.java:33: public class Worker1 {
Wants a more descriptive name, I think. Not sure what, but something that
implies this class's central role.

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/Worker1.java:66: Rounding defaultRounding =
Rounding.NONE;
Slight name confusion, NONE is the same as infinity.

Performance problem with defaults, most non-integer doubles end up taking the
slow path. Would be fine if callers had to explicitly ask for all digits, but
NumberFormatter.withLocale(whatever).format(0.1) gets the slow path.

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/Worker1.java:143: // TODO: Make sure this is
thread safe.
Looks suspicious to me. multiplier comes straight out of the Macro props. The
chain link, the rest is constant.

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/Worker1.java:179: MurkyModifier murkyMod = new
MurkyModifier(false);
Yikes, names. including what does 'false' mean, when out of context.

[sffc, 2017-08-17 04:24:36]

Thanks for the review! I addressed some of the comments. I'll get to the rest
soon.

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
File main/classes/core/src/newapi/NumberFormatter.java (right):

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/NumberFormatter.java:3: package newapi;
On 2017/08/15 21:00:03, andy.heninger wrote:
> What is the ultimate package? com.ibm.icu.text, along with all the other
> formatting APIs?

I was planning on com.ibm.icu.text, along with all the other formatting APIs,
unless we want to declare a whole new package like com.ibm.icu.number (which
could also simplify the code by eliminating the need for the duplicated classes
in com.ibm.icu.impl).

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
File main/classes/core/src/newapi/impl/MicroProps.java (right):

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/MicroProps.java:43: if (frozen) {
On 2017/08/15 21:00:03, andy.heninger wrote:
> Check who freezes, can there be races?

There shouldn't be any races.  I moved the "freeze" to the constructor to make
it more clear.

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/MicroProps.java:55: throw new
AssertionError();
On 2017/08/15 21:00:03, andy.heninger wrote:
> Could throw new AssertionError(e);
> to give more info should it happen.

Done.

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
File main/classes/core/src/newapi/impl/MurkyModifier.java (right):

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/MurkyModifier.java:30: final boolean isStrong;
On 2017/08/15 21:00:03, andy.heninger wrote:
> What does 'strong' mean?

See the documentation in the Modifier.java interface.  If a modifier is *not*
strong, it can be bubbled up and handled by the future range format.  For
example, scientific notation is strong, but not compact notation.

3E3-5E3
3-5K

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
File main/classes/core/src/newapi/impl/NumberFormatterImpl.java (right):

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/NumberFormatterImpl.java:80: volatile
AtomicInteger callCount;
On 2017/08/15 21:00:03, andy.heninger wrote:
> change to
>   volatile long callCount;
>   static final AtomicLongFieldUpdater<NumberFormatterImpl> callCountUpdater =
>           AtomicLongFieldUpdater.newUpdater(NumberFormatterImpl.class,
> "callCount");
> 
> Also change usage, noted below.

Done.  Thanks!

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/NumberFormatterImpl.java:224: }
On 2017/08/15 21:00:03, andy.heninger wrote:
> This lazy init is not safe, racing threads can create duplicate
AtomicIntegers.
> Using an AtomicLongFieldUpdater is better, and also eliminates the object
> creation in this path.
> Change callCount to long to head off possibility of int overflow.
> 
> Delete four lines above, change line below to
>     long currentCount = callCountUpdater.incrementAndGet(this);
> 
> See other comment, above.
> 
> Web research, performance of using AtomicFieldUpdater should be similar to
that
> of AtomicInteger. Creation is expensive, but it's one time only, a static
final.

Done.

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/NumberFormatterImpl.java:227: if (currentCount
== threshold) {
On 2017/08/15 21:00:03, andy.heninger wrote:
> This can be called with varying threshold values. Should the check for
(compiled
> != null) come first, in case some earlier call with a smaller threshold
already
> caused the compile to happen?

The threshold never changes. I re-wrote the code to save it in the macros
instead of being passed as an argument, which I believe is more readable.

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
File main/classes/core/src/newapi/impl/Worker1.java (right):

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/Worker1.java:143: // TODO: Make sure this is
thread safe.
On 2017/08/15 21:00:04, andy.heninger wrote:
> Looks suspicious to me. multiplier comes straight out of the Macro props. The
> chain link, the rest is constant.

I went ahead and modified the QuantityChain implementations to all enforce
safety with regard to the quantity chain (storing the parent in a final field). 
This should make thread safety more self-apparent, but it makes the code a
little bit less readable.

https://codereview.appspot.com/329960043/diff/1/main/classes/core/src/newapi/...
main/classes/core/src/newapi/impl/Worker1.java:179: MurkyModifier murkyMod = new
MurkyModifier(false);
On 2017/08/15 21:00:04, andy.heninger wrote:
> Yikes, names. including what does 'false' mean, when out of context.

Acknowledged. Added a comment.