RFR: 8074345: Enable RewriteBytecodes when VM runs with CDS

[Yumin Qi]

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.

    bool not_rewrite = UseSharedSpaces && RewriteBytecodes && 
RewriteFrequentPair;

    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.

> 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
Yumin
> Thanks,
> Coleen
>
>>
>> Thanks
>> Yumin
>>
>