RFR[S] 8005165 Platform-independent C++ vtables for CDS
Ioi Lam
ioi.lam at oracle.com
Tue Mar 7 01:25:23 UTC 2017
Hi Thomas & Coleen,
Thanks for your comments. I have updated the rev:
http://cr.openjdk.java.net/~iklam/jdk10/8005165-platform-independent-cds-vtable.v04/
[1] Switched back to computing the exact vtable size
[2] Move non-trivial functions outside of their class declaration
Thanks
- Ioi
On 3/6/17 8:51 AM, Thomas Stüfe wrote:
> Hi Coleen and Ioi,
>
> I had to port C++ code to platforms with terrible compilers for a time
> in my life, that is why I like code to be as portable as possible.
> That said, you are right in your argumentation, the SafeFetch solution
> is not terribly elegant and Ioi's original way of determining the
> vtable size is cleaner.
>
> I did some checks on some of our architectures with a test similar to
> Ioi's and on a first glance it seems to work for simple cases (single
> and public inheritance) on ppc (AIX) and Linux ia64. Although the
> vtables seemed to me to contain function descriptors, not real
> pointers to code, so this is something to keep in mind. But if the
> live vtable are copied, the function descriptors they contain should
> point to valid code too, so it should not matter. Just to remember to
> not expect every slot in the array to be a valid code pointer.
>
> So, in short, I remove my objection to Ioi's original solution, as far
> as that matters.
>
> I still think we rely on a lot here: Contiguous vtable containing
> absolute memory addresses, vtable pointer at start of object and
> vtable entries to be ordered from base->derived class. So I wonder how
> much effort it would be (now or in the future as a separate change) to
> have a fallback where - at loading time - instead of copying vtables
> the vtable pointers in the objects were fixed up to point to the new
> live vtables? I know this would be more expensive and potentially
> defeat the point of shared classes. But maybe not, it depends on how
> many objects are there, no?
>
> Kind Regards, Thomas
>
> On Sun, Mar 5, 2017 at 4:17 PM, <coleen.phillimore at oracle.com
> <mailto:coleen.phillimore at oracle.com>> wrote:
>
>
> Ioi, Some comments inline (where no comments, insert "ok") :)
>
>
> On 3/2/17 10:37 PM, Ioi Lam wrote:
>
> Hi Coleen,
>
> Thanks for the comments. I have updated the webrev. See
> in-line for responses.
>
> http://cr.openjdk.java.net/~iklam/jdk10/8005165-platform-independent-cds-vtable.v03/
> <http://cr.openjdk.java.net/%7Eiklam/jdk10/8005165-platform-independent-cds-vtable.v03/>
>
>
> On 3/2/17 8:48 AM, coleen.phillimore at oracle.com
> <mailto:coleen.phillimore at oracle.com> wrote:
>
>
> Ioi
> I like the concept of this a lot but have some stylistic
> comments to help people reading this code later.
>
> http://cr.openjdk.java.net/~iklam/jdk10/8005165-platform-independent-cds-vtable.v02/src/share/vm/memory/metaspaceShared.cpp.udiff.html
> <http://cr.openjdk.java.net/%7Eiklam/jdk10/8005165-platform-independent-cds-vtable.v02/src/share/vm/memory/metaspaceShared.cpp.udiff.html>
>
> s/vtab/vtable/g and s/Vtab/Vtable/ please. It doesn't
> save many characters, especially in CppVtableInfo/Testers
>
> Done.
>
> + // Start at slot 1, because slot 0 may be RTTI (on
> Solaris/Sparc)
> + int i;
> + for (i=1; ; i++) {
> Since you're using 'i' later, can you rename it to
> something descriptive. Or have another variable
> "vtable_length" to use later. This looks like an old
> style for loop.
>
> Done
>
> Can the functions for CppVtableInfo be declared outside of
> the class declaration? They don't need to be inline and
> then the debug code for testing the vtable size can be not
> in the middle of the class declaration. Then you can
> move the Tester classes to inside the same #ifndef PRODUCT
> block.
>
> Can you put #endif // PRODUCT when the ifdef covers
> several lines of code?
>
> Done
>
> vtab_of could be more descriptive, like cpp_vtable_for().
>
> I changed to vtable_of(). Because the class name is already
> CppVtableCloner, repeating the word "cpp" seems repetitive to me.
>
> Was PrintSharedSpaces was never converted to UL?
>
> Right. I've filed
> https://bugs.openjdk.java.net/browse/JDK-8176132
> <https://bugs.openjdk.java.net/browse/JDK-8176132>
> (-XX:+PrintSharedSpaces should be converted to use Unified
> Logging.)
>
> + int n = MAX_VTABLE_SIZE;
>
> Can you propagate MAX_VTABLE_SIZE to the places where it's
> used. n isn't descriptive. This starts out with
> max_vtable_size and then changes the size. Reusing 'n'
> makes this really hard to follow. Not having a comment
> that we only allocate enough slots for the vtable makes it
> hard too.
>
> + // allocate CppVtableInfo in the MD section + _info =
> (CppVtabInfo*)md_top;
> + _info->set_vtab_size(n); // initially set to max_vtable_size
> + + // allocate temporary local instance of the metadata
> type T + T tmp;
> + intptr_t* srcvtab = vtab_of(tmp);
> + intptr_t* dstvtab = _info->vtab();
> +
>
> Fixed.
>
> Something like that for comments. dstvtab is the
> destination_vtable in the MD section.
>
>
> I've dropped the md_ prefix from the functions that deal with
> the vtables, since they shouldn't care whether it's the "MD"
> section or not. Now it looks like this:
>
> // Allocate and initialize the C++ vtables, starting from top,
> but do not go past end.
> intptr_t*
> MetaspaceShared::allocate_cpp_vtable_clones(intptr_t* top,
> intptr_t* end) {
> assert(DumpSharedSpaces, "dump-time only");
> // Layout (each slot is a intptr_t):
> // [number of slots in the first vtable = n1]
> // [ <n1> slots for the first vtable]
> // [number of slots in the first second = n2]
> // [ <n2> slots for the second vtable]
> // ...
> // The order of the vtables is the same as the
> CPP_VTAB_PATCH_TYPES_DO macro.
> CPP_VTABLE_PATCH_TYPES_DO(ALLOC_CPP_VTABLE_CLONE);
> return top;
> }
>
> + for (int i=0; i<n; i++) {
> + const intptr_t bad = intptr_t(0xdeadbeef);
> + intptr_t num = SafeFetchN(&srcvtab[i], bad);
> + if (num == bad
> + // || i > 120 /* uncomment this line to test */
> + ) {
> + _info->set_vtab_size(i-1);
> + break;
> + }
> + dstvtab[i] = num;
> + }
> I dont understand this code. You get deadbeef for a bad
> value if SafeFetchN gets a fault but why would it get a
> fault at the end of the metadata's vtable? Couldn't it
> just run onto the next vtable? I think your original way
> of counting vtable entries might be better (sorry I didn't
> have time to study that thread).
>
> I've modified the comments to this. Does it make sense to you?
>
> // It is not always safe to call memcpy(), because
> srcvtable might be shorter than
> // MAX_VTABLE_SIZE, and the C++ linker might have placed
> the vtable at the very
> // end of the last page of libjvm.so. Crossing over to the
> next page might
> // cause a page fault.
>
> My fear is the JVM would suddenly start crashing because the
> order of .o files have changed on the linker's command line,
> or if you enable some special linker optimization flags. It's
> better safe than sorry.
>
>
> This wasn't exactly what I was not understanding. I didn't see
> that you are copying 120 entries from the old vtable and junk
> memory beyond the old vtable, unless you get a segv, in which case
> you copy less. I don't think you should copy random memory into
> the vtable in the archive. This doesn't seem secure, even with
> the segv protection.
>
> Since we already have assumptions about C++ vtable layout in the
> code and it's mostly specified by various ABIs, and you have the
> assert code, I think I would prefer that you copy only the vtable
> entries into the archive. I guess Thomas Stuefe had a different
> opinion. I've read the original thread. Two points:
>
> If new C++ compiler implementations add a discontigous vtable,
> both the SafeFetchN and subclass additional virtual function at
> end implementation will fail. I don't think C++ implementations
> would do this and a contiguous vtable as first in the instance has
> been standard for years. If our metadata adds multiple
> inheritance, the same issue would be a problem for both
> implementations, as well as for the implementation we have before
> Ioi's fix.
>
> Ioi's subclass adding virtual function method would work for any
> esoteric C++ implementations in my memory, except the vptr for the
> old DECC++ compiler was after the nonstatic data members (which
> would fail with all of our implementations).
>
> Since the code is there anyway for debug purposes, we're not
> saving code by implementing SafeFetchN. The SafeFetchN
> implementation isn't obvious at all what it's doing, and requires
> better comments, especially if you don't know already what
> SafeFetchN does. It looks really cryptic. The poisoned values
> also bothered me in that they overload other poisoned values in
> other parts of the jvm.
>
> Ioi, could you make all methods of CppVtableCloner out of line?
>
> The other changes look good, although I might have more requests
> for comments.
>
> Thanks,
> Coleen
>
>
> Would be nice to have comments here too!!
>
> + intptr_t* start = md_top;
>
> This doesn't do anything (?)
>
>
> Fixed. This was left over code.
>
>
> + MetaspaceShared::zero_cpp_vtable_clones_for_writing();
>
> Why not zero the destination vtable in allocate? Or does
> patching the vtable pointers call virtual functions? You
> could prevent that so you don't need this code.
>
> I added this comment:
>
> // During patching, some virtual methods may be called, so
> at this point
> // the vtables must contain valid methods (as filled in by
> CppVtableCloner::allocate).
> MetaspaceShared::patch_cpp_vtable_pointers();
>
> // The vtable clones contain addresses of the current process.
> // We don't want to write these addresses into the archive.
> MetaspaceShared::zero_cpp_vtable_clones_for_writing();
>
> + // Restore the vtable in case we invoke any virtual methods.
> + MetaspaceShared::clone_cpp_vtables((intptr_t*)vtbl_list);
> Can this be restore_cpp_vtables since that's what it's
> doing. The first is after the dump and the second call is
> at UseSharedSpaces. A couple of comments in this
> clone_cpp_vtables --> restore_cpp_vtables would be nice. eg:
>
> I prefer to use the word clone. Otherwise when you just say
> "vtable" it's not clear whether you're talking about the
> original one (made by the c++ linker), or the cloned one in
> the CDS archive.
>
> + static intptr_t* clone_vtable(const char* name,
> intptr_t* p) {
> + T tmp; // Allocate temporary dummy metadata object to
> get vtable initialized
> + CppVtabInfo* info = (CppVtabInfo*)p;
> + int n = info->vtab_size();
> + intptr_t* srcvtab = vtab_of(tmp);
> + intptr_t* dstvtab = info->vtab();
> +
> + // We already checked (and, if necessary, adjusted n)
> when the vtables were allocated, so we are
> + // safe to do memcpy.
> + if (PrintSharedSpaces) {
> + tty->print_cr("%s copying %d vtable entries", name, n);
> + }
> + memcpy(dstvtab, srcvtab, sizeof(intptr_t) * n);
> + return dstvtab + n;
> + }
>
> Done. I changed the wording
>
> T tmp; // Allocate temporary dummy metadata object to get
> to the original vtable.
>
> As we are not really "initializing a vtable" here.
>
> Same with 'patch'. It'd be so much faster and easier to
> read this code with more comments please.
>
> http://cr.openjdk.java.net/~iklam/jdk10/8005165-platform-independent-cds-vtable.v02/src/share/vm/oops/constantPool.hpp.udiff.html
> <http://cr.openjdk.java.net/%7Eiklam/jdk10/8005165-platform-independent-cds-vtable.v02/src/share/vm/oops/constantPool.hpp.udiff.html>
>
> Why are these testers here?
>
>
> I updated the comment:
>
> // Used by CDS. These classes need to access the private
> ConstantPool() constructor.
> template <class T> friend class CppVtableTesterA;
> template <class T> friend class CppVtableTesterB;
> template <class T> friend class CppVtableCloner;
>
>
> Thanks
> - Ioi
>
>
> On 3/1/17 3:25 AM, Ioi Lam wrote:
> https://bugs.openjdk.java.net/browse/JDK-8005165
> <https://bugs.openjdk.java.net/browse/JDK-8005165>
> http://cr.openjdk.java.net/~iklam/jdk10/8005165-platform-independent-cds-vtable.v02/
> <http://cr.openjdk.java.net/%7Eiklam/jdk10/8005165-platform-independent-cds-vtable.v02/>
>
> Hi,
>
> This is the official review (follow up of the
> "Determining the size of C++ vtables" thread
> onhotspot-dev at openjdk.java.net
> <mailto:onhotspot-dev at openjdk.java.net>).
>
> The new code has the same assumption as the
> existing code in JDK 10: for a C++ object that
> contains virtual methods (e.g., ConstantPool),
> we assume the first intptr_t slot of the
> object is a _vptr, which points to a vtable,
> which consists of no more than 150 intptr_t's.
>
> ConstantPool*p -->[ _vptr ] -------> [
> vtable slot 0 ]
> [ field #0 ] [
> vtable slot 1 ]
> [ field #1 ] [
> vtable slot 2 ]
> [ field #2 ] [
> vtable slot 3 ]
> [ .... ] [
> vtable slot 4]
> [
> vtable slot 5 ]
> [
> ... ]
>
> + In the existing code, we were pointing the
> vtable slots to
> code that's generated by HotSpot.
>
> + In the new code, we copy the vtable slots
> from an existing
> vtable (generated by the C++ linker).
>
> Per Thomas Stüfe's advice, I don't try to
> determine the size of the vtable (as that
> would add one more compiler requirement where
> new virtual methods added by a subclass must
> be placed at a higher offset in the vtable).
>
> Instead, I have added code in non-product
> builds to ensure that the vtables are no
> longer than 150 entries. You can run with
> "-XX:+PrintSharedSpaces -Xshare:dump" to print
> out the actual size of the vtables for your
> particular platform:
>
> ConstantPool has 12 virtual methods
> InstanceKlass has 113 virtual methods
> InstanceClassLoaderKlass has 113 virtual methods
> InstanceMirrorKlass has 113 virtual methods
> InstanceRefKlass has 113 virtual methods
> Method has 12 virtual methods
> ObjArrayKlass has 114 virtual methods
> TypeArrayKlass has 114 virtual methods
>
> As mentioned in the code comments, if you have
> an esoteric C++ compiler, the
> verify_sufficient_size() function will
> probably fail, but hopefully that would give
> you some hints for porting this code.
>
> To avoid accidentally touching an unmapped
> page, the code uses SafeFetchN for copying the
> vtable contents, and would shrink the vtable
> to less than 150 entries if necessary. I can't
> test this for real, but I've added some code
> to simulate an error:
>
> for (int i=0; i<n; i++) {
> const intptr_t bad = intptr_t(0xdeadbeef);
> intptr_t num = SafeFetchN(&srcvtab[i], bad);
> if (num == bad
> // || i > 120 /* uncomment this line
> to test */
> ) {
> _info->set_vtab_size(i-1);
> break;
> }
> dstvtab[i] = num;
> }
>
> Results:
>
> + Removed 850 lines of CPU-dependent code
>
> + CDS image is about 50K smaller
>
> + Previously Metadata objects must live in the
> read-write section in the CDS
> archive, because their _vptr was updated at
> run time. Now _vptr is no longer
> updated, so ConstantPool can be moved to the
> read-only section (see JDK-8171392).
>
> Thanks
> - Ioi
>
>
>
>
>
>
>
More information about the hotspot-dev
mailing list