RFR: 8074345: Enable RewriteBytecodes when VM runs with CDS
Coleen Phillimore
coleen.phillimore at oracle.com
Wed Mar 25 21:45:11 UTC 2015
Yumin,
This looks great. You addressed my earlier pre-review comments.
There are two other architectures now, that I don't believe we build and
test. I think it's only cpu code that would be changed so the fixes
for these architectures won't need a sponsor.
Zero doesn't support CDS so I think the minimal changes you've put in
bytecodeInterpreter.cpp are good.
Thanks!
Coleen
On 3/25/15, 5:24 PM, Yumin Qi wrote:
> Hi, Coleen
>
> New version based on Ioi's suggestion is located at:
> http://cr.openjdk.java.net/~minqi/8074345/webrev03/
>
> Test: JPRT. Manual test on -Xshare:[dump | on ]
>
> Thanks
> Yumin
>
> On 3/25/2015 9:58 AM, Coleen Phillimore wrote:
>>
>> Yes, this was on my to-do list, sorry I haven't gotten to it yet.
>> Coleen
>>
>> On 3/25/15, 12:38 PM, Ioi Lam wrote:
>>> Hi Yumin,
>>>
>>> The changes look good. Just a few nits:
>>>
>>> *src/share/vm/interpreter/bytecodes.hpp:**
>>> *
>>> 293 // Rewritten at CDS dump time to | Original bytecode
>>> 294 // _invoke_virtual rewritten on sparc, will be disabled if
>>> UseSharedSpaces turned on.
>>> 295 // ------------------------------+------------------
>>> 296 _nofast_getfield , // <- _getfield
>>> 297 _nofast_putfield , // <- _putfield
>>> 298 _nofast_aload_0 , // <- _aload_0
>>> 299 _nofast_iload , // <- _iload
>>>
>>> I think it should be reformatted to line up the columns:
>>>
>>> 293 // Rewritten at CDS dump time to | Original bytecode
>>> 295 // ------------------------------+------------------
>>> 296 _nofast_getfield , // <- _getfield
>>> 297 _nofast_putfield , // <- _putfield
>>> 298 _nofast_aload_0 , // <- _aload_0
>>> 299 _nofast_iload , // <- _iload
>>> 230 // NOTE: _invoke_virtual is rewritten only on sparc. This
>>> will be disabled if
>>> // UseSharedSpaces turned on.
>>>
>>> *src/share/vm/interpreter/rewriter.cpp:*
>>>
>>> There are many places that modify the Method object. Instead of
>>> putting asserts at all the places where an actual modification
>>> happens, I think it's better to use only one assert at the Rewriter
>>> entry point, and remove the other assets that you added:
>>>
>>> 516 void Rewriter::rewrite(instanceKlassHandle klass, TRAPS) {
>>> + if (!DumpSharedSpaces) {
>>> + assert(!MetaspaceShared::is_in_shared_space(klass()), "archive
>>> methods must not be rewritten at run time");
>>> + }
>>> 517 ResourceMark rm(THREAD);
>>> 518 Rewriter rw(klass, klass->constants(), klass->methods(),
>>> CHECK);
>>> 519 // (That's all, folks.)
>>> 520 }
>>>
>>> Also, I am not sure if the PPC directories in the repo have been
>>> 'locked' or not, but I guess you will find out when you do the push.
>>>
>>> I am not a Reviewer, so probably Coleen needs to look at this as well.
>>>
>>> Thanks
>>> - Ioi
>>>
>>> On 3/20/15, 1:53 PM, Yumin Qi wrote:
>>>> Hi, Coleen and all
>>>>
>>>> New version with suggested changes can be reviewed at:
>>>> http://cr.openjdk.java.net/~minqi/8074345/webrev02/
>>>>
>>>> Removed _fast_invokeinvirtual from last version, disable
>>>> rewriting _invokevirtual if UseSharedSpaces turned on. Only on
>>>> sparc _invokevirtual got rewritten. Other platforms as unimplemented.
>>>>
>>>> Thanks
>>>> Yumin
>>>>
>>>> On 3/11/2015 1:23 PM, Yumin Qi wrote:
>>>>> 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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list