Request for review: 6896617: Optimize sun.nio.cs.ISO_8859_1$Encode.encodeArrayLoop() on x86

[Ulf Zibis]

Am 10.01.2013 03:11, schrieb Vladimir Kozlov:
> On 1/9/13 5:11 PM, Vitaly Davidovich wrote:
>> One could write this as:
>> boolean overflow = false;
>> if (len > (dl - dp))
>> {
>>    overflow = true;
>>     len = dl - dp;
>> }
>>
>> One would hope jit can do this automatically and also CSE away the dl -
>> dp bit. :)
>
> Yes, JIT can convert Ulf's code into above code - SplitIf optimization in C2.

Yes, this I was assuming too. But I think, the correct code would be:
Alternative (3):
     boolean overflow;
     if (len > (dl - dp)) {
         overflow = true;
         len = dl - dp;
     } else
         overflow = false;
This would avoid first assigning overflow = false and later reverting it to overflow = true;
...or is JIT able to optimize this?

> But it could be complicated by surrounding code.

Could one check this by hsdis ?

Anyway I suspect if JIT will optimise this snippet in a reasonable time, because it is only executed 
once in encodeArrayLoop(), + the compile threshold is high, as method encodeArrayLoop() is long. So 
maybe we should focus on the best performance in interpreter for the 3 alternatives.
My assumption, the following would win, as it avoids to calculate dl - dp 2 times:
Alternative (4):
     int slen = sl - sp;
     int dlen = dl - dp;
     boolean overflow;
     if (slen > dlen) {
         overflow = true;
         slen = dlen;
     } else
         overflow = false;
... but maybe Alternative (5) is not far from that:
     int slen = sl - sp;
     int dlen = dl - dp;
     boolean overflow = slen > dlen;
     slen ? overflow = dlen : slen;

In case, if JIT comes in effect, both should optimize optimal.

Looking at the calculation of slen/dlen:
     int sp = src.arrayOffset() + src.position();
     int sl = src.arrayOffset() + src.limit();
     assert (sp <= sl);
     sp = (sp <= sl ? sp : sl);
     int slen = sl - sp;
My suggestion:
     int slen = src.remaining();   // should never be negative, so above 2 time (sp <= sl) are 
superfluous!
Additionally calculation of sl could be faster:
     int sl = sp + src.remaining();
which could be again faster and is equivalent to:
     int sl = sp + slen;
if slen is anyway calculated before.

Same for dlen, with on top, that dl is never used later 8-) , so could be completely dropped.

About the naming:
According to sp, sl, dp, dl I would prefer: sr, dr over (x)len.

About overflow:
There was a question some posts above, if it is necessary at all. Can one please answer?

The question is, if other error results have higher priority over CoderResult.OVERFLOW.

-Ulf


> And generating only one dl-dp is regular optimization which JIT does.
>
> Thanks,
> Vladimir
>
>>
>> Sent from my phone
>>
>> On Jan 9, 2013 7:39 PM, "Vladimir Kozlov" <vladimir.kozlov at oracle.com
>> <mailto:vladimir.kozlov at oracle.com>> wrote:
>>
>>      From JIT compiler view current code is better since it has only one
>>     compare. Your code has two compares: one to calculate "overflow" and
>>     an other (depended on first) to calculate "len".
>>
>>     Thanks,
>>     Vladimir
>>
>>     On 1/9/13 3:57 PM, Ulf Zibis wrote:
>>
>>         Another little simplification:
>>             179             boolean overflow = sr > dr;
>>             180             sr = overflow ? dr : sr;
>>         or in your existing logic:
>>            178             int len = sl - sp;
>>            179             boolean overflow = len > (dl - dp);
>>            180             len = overflow ? dl - dp : len;
>>         (len is equivalent to sr)
>>
>>         -Ulf
>>
>>         Am 09.01.2013 19:03, schrieb Vladimir Kozlov:
>>
>>             Ulf,
>>
>>             Thank you for this suggestion but I would like to keep
>>             surrounding
>>             code intact. I will rename "overflowflag" to "overflow". It
>>             is used to
>>             indicate that we should return CoderResult.OVERFLOW result.
>>
>>             Thanks,
>>             Vladimir
>>
>>             On 1/9/13 3:58 AM, Ulf Zibis wrote:
>>
>>                 Am 09.01.2013 01:10, schrieb Vitaly Davidovich:
>>
>>                     On Jan 8, 2013 6:18 PM, "Vladimir Kozlov"
>>                     <vladimir.kozlov at oracle.com
>> <mailto:vladimir.kozlov at oracle.com>>
>>                     wrote:
>>
>> http://cr.openjdk.java.net/~**__kvn/6896617_jdk/webrev
>> <http://cr.openjdk.java.net/~**kvn/6896617_jdk/webrev><http://__cr.openjdk.java.net/~kvn/__6896617_jdk/webrev
>> <http://cr.openjdk.java.net/~kvn/6896617_jdk/webrev>>
>>
>>
>>
>>                 Another tweak:
>>                    168             char[] sa = src.array();
>>                    169             int sp = src.arrayOffset() +
>>                 src.position();
>>                    170             int sr = src.remaining();
>>                    171             int sl = sp + sr;
>>                    172             assert (sp <= sl); // superfluous, sr
>>                 is always >= 0
>>                    173             sp = (sp <= sl ? sp : sl); //
>>                 superfluous "
>>                    174             byte[] da = dst.array();
>>                    175             int dp = dst.arrayOffset() +
>>                 dst.position();
>>                    170             int dr = dst.remaining();
>>                    176             int dl = dp + dr;
>>                    177             assert (dp <= dl); // superfluous   "
>>                    178             dp = (dp <= dl ? dp : dl); //
>>                 superfluous "
>>                    179             boolean overflow = false;
>>                    180             if (sr > dr) {
>>                    181                 sr = dr;
>>                    182                 overflow = true;
>>                    183             }
>>
>>                 Why you called it "overflowflag", in that way, you could
>>                 name each
>>                 variable "myvaluevariable" or "myvaluefield" ;-)
>>
>>
>>                 -Ulf
>>
>>
>>
>