RFR: 8074345: Enable RewriteBytecodes when VM runs with CDS

[Yumin Qi]

Thanks, I will have another webrev after build/test/perf test.

Yumin

On 3/11/2015 1:11 PM, Coleen Phillimore wrote:
>
> 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
>>>>
>>>
>>
>