RFR[S] 8005165 Platform-independent C++ vtables for CDS
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Tue Mar 7 13:48:35 UTC 2017
This looks really good!! Are you going to do a follow up to remove the
MC section?
thanks!
Coleen
On 3/6/17 8:25 PM, Ioi Lam wrote:
> 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