RFR: 8074345: Enable RewriteBytecodes when VM runs with CDS

[Coleen Phillimore]

Hi Yumin,  One comment embedded.

On 3/11/15, 2:04 PM, Yumin Qi wrote:
> Hi, Coleen
>
>   Thanks for the review. See embedded.
>
> On 3/10/2015 2:54 PM, Coleen Phillimore wrote:
>>
>> Yumin,
>>
>> The new bytecode approach came out pretty cleanly, or as cleanly as 
>> this could be.
>>
>> The file templateTable_x86_32 and 64 have just been merged, so you'll 
>> have to make your change in the new version.
>>
>> I also have some comments:
>>
>> http://cr.openjdk.java.net/~minqi/8074345/src/share/vm/interpreter/rewriter.cpp.udiff.html 
>>
>>
>> Typo "rewirting"
>>
> Sure.
>> In these files, can you break up the long lines into three lines?
>>
>> -  if (!is_static) { patch_bytecode(Bytecodes::_fast_fgetfield, Rbc, 
>> Rscratch); }
>> +  if (!is_static && rc == MAY_REWRITE) { 
>> patch_bytecode(Bytecodes::_fast_fgetfield, Rbc, Rscratch); }
>>
> Sure.
>>
>> http://cr.openjdk.java.net/~minqi/8074345/src/share/vm/interpreter/bytecodes.hpp.udiff.html 
>>
>>
>> How many bytecodes do we have now?  We're limited to 255 (or 256) and 
>> there are other new bytecodes being added.
>>
> now total is 234 (after the fix). See below answer.
>> What was the performance benefit to this?   I think if we wanted to 
>> be conservative, we'd turn off RewriteFrequentPairs and only do 
>> nofast_getfield and nofast_putfield.  I think they were the only 
>> bytecodes that actually affected performance.
>>
>> In this file above, can you remove the last block of comments about 
>> fast_linearswitch and fast_ldc?  I think this confuses rewriting in 
>> the interpreter and rewriting in the rewriter, or rather makes the 
>> confusion worse.  I don't think this comment is helpful.
>>
>> I'd prefer to see the first comment smaller also, like:
>>
>> +    // These bytecodes are rewritten at CDS dump time, so that we 
>> can prevent them from being
>> +    // rewritten at run time. This way, the ConstMethods can be 
>> placed in the CDS ReadOnly
>> +    // section, and RewriteByteCodes/RewriteFrequentPairs can 
>> rewrite non-CDS bytecodes
>> +    // at run time.
>> +    _nofast_getfield      ,
>> +    _nofast_putfield      ,
>> +    _nofast_aload_0       ,
>> +    _nofast_iload         ,
>> +    _nofast_invokevirtual ,
>>
>> It's sort of obvious which bytecode they rewrite.  I don't know how 
>> much performance fast_invokevfinal is worth.  I thought I deleted 
>> it.  Can we not rewrite this so we don't waste another bytecode on 
>> it?  Maybe add a RewriteVFinal option and consider removing it for 
>> the future?  x86 doesn't use it and I can't see how this would save 
>> any significant performance to be worth having!
>>
> _invokevirtual got rewritten on sparc and ppc. Now ppc is removed, no 
> need to take care for it. For sparc, it does patch code. I am thinking 
> of a way if we need to add _nofast_code as you indicated, we only have 
> 255 codes to use.

PPC isn't removed from the open repository.
>
>    bool not_rewrite = UseSharedSpaces && RewriteBytecodes && 
> RewriteFrequentPair;

I think the conditional would be

    bool not_rewrite = UseSharedSpaces || !RewriteBytecodes;
>
>    Can this boolean decide if we not rewrite the bytecode to fast? If 
> so, I can remove all the _nofast_code and do not patch code when it is 
> on.

Yes, this would be nice to not add the bytecode.

>> http://cr.openjdk.java.net/~minqi/8074345/src/share/vm/interpreter/templateTable.hpp.udiff.html 
>>
>>
>> +  enum RewriteControl { MAY_REWRITE, MAY_NOT_REWRITE };  // control 
>> for fast code under CDS
>>
>>
>> I don't know what our coding standard is but in the 
>> templateTable_<cpu>.cpp files these strings look like macros. I'd 
>> rather see them as MayRewrite or MayNotRewrite.
>>
> Agree.
>> http://cr.openjdk.java.net/~minqi/8074345/src/cpu/sparc/vm/templateTable_sparc.cpp.udiff.html 
>>
>>
>> I think there's a java_code() function that returns the original 
>> bytecode that you could use instead of the case statement in 
>> resolve_cache_and_index().   The indentation is odd in the webrev. 
>> This probably applies to the other cpu directories.
>>
>> One last question below:
>>
>> On 3/5/15, 4:21 PM, Yumin Qi wrote:
>>> Please review:
>>>
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8074345
>>> webrev: http://cr.openjdk.java.net/~minqi/8074345/
>>>
>>> Summary: Currently CDS when is disabled, RewriteBytecodes and 
>>> RewriteFrequentPairs are disabled due to ConstantMethod in CDS are 
>>> mapped read only. So memory fault will be triggered when 
>>> RewriteBytecodes turned on. This also disable all method rewritten, 
>>> leads interpreter run slower. Observed about 2% regression with C2 
>>> on some benchmarks, since interpreter speed is important to C2. By 
>>> enable RewriteBytecodes and RewriteFrequentPairs under CDS enabled, 
>>> adding _nofast_xxxx for corresponding fast codes at dump time to 
>>> avoid byte code rewritten at run time, we prevent byte code 
>>> rewritten and modify the read only shared portion in CDS. Meanwhile 
>>> other byte codes with fast codes still get speed up.
>>>
>>> Tests: JPRT, jtreg, refworkload (20+ benchmarks) on all supported 
>>> platforms.  Interpreter only tests showed about 3% improvement.
>>
>> What performance did you measure?  Is it -Xint -Xshare:on with and 
>> without your patch?  It was only 3% better?
>>
>> What was the difference in performance with -Xint 
>> -XX:-RewriteBytecodes vs. -Xint -XX:+RewriteBytecodes/FrequentPairs? 
>> I thought this was around 15%.
>>
> I will send you a separate email of the links which run with 
> CDS/NoCDS/CDS+Int

Thanks for the links.  From your experiments, I think your performance 
improvement with your patch and CDS with -Xmixed is 4%. That's good 
enough for a couple of bytecodes.

Coleen

>
> Thanks
> Yumin
>> Thanks,
>> Coleen
>>
>>>
>>> Thanks
>>> Yumin
>>>
>>
>