[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-dev
Subject: Re: RFR (XL): 8152664 - Support non-continuous CodeBlobs in HotSpot
From: Rickard =?iso-8859-1?Q?B=E4ckman?= <rickard.backman () oracle ! com>
Date: 2016-04-26 8:11:05
Message-ID: 20160426081105.GM19871 () rbackman
[Download RAW message or body]
Volker,
thanks for the review and the patch to rebase the change.
/R
On 04/25, Volker Simonis wrote:
> Hi Rickard,
>
> sorry for being late but I was at a conference last week.
> Unfortunately your patch doesn't apply cleanly to hs-comp any more. As this
> is probably my fault because I didn't send you my OK in time I resolved the
> conflicts :)
>
> The first seven can be ignored because they only touch the copyright year
> which has already been updated to 2016 now.
>
> Following are the changes needed to fix relocInfo.hpp, method.hpp and
> frame.cpp after you've applied your current patch (also attached in case
> the patch gets scrambled):
>
> diff -r a1d6c22335bb src/share/vm/code/relocInfo.hpp
> --- a/src/share/vm/code/relocInfo.hpp Mon Apr 25 14:30:01 2016 +0200
> +++ b/src/share/vm/code/relocInfo.hpp Mon Apr 25 15:12:16 2016 +0200
> @@ -28,6 +28,8 @@
> #include "memory/allocation.hpp"
> #include "runtime/os.hpp"
>
> +class nmethod;
> +class CompiledMethod;
> class Metadata;
> class NativeMovConstReg;
>
> diff -r a1d6c22335bb src/share/vm/oops/method.hpp
> --- a/src/share/vm/oops/method.hpp Mon Apr 25 14:30:01 2016 +0200
> +++ b/src/share/vm/oops/method.hpp Mon Apr 25 15:12:16 2016 +0200
> @@ -432,9 +432,9 @@
> // nmethod/verified compiler entry
> address verified_code_entry();
> bool check_code() const; // Not inline to avoid circular ref
> - nmethod* volatile code() const { assert( check_code(),
> "" ); return (nmethod *)OrderAccess::load_ptr_acquire(&_code); }
> + CompiledMethod* volatile code() const { assert(
> check_code(), "" ); return (CompiledMethod
> *)OrderAccess::load_ptr_acquire(&_code); }
> void clear_code(); // Clear out any compiled code
> - static void set_code(methodHandle mh, nmethod* code);
> + static void set_code(methodHandle mh, CompiledMethod* code);
> void set_adapter_entry(AdapterHandlerEntry* adapter) {
> constMethod()->set_adapter_entry(adapter);
> }
> diff -r a1d6c22335bb src/share/vm/runtime/frame.cpp
> --- a/src/share/vm/runtime/frame.cpp Mon Apr 25 14:30:01 2016 +0200
> +++ b/src/share/vm/runtime/frame.cpp Mon Apr 25 15:12:16 2016 +0200
> @@ -661,13 +661,16 @@
> }
> } else if (_cb->is_buffer_blob()) {
> st->print("v ~BufferBlob::%s", ((BufferBlob *)_cb)->name());
> - } else if (_cb->is_nmethod()) {
> - nmethod* nm = (nmethod*)_cb;
> - Method* m = nm->method();
> + } else if (_cb->is_compiled()) {
> + CompiledMethod* cm = (CompiledMethod*)_cb;
> + Method* m = cm->method();
> if (m != NULL) {
> - st->print("J %d%s", nm->compile_id(), (nm->is_osr_method() ? "%" :
> ""));
> - if (nm->compiler() != NULL) {
> - st->print(" %s", nm->compiler()->name());
> + if (cm->is_nmethod()) {
> + nmethod* nm = cm->as_nmethod();
> + st->print("J %d%s", nm->compile_id(), (nm->is_osr_method() ? "%"
> > ""));
> + if (nm->compiler() != NULL) {
> + st->print(" %s", nm->compiler()->name());
> + }
> }
> m->name_and_sig_as_C_string(buf, buflen);
> st->print(" %s", buf);
>
> Besides this, the change looks good.
>
> Thanks,
> Volker
>
>
> On Mon, Apr 25, 2016 at 2:15 PM, Rickard Bäckman <rickard.backman@oracle.com
> > wrote:
>
> > Volker,
> >
> > are you ok with the last changes?
> >
> > http://cr.openjdk.java.net/~rbackman/8152664.4/
> >
> > Thanks
> >
> > On 04/19, Volker Simonis wrote:
> > > On Tue, Apr 19, 2016 at 6:49 PM, Christian Thalinger <
> > > christian.thalinger@oracle.com> wrote:
> > >
> > > >
> > > > On Apr 19, 2016, at 4:30 AM, Volker Simonis <volker.simonis@gmail.com>
> > > > wrote:
> > > >
> > > > Hi Rickard,
> > > >
> > > > I just wanted to prepare the new webrev for 8151956 but I'm a little
> > > > confused because I realized that your latest webrev already contains
> > the
> > > > changes which I had proposed for 8151956.
> > > >
> > > > But after thinking about it a little bit I think that's fine. If I
> > prepare
> > > > a patch for 8151956 which is intended to be pushed BEFORE 8152664
> > you'd had
> > > > to adapt 8152664 to take care of the new changes introduced by
> > 8151956. If
> > > > I prepare a patch for 8151956 which is intended to be pushed AFTER
> > 8152664
> > > > it would be hard to review it (because it will depend on 8152664) and
> > we
> > > > would get a change in the repo which would not build on PPC64 and
> > AARCH64
> > > > which isn't nice either.
> > > >
> > > > So altogether I think it's fine to incorporate the fix for 8151956 into
> > > > your change. Please only don't forget to close 8151956 as "fixed by
> > > > 8152664" after you have pushed the changes for 8152664.
> > > >
> > > > I've verified that your last webrev builds and runs fine on
> > Linux/ppc64 and
> > > > AIX. You've also fixed all the issues I've addressed in my first mail
> > to
> > > > this thread and the typo in os_linux_aarch64.cpp found by Andrew -
> > thanks!
> > > >
> > > > Some final nit-picking:
> > > >
> > > > - you still have the white-space only change in os_windows.cpp
> > objected by
> > > > Vladimir.
> > > >
> > > > - in codeBlob.cpp can you please update the following comments to
> > reflect
> > > > the new types:
> > > >
> > > > // Creates a simple CodeBlob. Sets up the size of the different
> > > > regions.* CodeBlob::CodeBlob(const char* name, int header_size, int
> > > > size, int frame_complete, int locs_size) {** assert(size ==
> > > > round_to(size, oopSize), "unaligned size");**+
> > > > RuntimeBlob::RuntimeBlob(const char* name, int header_size, int size,
> > > > int frame_complete, int locs_size)*
> > > >
> > > > // Creates a CodeBlob from a CodeBuffer. Sets up the size of the
> > > > different regions, // and copy code and relocation info.*!
> > > > CodeBlob::CodeBlob(**! RuntimeBlob::RuntimeBlob(*
> > > >
> > > >
> > > > - why do we need:
> > > >
> > > > *+ bool make_not_used() { return make_not_entrant(); }*
> > > >
> > > > it only forwards to make_not_entrant() and it is only used a single
> > time in
> > > > ciEnv.cpp:
> > > >
> > > > *! old->make_not_entrant();**!
> > > > old->make_not_used();*
> > > >
> > > >
> > > > I can answer this. make_not_used is virtual:
> > > >
> > > > virtual bool make_not_used() = 0;
> > > >
> > > > Can you guess why this is the case? :-) The reason is that the
> > > > implementation is different for AOT compiled methods.
> > > >
> > > >
> > > OK, I see. Thanks for the background info but now I can not refrain from
> > > commenting :)
> > >
> > > If SAP (or anybody else outside Oracle) would submit such a kind of XL
> > > change in order to better support let's say it's closed HPUX/Itanium
> > port I
> > > don't think it would be even considered.
> > >
> > > I don't want to reject these specific change (I came to terms with it :)
> > > but I think this should stand as bad example for changes which will not
> > > happen too often in the future.
> > >
> > >
> > > >
> > > >
> > > > - I don't understand why we need both NMethodIterator and
> > > > CompiledMethodIterator - they're virtually the same and nmethod is
> > > > currently the only subclass of CompiledMethod. Can you please be more
> > > > specific why you've changed some instances of NMethodIterator to
> > > > CompiledMethodIterator and others not. Without background information
> > this
> > > > makes no sense to me. Also, the advance method in
> > CompiledMethodIterator
> > > > isn't "inline" while the one in NMethodIterator is - don't know if this
> > > > will be a performance problem.
> > > >
> > > > The rest looks good to me but please notice that I still haven't
> > looked at
> > > > all changes (especially not on the agent/ and dtrace/ files). So you
> > should
> > > > get at least one more reviewer for such a big change.
> > > >
> > > > Regards,
> > > > Volker
> > > >
> > > >
> > > >
> > > > On Tue, Apr 19, 2016 at 7:32 AM, Rickard Bäckman <
> > > > rickard.backman@oracle.com
> > > >
> > > > wrote:
> > > >
> > > >
> > > > Here is the updated webrev, rebased and I think I have fixed all the
> > > > comments with one exception.
> > > >
> > > > I've avoided making CompiledMethodIterator and NMethodIterator a
> > > > template class for now. I agree we should do something to reuse the
> > > > parts that are identical but for now I think there will be a few more
> > > > changes to CompiledMethodIterator in an upcoming RFR. So can we hold
> > off
> > > > with that change?
> > > >
> > > > Webrev: http://cr.openjdk.java.net/~rbackman/8152664.3/
> > > >
> > > > Thanks
> > > >
> > > > On 04/07, Rickard Bäckman wrote:
> > > >
> > > > Hi,
> > > >
> > > > can I please have review for this patch please?
> > > >
> > > > So far CodeBlobs have required all the data (metadata, oops, code, etc)
> > > > to be in one continuous blob With this patch we are looking to change
> > > > that. It's been done by changing offsets in CodeBlob to addresses,
> > > > making some methods virtual to allow different behavior and also
> > > > creating a couple of new classes. CompiledMethod now sits inbetween
> > > > CodeBlob and nmethod.
> > > >
> > > > CR: https://bugs.openjdk.java.net/browse/JDK-8152664
> > > > Webrev: http://cr.openjdk.java.net/~rbackman/8152664/
> > > >
> > > > Thanks
> > > > /R
> > > >
> > > > /R
> > > >
> > > >
> > > >
> >
> diff -r a1d6c22335bb src/share/vm/code/relocInfo.hpp
> --- a/src/share/vm/code/relocInfo.hpp Mon Apr 25 14:30:01 2016 +0200
> +++ b/src/share/vm/code/relocInfo.hpp Mon Apr 25 15:12:16 2016 +0200
> @@ -28,6 +28,8 @@
> #include "memory/allocation.hpp"
> #include "runtime/os.hpp"
>
> +class nmethod;
> +class CompiledMethod;
> class Metadata;
> class NativeMovConstReg;
>
> diff -r a1d6c22335bb src/share/vm/oops/method.hpp
> --- a/src/share/vm/oops/method.hpp Mon Apr 25 14:30:01 2016 +0200
> +++ b/src/share/vm/oops/method.hpp Mon Apr 25 15:12:16 2016 +0200
> @@ -432,9 +432,9 @@
> // nmethod/verified compiler entry
> address verified_code_entry();
> bool check_code() const; // Not inline to avoid circular ref
> - nmethod* volatile code() const { assert( check_code(), "" ); \
> return (nmethod *)OrderAccess::load_ptr_acquire(&_code); } + CompiledMethod* \
> volatile code() const { assert( check_code(), "" ); return \
> (CompiledMethod *)OrderAccess::load_ptr_acquire(&_code); } void clear_code(); \
> // Clear out any compiled code
> - static void set_code(methodHandle mh, nmethod* code);
> + static void set_code(methodHandle mh, CompiledMethod* code);
> void set_adapter_entry(AdapterHandlerEntry* adapter) {
> constMethod()->set_adapter_entry(adapter);
> }
> diff -r a1d6c22335bb src/share/vm/runtime/frame.cpp
> --- a/src/share/vm/runtime/frame.cpp Mon Apr 25 14:30:01 2016 +0200
> +++ b/src/share/vm/runtime/frame.cpp Mon Apr 25 15:12:16 2016 +0200
> @@ -661,13 +661,16 @@
> }
> } else if (_cb->is_buffer_blob()) {
> st->print("v ~BufferBlob::%s", ((BufferBlob *)_cb)->name());
> - } else if (_cb->is_nmethod()) {
> - nmethod* nm = (nmethod*)_cb;
> - Method* m = nm->method();
> + } else if (_cb->is_compiled()) {
> + CompiledMethod* cm = (CompiledMethod*)_cb;
> + Method* m = cm->method();
> if (m != NULL) {
> - st->print("J %d%s", nm->compile_id(), (nm->is_osr_method() ? "%" : ""));
> - if (nm->compiler() != NULL) {
> - st->print(" %s", nm->compiler()->name());
> + if (cm->is_nmethod()) {
> + nmethod* nm = cm->as_nmethod();
> + st->print("J %d%s", nm->compile_id(), (nm->is_osr_method() ? "%" : ""));
> + if (nm->compiler() != NULL) {
> + st->print(" %s", nm->compiler()->name());
> + }
> }
> m->name_and_sig_as_C_string(buf, buflen);
> st->print(" %s", buf);
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic