RFR[S] 8005165 Platform-independent C++ vtables for CDS
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Mon Mar 6 12:27:28 UTC 2017
On 3/5/17 5:35 PM, Ioi Lam wrote:
>
>
> On 3/5/17 7:17 AM, 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/
>>>
>>>
>>> On 3/2/17 8:48 AM, 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
>>>>
>>>> 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
>>> (-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.
>>
>
> I can go with either implementation, although I like the original one
> that doesn't use SafeFetch.
>> Ioi, could you make all methods of CppVtableCloner out of line?
>>
> Is it for debugging purposes? I am using a recent version of gdb and I
> have no problems setting break points or stepping into the code. I can
> move the bigger methods out, but for clarity I think it's better to
> leave the small methods inside the class declaration.
Yes, only the bigger methods need to be out of line. I've never had a
problem with gdb and separating declaration and implementation
(details!) make it much easier to read, or skip, depending on what
you're looking for.
thanks!
Coleen
>
> Thanks
> - Ioi
>
>> 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
>>>>
>>>> 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
>>>>>>> http://cr.openjdk.java.net/~iklam/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).
>>>>>>>
>>>>>>> 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