RFR (XL): 8152664 - Support non-continuous CodeBlobs in HotSpot
Volker Simonis
volker.simonis at gmail.com
Mon Apr 25 13:34:10 UTC 2016
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 at 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 at oracle.com> wrote:
> >
> > >
> > > On Apr 19, 2016, at 4:30 AM, Volker Simonis <volker.simonis at 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 at 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
> > >
> > >
> > >
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 8152664_v4_addon.patch
Type: text/x-patch
Size: 2403 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/hotspot-dev/attachments/20160425/899c05af/8152664_v4_addon.patch>
More information about the hotspot-dev
mailing list