On Jan 21, 2014, at 11:05 PM, Xueming Shen <xueming.shen@oracle.com> wrote:

Hi Paul, thanks for reviewing the changeset, comment inlined below.

On 01/20/2014 09:24 AM, Paul Sandoz wrote:

...

Some quick comments.

In String.toLowerCase:

- it would be nice to get rid of the pseudo goto using the "scan" labelled block.

webrev has been updated to remove the pseudo goto by checking the "first" against "len" after the loop break.

Much for readable :-)

...

- there is no need for the "localeDependent" local variable.

I just tried to not throw away the result of this "localeDependent", which is still needed in the toXCaseEx() methods.

if (lang == "tr" || lang == "az" || lang == "lt") { // local dependent return toLowerCaseEx(result, firstUpper, locale, true); } // otherwise false is passed to subsequent calls to toLowerCaseEx ?

...

- you might be able to optimize by doing (could depend on the answer to the next point):

int c = (int)value[i]; int lc = Character.toLowerCase(c); if (.....) { result[i] = (char)lc; } else { return toLowerCaseEx(result, i, locale, localeDependent); }

- Do you need to check ERROR for the result of toLowerCase?

2586 if (c == Character.ERROR ||

Yes, Character.toLowerCase() should never return ERROR (while the package private Character.toUpperCaseEx() will). In theory there is no need to check if the return value of Character.toUpperCase(int) > min_supplementary_code_point in our loop, because there is no bmp character returns a supplementary code point as its lower case. But since it's a data driven mapping table, there is no guarantee the unicode data table is not going to change in the "future", so I still keep the check. The webrev has been updated to use one single "if" inside the loop.

I have a "single if" implementation for the toUpperCase() as well, though don't sure which one is better/faster :-)

OK, i suppose one could measure :-) Not sure how much it is worth obsessing over but... int c = (int)value[i]; if (c < '\u03A3') { result[i] = (char)Character.toLowerCase(c); // Is that safe? } else if (c < Character.MIN_HIGH_SURROGATE && c != 'u03A3' && (c = Character.toLowerCase(c)) < Character.MIN_SUPPLEMENTARY_CODE_POINT)) { result[i] = (char)c; } else { return toLowerCaseEx(result, i, locale, localeDependent); } or: int c = (int)value[i]; int lc = Character.toLowerCase(c); // is that safe? if (c < '\u03A3') { result[i] = (char)lc; } else if (c < Character.MIN_HIGH_SURROGATE && c != 'u03A3' && lc < Character.MIN_SUPPLEMENTARY_CODE_POINT)) { result[i] = (char)lc; } else { return toLowerCaseEx(result, i, locale, localeDependent); } or: int c = (int)value[i]; int lc = Character.toLowerCase(c); // is that safe? if (c < '\u03A3' || (c < Character.MIN_HIGH_SURROGATE && c != 'u03A3' && lc < Character.MIN_SUPPLEMENTARY_CODE_POINT))) { result[i] = (char)lc; } else { return toLowerCaseEx(result, i, locale, localeDependent); } FWIW i personally find those solutions easier to read, if they are safe w.r.t. Character.toLowerCase and that annoying greek character. Paul.

http://cr.openjdk.java.net/~sherman/8032012/

-Sherman

Hi, Let's try to wrap it up, otherwise I may drop the ball somewhere :-) On 01/22/2014 07:20 AM, Paul Sandoz wrote:

...

if (lang == "tr" || lang == "az" || lang == "lt") { // local dependent return toLowerCaseEx(result, firstUpper, locale, true); } // otherwise false is passed to subsequent calls to toLowerCaseEx

?

toLowerCaseEx will also be invoked later (in your another suggestion next), which needs a "localeDependent".

...

...

- you might be able to optimize by doing (could depend on the answer to the next point):

int c = (int)value[i]; int lc = Character.toLowerCase(c); if (.....) { result[i] = (char)lc; } else { return toLowerCaseEx(result, i, locale, localeDependent); }

- Do you need to check ERROR for the result of toLowerCase?

2586 if (c == Character.ERROR ||

Yes, Character.toLowerCase() should never return ERROR (while the package private Character.toUpperCaseEx() will). In theory there is no need to check if the return value of Character.toUpperCase(int)> min_supplementary_code_point in our loop, because there is no bmp character returns a supplementary code point as its lower case. But since it's a data driven mapping table, there is no guarantee the unicode data table is not going to change in the "future", so I still keep the check. The webrev has been updated to use one single "if" inside the loop.

I have a "single if" implementation for the toUpperCase() as well, though don't sure which one is better/faster :-)

OK, i suppose one could measure :-)

Not sure how much it is worth obsessing over but...

int c = (int)value[i]; if (c< '\u03A3') { result[i] = (char)Character.toLowerCase(c); // Is that safe? } else if (c< Character.MIN_HIGH_SURROGATE&& c != 'u03A3'&& (c = Character.toLowerCase(c))< Character.MIN_SUPPLEMENTARY_CODE_POINT)) { result[i] = (char)c; } else { return toLowerCaseEx(result, i, locale, localeDependent); }

or:

int c = (int)value[i]; int lc = Character.toLowerCase(c); // is that safe? if (c< '\u03A3') { result[i] = (char)lc; } else if (c< Character.MIN_HIGH_SURROGATE&& c != 'u03A3'&& lc< Character.MIN_SUPPLEMENTARY_CODE_POINT)) { result[i] = (char)lc; } else { return toLowerCaseEx(result, i, locale, localeDependent); }

or:

int c = (int)value[i]; int lc = Character.toLowerCase(c); // is that safe? if (c< '\u03A3' || (c< Character.MIN_HIGH_SURROGATE&& c != 'u03A3'&& lc< Character.MIN_SUPPLEMENTARY_CODE_POINT))) { result[i] = (char)lc; } else { return toLowerCaseEx(result, i, locale, localeDependent); }

FWIW i personally find those solutions easier to read, if they are safe w.r.t. Character.toLowerCase and that annoying greek character.

The existing code has the clear and explicit logic that (1) j.l.C.toLowerCase() can NOT handle surrogate(s) (2) j.l.C.toLowerCase() can NOT handle \u03A3 (3) if for some reasons j.l.C.toLowerCase() returns a supplementary character for a non-bmp character, the current "one-char" based result can NOT handle it, So in above cases, go to "ex" version. The suggested approach is kinda of hacky and the logic is not clear. For example it tries to optimize with c < \u03a3 cases, it works with the assumption that all lower cases of < \u03a3 characters are never a supplementary characters, which is true for now and probably will be true forever, but it actually changes the design and implementation logic (be driven by the backend unicode case mapping data, not an "assumption"). The (c< Character.MIN_HIGH_SURROGATE&& c != 'u03A3'&& lc< Character.MIN_SUPPLEMENTARY_CODE_POINT) part appears to be fine, but its logic is not as explicit as the existing one. Given the measurement indicates it does not bring in measurable improvement, personally I would prefer to keep the existing one, if you don't have a very strong opinion on this. http://cr.openjdk.java.net/~sherman/8032012/ Thanks! -Sherman

On 02/05/2014 11:09 AM, Paul Sandoz wrote:

On Feb 5, 2014, at 6:58 PM, Xueming Shen<xueming.shen@oracle.com> wrote:

...

Hi,

Let's try to wrap it up, otherwise I may drop the ball somewhere :-)

On 01/22/2014 07:20 AM, Paul Sandoz wrote:

...

if (lang == "tr" || lang == "az" || lang == "lt") { // local dependent return toLowerCaseEx(result, firstUpper, locale, true); } // otherwise false is passed to subsequent calls to toLowerCaseEx

?

toLowerCaseEx will also be invoked later (in your another suggestion next), which needs a "localeDependent".

But is not the second call to toLowerCaseEx always invoked with a value of false?

2574 String lang = locale.getLanguage(); 2575 final boolean localeDependent = (lang == "tr" || lang == "az" || lang == "lt"); 2576 if (localeDependent) { 2577 return toLowerCaseEx(result, firstUpper, locale, localeDependent); //<-- localeDependent is true 2578 } 2579 for (int i = firstUpper; i< len; i++) { 2580 int c = (int)value[i]; 2581 if (c>= Character.MIN_HIGH_SURROGATE&& c<= Character.MAX_HIGH_SURROGATE || 2582 c == '\u03A3' || // GREEK CAPITAL LETTER SIGMA 2583 (c = Character.toLowerCase(c))>= Character.MIN_SUPPLEMENTARY_CODE_POINT) { 2584 return toLowerCaseEx(result, i, locale, localeDependent); //<-- localDependent is false 2585 } 2586 result[i] = (char)c; 2587 } 2588 return new String(result, true);

You are absolutely right :-) I will update as suggested. -sherman

Am 05.02.2014 21:06, schrieb Xueming Shen:

http://cr.openjdk.java.net/~sherman/8032012/webrev

webrev has been updated accordingly.

I still more like the break-to-label approach. It looks more logical and saves one comparison. This might count on very very short strings, but I would rename it to something like find1stUpperCase: or scanForUpperCase. As an alternative you could write "if (first >= len)" as counterpart to the last "first < len" operation in the for loop, which therefore should be merged by JIT. (Or use "first != len" in the for loop.) -Ulf

On 02/05/2014 07:43 PM, Xueming Shen wrote:

On 02/05/2014 11:09 AM, Paul Sandoz wrote:

...

On Feb 5, 2014, at 6:58 PM, Xueming Shen<xueming.shen@oracle.com> wrote:

...

Hi,

Let's try to wrap it up, otherwise I may drop the ball somewhere :-)

On 01/22/2014 07:20 AM, Paul Sandoz wrote:

...

if (lang == "tr" || lang == "az" || lang == "lt") { // local dependent return toLowerCaseEx(result, firstUpper, locale, true); } // otherwise false is passed to subsequent calls to toLowerCaseEx

?

toLowerCaseEx will also be invoked later (in your another suggestion next), which needs a "localeDependent".

But is not the second call to toLowerCaseEx always invoked with a value of false?

2574 String lang = locale.getLanguage(); 2575 final boolean localeDependent = (lang == "tr" || lang == "az" || lang == "lt"); 2576 if (localeDependent) { 2577 return toLowerCaseEx(result, firstUpper, locale, localeDependent); //<-- localeDependent is true 2578 } 2579 for (int i = firstUpper; i< len; i++) { 2580 int c = (int)value[i]; 2581 if (c>= Character.MIN_HIGH_SURROGATE&& c<= Character.MAX_HIGH_SURROGATE || 2582 c == '\u03A3' || // GREEK CAPITAL LETTER SIGMA 2583 (c = Character.toLowerCase(c))>= Character.MIN_SUPPLEMENTARY_CODE_POINT) { 2584 return toLowerCaseEx(result, i, locale, localeDependent); //<-- localDependent is false 2585 } 2586 result[i] = (char)c; 2587 } 2588 return new String(result, true);

You are absolutely right :-) I will update as suggested.

-sherman

Hi Sherman, the code can be faster if the first loop call toLowerCaseEx in case of a surrogate instead of modifying srcCount because in that case the JIT will see that the increment is constant. Note that with the current code, the performance of toLowerCase if it was never called with a string that contains a surrogate are great but if toLowerCase is called once with a string that contains a surrogate, even if toLowercase is called after with strings that never contain a surrogate performance will be slower than it was. Calling toLowerCaseEx will make a toLowerCase with a surrogate slower but make toLowerCase faster in the common case. cheers, Rémi

On 02/05/2014 10:30 PM, Xueming Shen wrote:

Hi Remi,

Good suggestion. Now the "common case" path is much simple and faster :-) I'm seeing a 5%-10% boost for the normal-non-surrogates case. And it appears the bmp+surr mixed is getting faster as well. Though I would assume the it would get slower in case of "no-case-folding" + surrogates. But the common case wins here.

http://cr.openjdk.java.net/~sherman/8032012/webrev/

Thanks! -Sherman

nice, cool ! Rémi

On 02/05/2014 01:00 PM, Remi Forax wrote:

...

Hi Sherman, the code can be faster if the first loop call toLowerCaseEx in case of a surrogate instead of modifying srcCount because in that case the JIT will see that the increment is constant.

Note that with the current code, the performance of toLowerCase if it was never called with a string that contains a surrogate are great but if toLowerCase is called once with a string that contains a surrogate, even if toLowercase is called after with strings that never contain a surrogate performance will be slower than it was. Calling toLowerCaseEx will make a toLowerCase with a surrogate slower but make toLowerCase faster in the common case.

cheers, Rémi

On 02/05/2014 03:28 PM, Ulf Zibis wrote:

Additionally you could use Character.isSurrogate() and Character.isSupplementaryCode<point() at appropriate places. Both are better optimized for JIT.

j.l.C.isSupplementaryCodePoint() checks up boundary of supp, we probably don't need it here, as the returning code point is either a ERROR or a valid unicode code point. I'm not sure about the j.l.C.isSurrogate(), which takes a char and we have an int here. I would expect the javac will inline the constants for me, but I don't know whether jit can inline and then optimize away the explicit casting i2c. Not a big deal though. -sherman

-Ulf

Am 05.02.2014 22:30, schrieb Xueming Shen:

...

Hi Remi,

Good suggestion. Now the "common case" path is much simple and faster :-) I'm seeing a 5%-10% boost for the normal-non-surrogates case. And it appears the bmp+surr mixed is getting faster as well. Though I would assume the it would get slower in case of "no-case-folding" + surrogates. But the common case wins here.

http://cr.openjdk.java.net/~sherman/8032012/webrev/