RFR [9]: 8050142: Optimize java.util.Formatter

[Claes Redestad]

On 2014-09-24 20:14, Xueming Shen wrote:
> Shouldn't this one be at the same class (FormatSpecifier?) as the 
> "localizedMagnitudeExp"?
> the webrev shows it is one level up.

Nice catch!

> Btw, when we are here, maybe we can just remove the 
> FormatSpecifier.getZero(), it appears
> we have a static version already at Formatter level.

They are not identical, and in fact, I think some subtle things would 
change if localizedMagnitudeExp
isn't moved up: FormatSpecifier.getZero() check versus the locale the 
Formatter was created
with while the static method compares against Locale.US. This changes 
semantics when calling
format operations with null, which is a legal option:

Formatter f = new Formatter(Locale.X); // where Locale.X is a locale 
where zero != '0'
f.format(null, "%.0e", 1000000000.0); // would print the exponent in 
different ways

However, current behavior might actually be wrong since it's in 
contradiction to the specification,
e.g., Formatter#format(Locale l, String format, Object ... args):

      * @param  l
      *         The {@linkplain java.util.Locale locale} to apply during
      *         formatting.  If {@code l} is {@code null} then no 
localization
      *         is applied.  This does not change this object's locale 
that was
      *         set during construction.

Most code interprets l == null like this, but FormatSpecifier.getZero() 
is an exception. I think this
needs to be investigated and merits a bug of its own.

Updated localizedMagnitudeExp to reside in FormatSpecifier, leaving 
getZero alone:

http://cr.openjdk.java.net/~redestad/8050142/webrev.10/

/Claes
>
> On 09/24/2014 10:41 AM, Xueming Shen wrote:
>> Looks good.
>>
>> On 09/23/2014 02:07 PM, Claes Redestad wrote:
>>> How about:
>>>
>>>     // Specialized localization of exponents, where the source value 
>>> can only
>>>     // contain characters '0' through '9', starting at index offset, 
>>> and no
>>>     // group separators is added for any locale.
>>>     private void localizedMagnitudeExp(StringBuilder sb, char[] value,
>>>             final int offset, Locale l) {
>>>         char zero = getZero(l);
>>>
>>>         int len = value.length;
>>>         for (int j = offset; j < len; j++) {
>>>             char c = value[j];
>>>             sb.append((char) ((c - '0') + zero));
>>>         }
>>>     }
>>>
>>> Webrev including this and the fixes for Conversion.SCIENTIFIC:
>>> http://cr.openjdk.java.net/~redestad/8050142/webrev.09/
>>>
>>> /Claes
>>>
>>> On 2014-09-23 22:12, Xueming Shen wrote:
>>>> I don''t think an exponent should ever have a "dot', it always a 
>>>> signed integer. I think we can
>>>> just remove the dead code, maybe put some wording to explain why no 
>>>> group, no dot here.
>>>>
>>>> -Sherman
>>>>
>>>> On 09/23/2014 12:42 PM, Claes Redestad wrote:
>>>>> Ouch... but wait... the char[] returned from 
>>>>> sun.misc.FormattedFloatingDecimal.getExponent() can never contain 
>>>>> a '.', so we'll never find a dot here. Remove the dead code or fix 
>>>>> the logic?
>>>>>
>>>>> /Claes
>>>>>
>>>>> On 2014-09-23 21:30, Xueming Shen wrote:
>>>>>> Also the logic in the loop of localizedMagnitudeExp() does not 
>>>>>> look correct.
>>>>>> Shouldn't it be
>>>>>>
>>>>>> char c= value[j];
>>>>>> if (c == '.') {
>>>>>>    append l10n dot...
>>>>>> } else {
>>>>>> sb.append(c - '0' + zero);
>>>>>> }
>>>>>>
>>>>>> it appears the 'else" is missing? or I read it wrong?
>>>>>>
>>>>>> -Sherman
>>>>>>
>>>>>> On 09/23/2014 12:27 PM, Claes Redestad wrote:
>>>>>>> On 2014-09-23 21:14, Xueming Shen wrote:
>>>>>>>> On 09/22/2014 12:43 PM, Claes Redestad wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Sherman pointed out that there was a path that could actually 
>>>>>>>>> take a minor performance hit from this patch,
>>>>>>>>> which would be unacceptable. This version takes the minimal 
>>>>>>>>> approach to addressing this by adding back a
>>>>>>>>> method operating on a char[], simplified for the specific 
>>>>>>>>> usage case (the exponent part of a %g double formatting):
>>>>>>>>>
>>>>>>>>> http://cr.openjdk.java.net/~redestad/8050142/webrev.07/
>>>>>>>>>
>>>>>>>>> This latest patch passes using the extended test coverage of 
>>>>>>>>> java.util.Formatter I've proposed in 8058887, see
>>>>>>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-September/028849.html 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> Hi Claes,
>>>>>>>>
>>>>>>>> Shouldn't we also keep the exp as char[] as well in "c == 
>>>>>>>> Conversion.SCIENTIFIC" case?
>>>>>>>> It appears the usage of exp is the same as the one in 
>>>>>>>> "GENERAL", in which we are keeping
>>>>>>>> the simple char[] for exp.
>>>>>>>
>>>>>>> You're right, of course. I'll update the webrev.
>>>>>>>
>>>>>>> /Claes
>>>>>>
>>>>>
>>>>
>>>
>>
>