RFR: 8074345: Enable RewriteBytecodes when VM runs with CDS
Severin Gehwolf
sgehwolf at redhat.com
Fri Mar 27 11:16:22 UTC 2015
Hi,
On Fri, 2015-03-27 at 11:14 +0100, Thomas Stüfe wrote:
> Hi Severin,
>
> Could be a product issue. Tried a slowdebug build and my laptop
> battery just died on me but I believe the Hotspot was built completely
> before that.
Yes, reproduced. It's a release type zero build issue.
Cheers,
Severin
> .. Thomas
>
> On Mar 27, 2015 10:48 AM, "Severin Gehwolf" <sgehwolf at redhat.com>
> wrote:
> Hi,
>
> On Fri, 2015-03-27 at 09:58 +0100, Thomas Stüfe wrote:
> > Hi,
> >
> >
> > could this break zero? I try to build zero and get the
> following build
> > error:
> >
> >
> > /shared/projects/openjdk/jdk9-hs-rt/sources/hotspot/src/share/vm/interpreter/bytecodeInterpreter.cpp: In static member function ‘static void BytecodeInterpreter::run(interpreterState)’:
> > /shared/projects/openjdk/jdk9-hs-rt/sources/hotspot/src/share/vm/interpreter/bytecodeInterpreter.cpp:581:33: error: label ‘opc_nofast_getfield’ used but not defined
> > /* 0xE8 */
> >
> &&opc_invokehandle,&&opc_nofast_getfield,&&opc_nofast_putfield,
> > &&opc_nofast_aload_0,
> > ^
> > /shared/projects/openjdk/jdk9-hs-rt/sources/hotspot/src/share/vm/interpreter/bytecodeInterpreter.cpp:581:55: error: label ‘opc_nofast_putfield’ used but not defined
> > /* 0xE8 */
> >
> &&opc_invokehandle,&&opc_nofast_getfield,&&opc_nofast_putfield,
> > &&opc_nofast_aload_0,
> > ^
> > /shared/projects/openjdk/jdk9-hs-rt/sources/hotspot/src/share/vm/interpreter/bytecodeInterpreter.cpp:581:78: error: label ‘opc_nofast_aload_0’ used but not defined
> > /* 0xE8 */
> >
> &&opc_invokehandle,&&opc_nofast_getfield,&&opc_nofast_putfield,
> > &&opc_nofast_aload_0,
> >
> > ^
> > /shared/projects/openjdk/jdk9-hs-rt/sources/hotspot/src/share/vm/interpreter/bytecodeInterpreter.cpp:582:14: error: label ‘opc_nofast_iload’ used but not defined
> > /* 0xEC */ &&opc_nofast_iload,&&opc_default,
> &&opc_default,
> > &&opc_default,
> > ^
> > make[8]: *** [bytecodeInterpreter.o] Error 1
> >
> >
> >
> >
> > I build with a very unexciting configure line:
> >
> >
> > bash ../sources/configure
> --with-boot-jdk=/shared/projects/sapjvm_8
> > --with-jvm-variants=zero
> >
> >
> >
> > on Ubuntu 14.4.
> >
> >
> > Reverting back before this change makes the build go through
> (save for
> > the printf() format issue in frame_zero.cpp which Colleen
> fixed in a
> > later patch).
> >
> >
> > Am I building wrong?
>
> I have Zero (slowdebug) building fine on F21 with
> java-1.8.0-openjdk as
> boot JDK. This is revision 2206bbeb3185, so includes Coleen's
> fix and
> 8074345.
>
> $ ../build/linux-x86_64-normal-zero-slowdebug/images/jdk/bin/java -version
> openjdk version "1.9.0-internal-debug"
> OpenJDK Runtime Environment (build
> 1.9.0-internal-debug-sgehwolf_2015_03_26_10_51-b00)
> OpenJDK 64-Bit Zero VM (build
> 1.9.0-internal-debug-sgehwolf_2015_03_26_10_51-b00,
> interpreted mode)
>
> Did you do make dist-clean? Could this be product build only
> problem?
>
> Cheers,
> Severin
>
> > On Wed, Mar 25, 2015 at 10:48 PM, Yumin Qi
> <yumin.qi at oracle.com>
> > wrote:
> > Thanks for your review!
> >
> > Yumin
> >
> >
> > On 3/25/2015 2:45 PM, Coleen Phillimore wrote:
> >
> > 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