Determining the size of C++ vtables

classic Classic list List threaded Threaded
32 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Determining the size of C++ vtables

Ioi Lam
Hi,

I am working on https://bugs.openjdk.java.net/browse/JDK-8005165 (Remove
CPU-dependent code in self-patching vtables), I need a way find out the size
of a C++ vtable. I ended up doing this:


// Objects of the Metadata types (such as Klass and ConstantPool) have
C++ vtables.
// (In GCC this is the field <Type>::_vptr, i.e., first word in the object.)
//
// Addresses of the vtables and the methods may be different across JVM
runs,
// if libjvm.so is dynamically loaded at a different base address.
//
// To ensure that the Metadata objects in the CDS archive always have
the correct vtable:
//
// + at dump time:  we redirect the _vptr to point to our own vtables inside
//                  the CDS image
// + at run time:   we clone the actual contents of the vtables from
libjvm.so
//                  into our own tables.
//
// To determine the size of the vtable for each type, we use the following
// trick by declaring 2 subclasses:
//
//   class CppVtabTesterA: public InstanceKlass {
//          virtual int   last_virtual_method() {return 1;}
//   };
//   class CppVtabTesterB: public InstanceKlass {
//          virtual void* last_virtual_method() {return NULL};
//   };
//
// CppVtabTesterA and CppVtabTesterB's vtables have the following
properties:
// - Their size (N+1) is exactly one more than the size of
InstanceKlass's vtable (N)
// - The first N entries have are exactly the same as in InstanceKlass's
vtable.
// - Their last entry is different.
//
// So to determine the value of N, we just walk CppVtabTesterA and
CppVtabTesterB's tables
// and find the first entry that's different


Could anyone comment if this is acceptable? I know it's not 100%
portable (C++ doesn't
specify where to find the vtable, or what's inside), but my assumptions
is the same as
the existing code. I.e., _vptr is a pointer located at offset 0 of the
object, and it
points to a one-dimensional array.

So at least it's not any worse than before?

Thanks
- Ioi

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Determining the size of C++ vtables

Ioi Lam


On 2/23/17 7:47 PM, Ioi Lam wrote:

> Hi,
>
> I am working on https://bugs.openjdk.java.net/browse/JDK-8005165 (Remove
> CPU-dependent code in self-patching vtables), I need a way find out
> the size
> of a C++ vtable. I ended up doing this:
>
>
> // Objects of the Metadata types (such as Klass and ConstantPool) have
> C++ vtables.
> // (In GCC this is the field <Type>::_vptr, i.e., first word in the
> object.)
> //
> // Addresses of the vtables and the methods may be different across
> JVM runs,
> // if libjvm.so is dynamically loaded at a different base address.
> //
> // To ensure that the Metadata objects in the CDS archive always have
> the correct vtable:
> //
> // + at dump time:  we redirect the _vptr to point to our own vtables
> inside
> //                  the CDS image
> // + at run time:   we clone the actual contents of the vtables from
> libjvm.so
> //                  into our own tables.
> //
> // To determine the size of the vtable for each type, we use the
> following
> // trick by declaring 2 subclasses:
> //
> //   class CppVtabTesterA: public InstanceKlass {
> //          virtual int   last_virtual_method() {return 1;}
> //   };
> //   class CppVtabTesterB: public InstanceKlass {
> //          virtual void* last_virtual_method() {return NULL};
> //   };
> //
> // CppVtabTesterA and CppVtabTesterB's vtables have the following
> properties:
> // - Their size (N+1) is exactly one more than the size of
> InstanceKlass's vtable (N)
> // - The first N entries have are exactly the same as in
> InstanceKlass's vtable.
> // - Their last entry is different.
> //
> // So to determine the value of N, we just walk CppVtabTesterA and
> CppVtabTesterB's tables
> // and find the first entry that's different
>
>
> Could anyone comment if this is acceptable? I know it's not 100%
> portable (C++ doesn't
> specify where to find the vtable, or what's inside), but my
> assumptions is the same as
> the existing code. I.e., _vptr is a pointer located at offset 0 of the
> object, and it
> points to a one-dimensional array.
>
> So at least it's not any worse than before?
>
> Thanks
> - Ioi
>
By the way, I first tried having only a single "tester" class and walk
the vtable to look for &last_virtual_method, but the C++ compiler told
me that taking the address of a non-static function is not allowed .....
so I ended up creating two tester classes and checking their differences.



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Determining the size of C++ vtables

Thomas Stüfe-2
Hi Ioi,

After reading through the original mailthread referenced in the bug report,
the only reason the "just copy enough (200) slots from the vtable in
libjvm.so to the vtable in the image" approach does not work for you is
that you are afraid of hitting unmapped memory? If I understand it
correctly.

If that is the case, why not copy 200 slots with SafeFetch and stop at the
first error? That should not incur much additional costs as long as you do
not hit an unmapped area, in which case you'd stop anyway.

That way you make yourself a bit less dependent on compiler internals. I
agree that your approach is more elegant, though.

Kind Regards, Thomas

On Fri, Feb 24, 2017 at 4:55 AM, Ioi Lam <[hidden email]> wrote:

>
>
> On 2/23/17 7:47 PM, Ioi Lam wrote:
>
>> Hi,
>>
>> I am working on https://bugs.openjdk.java.net/browse/JDK-8005165 (Remove
>> CPU-dependent code in self-patching vtables), I need a way find out the
>> size
>> of a C++ vtable. I ended up doing this:
>>
>>
>> // Objects of the Metadata types (such as Klass and ConstantPool) have
>> C++ vtables.
>> // (In GCC this is the field <Type>::_vptr, i.e., first word in the
>> object.)
>> //
>> // Addresses of the vtables and the methods may be different across JVM
>> runs,
>> // if libjvm.so is dynamically loaded at a different base address.
>> //
>> // To ensure that the Metadata objects in the CDS archive always have the
>> correct vtable:
>> //
>> // + at dump time:  we redirect the _vptr to point to our own vtables
>> inside
>> //                  the CDS image
>> // + at run time:   we clone the actual contents of the vtables from
>> libjvm.so
>> //                  into our own tables.
>> //
>> // To determine the size of the vtable for each type, we use the following
>> // trick by declaring 2 subclasses:
>> //
>> //   class CppVtabTesterA: public InstanceKlass {
>> //          virtual int   last_virtual_method() {return 1;}
>> //   };
>> //   class CppVtabTesterB: public InstanceKlass {
>> //          virtual void* last_virtual_method() {return NULL};
>> //   };
>> //
>> // CppVtabTesterA and CppVtabTesterB's vtables have the following
>> properties:
>> // - Their size (N+1) is exactly one more than the size of
>> InstanceKlass's vtable (N)
>> // - The first N entries have are exactly the same as in InstanceKlass's
>> vtable.
>> // - Their last entry is different.
>> //
>> // So to determine the value of N, we just walk CppVtabTesterA and
>> CppVtabTesterB's tables
>> // and find the first entry that's different
>>
>>
>> Could anyone comment if this is acceptable? I know it's not 100% portable
>> (C++ doesn't
>> specify where to find the vtable, or what's inside), but my assumptions
>> is the same as
>> the existing code. I.e., _vptr is a pointer located at offset 0 of the
>> object, and it
>> points to a one-dimensional array.
>>
>> So at least it's not any worse than before?
>>
>> Thanks
>> - Ioi
>>
>> By the way, I first tried having only a single "tester" class and walk
> the vtable to look for &last_virtual_method, but the C++ compiler told me
> that taking the address of a non-static function is not allowed ..... so I
> ended up creating two tester classes and checking their differences.
>
>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Determining the size of C++ vtables

Ioi Lam


On 2/23/17 11:17 PM, Thomas Stüfe wrote:

> Hi Ioi,
>
> After reading through the original mailthread referenced in the bug
> report, the only reason the "just copy enough (200) slots from the
> vtable in libjvm.so to the vtable in the image" approach does not work
> for you is that you are afraid of hitting unmapped memory? If I
> understand it correctly.
>
> If that is the case, why not copy 200 slots with SafeFetch and stop at
> the first error? That should not incur much additional costs as long
> as you do not hit an unmapped area, in which case you'd stop anyway.
>
Oh, that's a good simplification.I hadn't thought about using SafeFetch :-(

One issue with it is the 200 slots are too big (ConstantPool needs just
10 slots), so finding the exact size is a little more efficient and
saves a few KBs.

Thanks
- Ioi

> That way you make yourself a bit less dependent on compiler internals.
> I agree that your approach is more elegant, though.
>
> Kind Regards, Thomas
>
> On Fri, Feb 24, 2017 at 4:55 AM, Ioi Lam <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>
>
>     On 2/23/17 7:47 PM, Ioi Lam wrote:
>
>         Hi,
>
>         I am working on
>         https://bugs.openjdk.java.net/browse/JDK-8005165
>         <https://bugs.openjdk.java.net/browse/JDK-8005165> (Remove
>         CPU-dependent code in self-patching vtables), I need a way
>         find out the size
>         of a C++ vtable. I ended up doing this:
>
>
>         // Objects of the Metadata types (such as Klass and
>         ConstantPool) have C++ vtables.
>         // (In GCC this is the field <Type>::_vptr, i.e., first word
>         in the object.)
>         //
>         // Addresses of the vtables and the methods may be different
>         across JVM runs,
>         // if libjvm.so is dynamically loaded at a different base address.
>         //
>         // To ensure that the Metadata objects in the CDS archive
>         always have the correct vtable:
>         //
>         // + at dump time:  we redirect the _vptr to point to our own
>         vtables inside
>         //                  the CDS image
>         // + at run time:   we clone the actual contents of the
>         vtables from libjvm.so
>         //                  into our own tables.
>         //
>         // To determine the size of the vtable for each type, we use
>         the following
>         // trick by declaring 2 subclasses:
>         //
>         //   class CppVtabTesterA: public InstanceKlass {
>         //          virtual int   last_virtual_method() {return 1;}
>         //   };
>         //   class CppVtabTesterB: public InstanceKlass {
>         //          virtual void* last_virtual_method() {return NULL};
>         //   };
>         //
>         // CppVtabTesterA and CppVtabTesterB's vtables have the
>         following properties:
>         // - Their size (N+1) is exactly one more than the size of
>         InstanceKlass's vtable (N)
>         // - The first N entries have are exactly the same as in
>         InstanceKlass's vtable.
>         // - Their last entry is different.
>         //
>         // So to determine the value of N, we just walk CppVtabTesterA
>         and CppVtabTesterB's tables
>         // and find the first entry that's different
>
>
>         Could anyone comment if this is acceptable? I know it's not
>         100% portable (C++ doesn't
>         specify where to find the vtable, or what's inside), but my
>         assumptions is the same as
>         the existing code. I.e., _vptr is a pointer located at offset
>         0 of the object, and it
>         points to a one-dimensional array.
>
>         So at least it's not any worse than before?
>
>         Thanks
>         - Ioi
>
>     By the way, I first tried having only a single "tester" class and
>     walk the vtable to look for &last_virtual_method, but the C++
>     compiler told me that taking the address of a non-static function
>     is not allowed ..... so I ended up creating two tester classes and
>     checking their differences.
>
>
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Determining the size of C++ vtables

Ioi Lam
In reply to this post by Ioi Lam


On 2/23/17 7:55 PM, Ioi Lam wrote:

>
>
> On 2/23/17 7:47 PM, Ioi Lam wrote:
>> Hi,
>>
>> I am working on https://bugs.openjdk.java.net/browse/JDK-8005165 (Remove
>> CPU-dependent code in self-patching vtables), I need a way find out
>> the size
>> of a C++ vtable. I ended up doing this:
>>
>>
>> // Objects of the Metadata types (such as Klass and ConstantPool)
>> have C++ vtables.
>> // (In GCC this is the field <Type>::_vptr, i.e., first word in the
>> object.)
>> //
>> // Addresses of the vtables and the methods may be different across
>> JVM runs,
>> // if libjvm.so is dynamically loaded at a different base address.
>> //
>> // To ensure that the Metadata objects in the CDS archive always have
>> the correct vtable:
>> //
>> // + at dump time:  we redirect the _vptr to point to our own vtables
>> inside
>> //                  the CDS image
>> // + at run time:   we clone the actual contents of the vtables from
>> libjvm.so
>> //                  into our own tables.
>> //
>> // To determine the size of the vtable for each type, we use the
>> following
>> // trick by declaring 2 subclasses:
>> //
>> //   class CppVtabTesterA: public InstanceKlass {
>> //          virtual int   last_virtual_method() {return 1;}
>> //   };
>> //   class CppVtabTesterB: public InstanceKlass {
>> //          virtual void* last_virtual_method() {return NULL};
>> //   };
>> //
>> // CppVtabTesterA and CppVtabTesterB's vtables have the following
>> properties:
>> // - Their size (N+1) is exactly one more than the size of
>> InstanceKlass's vtable (N)
>> // - The first N entries have are exactly the same as in
>> InstanceKlass's vtable.
>> // - Their last entry is different.
>> //
>> // So to determine the value of N, we just walk CppVtabTesterA and
>> CppVtabTesterB's tables
>> // and find the first entry that's different
>>
>>
>> Could anyone comment if this is acceptable? I know it's not 100%
>> portable (C++ doesn't
>> specify where to find the vtable, or what's inside), but my
>> assumptions is the same as
>> the existing code. I.e., _vptr is a pointer located at offset 0 of
>> the object, and it
>> points to a one-dimensional array.
>>
>> So at least it's not any worse than before?
>>
>> Thanks
>> - Ioi
>>
> By the way, I first tried having only a single "tester" class and walk
> the vtable to look for &last_virtual_method, but the C++ compiler told
> me that taking the address of a non-static function is not allowed
> ..... so I ended up creating two tester classes and checking their
> differences.
>
>

Just to clarify the above comments: taking a "reference" to a virtual
function is allowed, but it's not specified what the "reference" is.
With gcc, it's a 16-bit value:

    #include <stdio.h>

    class CppVtabTester {
    public:
       virtual int func1() {return 1;}
       int func2()         {return 2;}
    };

    int main() {
       union {
         int (CppVtabTester::*ptr)();
         void* val[2];
       } a, b;

       a.ptr = &CppVtabTester::func1;
       b.ptr = &CppVtabTester::func2;

       printf("%d\n", (int)sizeof(a.ptr));
       printf("ref: %p %p\n", a.val[0], a.val[1]);
       printf("ref: %p %p\n", b.val[0], b.val[1]);

       return 0;
    }

    ioimac ~/tmp$ gcc t.cpp
    ioimac ~/tmp$ ./a.out
    16
    ref: 0x1 0x0
    ref: 0x102d93f70 0x0

Unfortunately, the reference for a virtual function is just its vtable
index :-(

Taking the "address" of a virtual function in some versions of gcc will
give the real address (with a warning), and some versions of gcc will
disallow it:

    printf("ptr: %p\n", (void*)(&CppVtabTester::func1));
    .....
    t.cpp:21:35: error: cannot cast from type 'int (CppVtabTester::*)()'
    to pointer
           type 'void *'
       printf("ptr : %p\n", (void*)(&CppVtabTester::func1));






Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Determining the size of C++ vtables

Thomas Stüfe-2
In reply to this post by Ioi Lam
Hi ioi,

On Fri, Feb 24, 2017 at 7:50 PM, Ioi Lam <[hidden email]> wrote:

>
>
> On 2/23/17 11:17 PM, Thomas Stüfe wrote:
>
> Hi Ioi,
>
> After reading through the original mailthread referenced in the bug
> report, the only reason the "just copy enough (200) slots from the vtable
> in libjvm.so to the vtable in the image" approach does not work for you is
> that you are afraid of hitting unmapped memory? If I understand it
> correctly.
>
> If that is the case, why not copy 200 slots with SafeFetch and stop at the
> first error? That should not incur much additional costs as long as you do
> not hit an unmapped area, in which case you'd stop anyway.
>
> Oh, that's a good simplification.I hadn't thought about using SafeFetch :-(
>
> One issue with it is the 200 slots are too big (ConstantPool needs just 10
> slots), so finding the exact size is a little more efficient and saves a
> few KBs.
>
>
How many vtables are you copying? Does it really matter?

As I said, I think your approach is quite elegant, but not terribly
portable. We already make some assumptions (that vtables are organized as
one continuous block of function pointers) and you add some more
assumptions about the order of the methods in the vtable.

I think even if you can make sure that it works now with our set of
platforms and compilers, this adds the risk that a future compiler update
may change the vtable layout and break the code. E.g. a vtable dumped with
a VM compilers by an older compiler could not be loaded with the newly
compiled VM.

Also, you set the bar a bit higher for future ports, because you add
requirements for the C++ compiler.

But I may be too cautious here and I am waiting for others to chime in.

As a side note, it would make sense to write a tiny C++ test which copies
vtables around using your technique and then checks if they still work;
such a test we could use on out platforms to check if our compilers can
cope. Such a test would also be a good sanity check to add to the VM
initialization if CDS is active.

Kind Regards, Thomas



> Thanks
> - Ioi
>
>
> That way you make yourself a bit less dependent on compiler internals. I
> agree that your approach is more elegant, though.
>
> Kind Regards, Thomas
>
> On Fri, Feb 24, 2017 at 4:55 AM, Ioi Lam <[hidden email]> wrote:
>
>>
>>
>> On 2/23/17 7:47 PM, Ioi Lam wrote:
>>
>>> Hi,
>>>
>>> I am working on https://bugs.openjdk.java.net/browse/JDK-8005165 (Remove
>>> CPU-dependent code in self-patching vtables), I need a way find out the
>>> size
>>> of a C++ vtable. I ended up doing this:
>>>
>>>
>>> // Objects of the Metadata types (such as Klass and ConstantPool) have
>>> C++ vtables.
>>> // (In GCC this is the field <Type>::_vptr, i.e., first word in the
>>> object.)
>>> //
>>> // Addresses of the vtables and the methods may be different across JVM
>>> runs,
>>> // if libjvm.so is dynamically loaded at a different base address.
>>> //
>>> // To ensure that the Metadata objects in the CDS archive always have
>>> the correct vtable:
>>> //
>>> // + at dump time:  we redirect the _vptr to point to our own vtables
>>> inside
>>> //                  the CDS image
>>> // + at run time:   we clone the actual contents of the vtables from
>>> libjvm.so
>>> //                  into our own tables.
>>> //
>>> // To determine the size of the vtable for each type, we use the
>>> following
>>> // trick by declaring 2 subclasses:
>>> //
>>> //   class CppVtabTesterA: public InstanceKlass {
>>> //          virtual int   last_virtual_method() {return 1;}
>>> //   };
>>> //   class CppVtabTesterB: public InstanceKlass {
>>> //          virtual void* last_virtual_method() {return NULL};
>>> //   };
>>> //
>>> // CppVtabTesterA and CppVtabTesterB's vtables have the following
>>> properties:
>>> // - Their size (N+1) is exactly one more than the size of
>>> InstanceKlass's vtable (N)
>>> // - The first N entries have are exactly the same as in InstanceKlass's
>>> vtable.
>>> // - Their last entry is different.
>>> //
>>> // So to determine the value of N, we just walk CppVtabTesterA and
>>> CppVtabTesterB's tables
>>> // and find the first entry that's different
>>>
>>>
>>> Could anyone comment if this is acceptable? I know it's not 100%
>>> portable (C++ doesn't
>>> specify where to find the vtable, or what's inside), but my assumptions
>>> is the same as
>>> the existing code. I.e., _vptr is a pointer located at offset 0 of the
>>> object, and it
>>> points to a one-dimensional array.
>>>
>>> So at least it's not any worse than before?
>>>
>>> Thanks
>>> - Ioi
>>>
>>> By the way, I first tried having only a single "tester" class and walk
>> the vtable to look for &last_virtual_method, but the C++ compiler told me
>> that taking the address of a non-static function is not allowed ..... so I
>> ended up creating two tester classes and checking their differences.
>>
>>
>>
>>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR[S] 8005165 Platform-independent C++ vtables for CDS

Ioi Lam
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 on [hidden email]).

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



Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Determining the size of C++ vtables

Ioi Lam
In reply to this post by Thomas Stüfe-2


On 2/24/17 11:52 PM, Thomas Stüfe wrote:

> Hi ioi,
>
> On Fri, Feb 24, 2017 at 7:50 PM, Ioi Lam <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>
>
>     On 2/23/17 11:17 PM, Thomas Stüfe wrote:
>>     Hi Ioi,
>>
>>     After reading through the original mailthread referenced in the
>>     bug report, the only reason the "just copy enough (200) slots
>>     from the vtable in libjvm.so to the vtable in the image" approach
>>     does not work for you is that you are afraid of hitting unmapped
>>     memory? If I understand it correctly.
>>
>>     If that is the case, why not copy 200 slots with SafeFetch and
>>     stop at the first error? That should not incur much additional
>>     costs as long as you do not hit an unmapped area, in which case
>>     you'd stop anyway.
>>
>     Oh, that's a good simplification.I hadn't thought about using
>     SafeFetch :-(
>
>     One issue with it is the 200 slots are too big (ConstantPool needs
>     just 10 slots), so finding the exact size is a little more
>     efficient and saves a few KBs.
>
>
> How many vtables are you copying? Does it really matter?
>
I am copying only 8 vtables, so it doesn't really matter.

> As I said, I think your approach is quite elegant, but not terribly
> portable. We already make some assumptions (that vtables are organized
> as one continuous block of function pointers) and you add some more
> assumptions about the order of the methods in the vtable.
>
> I think even if you can make sure that it works now with our set of
> platforms and compilers, this adds the risk that a future compiler
> update may change the vtable layout and break the code. E.g. a vtable
> dumped with a VM compilers by an older compiler could not be loaded
> with the newly compiled VM.
This shouldn't be a problem with usual compiler upgrades:

You can load a CDS image only with the VM that generates it (we have a
VM version string check), so if you have upgraded the compiler (or
enabled RTTI and rebuilt the VM), so you have larger vtables, you won't
be able to load any old CDS image anyway.

However, if your compiler completely changes the way it implements
vtables (e.g., adding one level of indirection to vptr), then, things
will break, but the existing CDS vtable code also has the same problem.

> Also, you set the bar a bit higher for future ports, because you add
> requirements for the C++ compiler.
>
> But I may be too cautious here and I am waiting for others to chime in.
>
> As a side note, it would make sense to write a tiny C++ test which
> copies vtables around using your technique and then checks if they
> still work; such a test we could use on out platforms to check if our
> compilers can cope. Such a test would also be a good sanity check to
> add to the VM initialization if CDS is active.
>
I've taken your suggestion to use SafeFetchN instead of trying to figure
out the vtable size. That simplifies the product-build a bit.

I put my vtable sizing code in debug builds. It's not strictly portable,
but it probably fail quickly on an esoteric c++ compiler so you can
figure out what's wrong. It will also assert if you have added too many
new virtual method to overflow the current limit of 150.

Please reply your comments to the RFR e-mail thread.

Thanks
- Ioi

> Kind Regards, Thomas
>
>     Thanks
>     - Ioi
>
>
>>     That way you make yourself a bit less dependent on compiler
>>     internals. I agree that your approach is more elegant, though.
>>
>>     Kind Regards, Thomas
>>
>>     On Fri, Feb 24, 2017 at 4:55 AM, Ioi Lam <[hidden email]
>>     <mailto:[hidden email]>> wrote:
>>
>>
>>
>>         On 2/23/17 7:47 PM, Ioi Lam wrote:
>>
>>             Hi,
>>
>>             I am working on
>>             https://bugs.openjdk.java.net/browse/JDK-8005165
>>             <https://bugs.openjdk.java.net/browse/JDK-8005165> (Remove
>>             CPU-dependent code in self-patching vtables), I need a
>>             way find out the size
>>             of a C++ vtable. I ended up doing this:
>>
>>
>>             // Objects of the Metadata types (such as Klass and
>>             ConstantPool) have C++ vtables.
>>             // (In GCC this is the field <Type>::_vptr, i.e., first
>>             word in the object.)
>>             //
>>             // Addresses of the vtables and the methods may be
>>             different across JVM runs,
>>             // if libjvm.so is dynamically loaded at a different base
>>             address.
>>             //
>>             // To ensure that the Metadata objects in the CDS archive
>>             always have the correct vtable:
>>             //
>>             // + at dump time:  we redirect the _vptr to point to our
>>             own vtables inside
>>             //                  the CDS image
>>             // + at run time:   we clone the actual contents of the
>>             vtables from libjvm.so
>>             //                  into our own tables.
>>             //
>>             // To determine the size of the vtable for each type, we
>>             use the following
>>             // trick by declaring 2 subclasses:
>>             //
>>             //   class CppVtabTesterA: public InstanceKlass {
>>             //          virtual int  last_virtual_method() {return 1;}
>>             //   };
>>             //   class CppVtabTesterB: public InstanceKlass {
>>             //          virtual void* last_virtual_method() {return
>>             NULL};
>>             //   };
>>             //
>>             // CppVtabTesterA and CppVtabTesterB's vtables have the
>>             following properties:
>>             // - Their size (N+1) is exactly one more than the size
>>             of InstanceKlass's vtable (N)
>>             // - The first N entries have are exactly the same as in
>>             InstanceKlass's vtable.
>>             // - Their last entry is different.
>>             //
>>             // So to determine the value of N, we just walk
>>             CppVtabTesterA and CppVtabTesterB's tables
>>             // and find the first entry that's different
>>
>>
>>             Could anyone comment if this is acceptable? I know it's
>>             not 100% portable (C++ doesn't
>>             specify where to find the vtable, or what's inside), but
>>             my assumptions is the same as
>>             the existing code. I.e., _vptr is a pointer located at
>>             offset 0 of the object, and it
>>             points to a one-dimensional array.
>>
>>             So at least it's not any worse than before?
>>
>>             Thanks
>>             - Ioi
>>
>>         By the way, I first tried having only a single "tester" class
>>         and walk the vtable to look for &last_virtual_method, but the
>>         C++ compiler told me that taking the address of a non-static
>>         function is not allowed ..... so I ended up creating two
>>         tester classes and checking their differences.
>>
>>
>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Determining the size of C++ vtables

Thomas Stüfe-2
Hi Ioi,

On Wed, Mar 1, 2017 at 3:37 AM, Ioi Lam <[hidden email]> wrote:

>
>
> On 2/24/17 11:52 PM, Thomas Stüfe wrote:
>
> Hi ioi,
>
> On Fri, Feb 24, 2017 at 7:50 PM, Ioi Lam <[hidden email]> wrote:
>
>>
>>
>> On 2/23/17 11:17 PM, Thomas Stüfe wrote:
>>
>> Hi Ioi,
>>
>> After reading through the original mailthread referenced in the bug
>> report, the only reason the "just copy enough (200) slots from the vtable
>> in libjvm.so to the vtable in the image" approach does not work for you is
>> that you are afraid of hitting unmapped memory? If I understand it
>> correctly.
>>
>> If that is the case, why not copy 200 slots with SafeFetch and stop at
>> the first error? That should not incur much additional costs as long as you
>> do not hit an unmapped area, in which case you'd stop anyway.
>>
>> Oh, that's a good simplification.I hadn't thought about using SafeFetch
>> :-(
>>
>> One issue with it is the 200 slots are too big (ConstantPool needs just
>> 10 slots), so finding the exact size is a little more efficient and saves a
>> few KBs.
>>
>>
> How many vtables are you copying? Does it really matter?
>
> I am copying only 8 vtables, so it doesn't really matter.
>
> As I said, I think your approach is quite elegant, but not terribly
> portable. We already make some assumptions (that vtables are organized as
> one continuous block of function pointers) and you add some more
> assumptions about the order of the methods in the vtable.
>
> I think even if you can make sure that it works now with our set of
> platforms and compilers, this adds the risk that a future compiler update
> may change the vtable layout and break the code. E.g. a vtable dumped with
> a VM compilers by an older compiler could not be loaded with the newly
> compiled VM.
>
> This shouldn't be a problem with usual compiler upgrades:
>
> You can load a CDS image only with the VM that generates it (we have a VM
> version string check), so if you have upgraded the compiler (or enabled
> RTTI and rebuilt the VM), so you have larger vtables, you won't be able to
> load any old CDS image anyway.
>
> However, if your compiler completely changes the way it implements vtables
> (e.g., adding one level of indirection to vptr), then, things will break,
> but the existing CDS vtable code also has the same problem.
>
> Also, you set the bar a bit higher for future ports, because you add
> requirements for the C++ compiler.
>
> But I may be too cautious here and I am waiting for others to chime in.
>
> As a side note, it would make sense to write a tiny C++ test which copies
> vtables around using your technique and then checks if they still work;
> such a test we could use on out platforms to check if our compilers can
> cope. Such a test would also be a good sanity check to add to the VM
> initialization if CDS is active.
>
> I've taken your suggestion to use SafeFetchN instead of trying to figure
> out the vtable size. That simplifies the product-build a bit.
>
> I put my vtable sizing code in debug builds. It's not strictly portable,
> but it probably fail quickly on an esoteric c++ compiler so you can figure
> out what's wrong. It will also assert if you have added too many new
> virtual method to overflow the current limit of 150.
>
> Please reply your comments to the RFR e-mail thread.
>
>
Thank you for taking my suggestions, I will take a look at that RFR.

Thomas


> Thanks
> - Ioi
>
>
> Kind Regards, Thomas
>
>
>
>> Thanks
>> - Ioi
>>
>>
>> That way you make yourself a bit less dependent on compiler internals. I
>> agree that your approach is more elegant, though.
>>
>> Kind Regards, Thomas
>>
>> On Fri, Feb 24, 2017 at 4:55 AM, Ioi Lam <[hidden email]> wrote:
>>
>>>
>>>
>>> On 2/23/17 7:47 PM, Ioi Lam wrote:
>>>
>>>> Hi,
>>>>
>>>> I am working on https://bugs.openjdk.java.net/browse/JDK-8005165
>>>> (Remove
>>>> CPU-dependent code in self-patching vtables), I need a way find out the
>>>> size
>>>> of a C++ vtable. I ended up doing this:
>>>>
>>>>
>>>> // Objects of the Metadata types (such as Klass and ConstantPool) have
>>>> C++ vtables.
>>>> // (In GCC this is the field <Type>::_vptr, i.e., first word in the
>>>> object.)
>>>> //
>>>> // Addresses of the vtables and the methods may be different across JVM
>>>> runs,
>>>> // if libjvm.so is dynamically loaded at a different base address.
>>>> //
>>>> // To ensure that the Metadata objects in the CDS archive always have
>>>> the correct vtable:
>>>> //
>>>> // + at dump time:  we redirect the _vptr to point to our own vtables
>>>> inside
>>>> //                  the CDS image
>>>> // + at run time:   we clone the actual contents of the vtables from
>>>> libjvm.so
>>>> //                  into our own tables.
>>>> //
>>>> // To determine the size of the vtable for each type, we use the
>>>> following
>>>> // trick by declaring 2 subclasses:
>>>> //
>>>> //   class CppVtabTesterA: public InstanceKlass {
>>>> //          virtual int   last_virtual_method() {return 1;}
>>>> //   };
>>>> //   class CppVtabTesterB: public InstanceKlass {
>>>> //          virtual void* last_virtual_method() {return NULL};
>>>> //   };
>>>> //
>>>> // CppVtabTesterA and CppVtabTesterB's vtables have the following
>>>> properties:
>>>> // - Their size (N+1) is exactly one more than the size of
>>>> InstanceKlass's vtable (N)
>>>> // - The first N entries have are exactly the same as in
>>>> InstanceKlass's vtable.
>>>> // - Their last entry is different.
>>>> //
>>>> // So to determine the value of N, we just walk CppVtabTesterA and
>>>> CppVtabTesterB's tables
>>>> // and find the first entry that's different
>>>>
>>>>
>>>> Could anyone comment if this is acceptable? I know it's not 100%
>>>> portable (C++ doesn't
>>>> specify where to find the vtable, or what's inside), but my assumptions
>>>> is the same as
>>>> the existing code. I.e., _vptr is a pointer located at offset 0 of the
>>>> object, and it
>>>> points to a one-dimensional array.
>>>>
>>>> So at least it's not any worse than before?
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>> By the way, I first tried having only a single "tester" class and walk
>>> the vtable to look for &last_virtual_method, but the C++ compiler told me
>>> that taking the address of a non-static function is not allowed ..... so I
>>> ended up creating two tester classes and checking their differences.
>>>
>>>
>>>
>>>
>>
>>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR[S] 8005165 Platform-independent C++ vtables for CDS

Thomas Stüfe-2
In reply to this post by Ioi Lam
Hi Ioi,

I did not yet look closely at your change yet, just a small nit: To prevent
the copying SafeFetch coding from accidentally tripping over a real
"deadbeef" value which may be a valid part of the vtable - however
completely unlikely this is - one could call SafeFetch twice with two
different bad values,eg:

const intptr_t bad1 = intptr_t(0xdeadbeef);
const intptr_t bad2 = intptr_t(0xbeefdead);
if (SafeFetchN(ptr, bad1) == bad1 && SafeFetchN(ptr, bad2) == bad2) { ...
break out ... }

Kind Regards, Thomas


On Wed, Mar 1, 2017 at 3:25 AM, Ioi Lam <[hidden email]> wrote:

> https://bugs.openjdk.java.net/browse/JDK-8005165
> http://cr.openjdk.java.net/~iklam/jdk10/8005165-platform-ind
> ependent-cds-vtable.v02/
>
> Hi,
>
> This is the official review (follow up of the "Determining the size of C++
> vtables" thread on [hidden email]).
>
> 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
>
>
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR[S] 8005165 Platform-independent C++ vtables for CDS

Ioi Lam
In reply to this post by Ioi Lam
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, [hidden email] 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.
> 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 [hidden email]).
>>>>
>>>> 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
>>>>
>>>>
>>>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR[S] 8005165 Platform-independent C++ vtables for CDS

Ioi Lam
In reply to this post by Thomas Stüfe-2


On 3/1/17 2:22 AM, Thomas Stüfe wrote:

> Hi Ioi,
>
> I did not yet look closely at your change yet, just a small nit: To
> prevent the copying SafeFetch coding from accidentally tripping over a
> real "deadbeef" value which may be a valid part of the vtable -
> however completely unlikely this is - one could call SafeFetch twice
> with two different bad values,eg:
>
> const intptr_t bad1 = intptr_t(0xdeadbeef);
> const intptr_t bad2 = intptr_t(0xbeefdead);
> if (SafeFetchN(ptr, bad1) == bad1 && SafeFetchN(ptr, bad2) == bad2) {
> ... break out ... }
>
> Kind Regards, Thomas
>

Hi Thomas, thanks for the suggestion. Now the code looks like this:

       const intptr_t bad1 = intptr_t(0xdeadbeef);
       const intptr_t bad2 = intptr_t(0xbaadf00d);
       intptr_t num = SafeFetchN(&srcvtable[i], bad1);
       if ((num == bad1 && SafeFetchN(&srcvtable[i], bad2) == bad2)
           // || i > 120 /* uncomment this line to test */
           ) {
         _info->set_vtable_size(i-1);
         break;
       }

I've updated the webrev with this change and other recommendations by
Coleen:

http://cr.openjdk.java.net/~iklam/jdk10/8005165-platform-independent-cds-vtable.v03/

Thanks
- Ioi

>
> On Wed, Mar 1, 2017 at 3:25 AM, Ioi Lam <[hidden email]
> <mailto:[hidden email]>> 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 on [hidden email]
>     <mailto:[hidden email]>).
>
>     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
>
>
>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR[S] 8005165 Platform-independent C++ vtables for CDS

Jiangli Zhou
Hi Ioi,

This is great!

I think it might be more beneficial to not zero out the cloned vtables before writing into the archive. If the runtime libjvm.so is loaded at the same based address, then you could avoid re-cloning the vtables at runtime completely with just one quick comparison of the libjvm base address. In that case, the MD would be read-only also. That would improve both the memory and startup efficiency a bit further.
 883   // The vtable clones contain addresses of the current process.
 884   // We don't want to write these addresses into the archive.
 885   MetaspaceShared::zero_cpp_vtable_clones_for_writing();
Thanks,
Jiangli

> On Mar 2, 2017, at 7:40 PM, Ioi Lam <[hidden email]> wrote:
>
>
>
> On 3/1/17 2:22 AM, Thomas Stüfe wrote:
>> Hi Ioi,
>>
>> I did not yet look closely at your change yet, just a small nit: To prevent the copying SafeFetch coding from accidentally tripping over a real "deadbeef" value which may be a valid part of the vtable - however completely unlikely this is - one could call SafeFetch twice with two different bad values,eg:
>>
>> const intptr_t bad1 = intptr_t(0xdeadbeef);
>> const intptr_t bad2 = intptr_t(0xbeefdead);
>> if (SafeFetchN(ptr, bad1) == bad1 && SafeFetchN(ptr, bad2) == bad2) { ... break out ... }
>>
>> Kind Regards, Thomas
>>
>
> Hi Thomas, thanks for the suggestion. Now the code looks like this:
>
>      const intptr_t bad1 = intptr_t(0xdeadbeef);
>      const intptr_t bad2 = intptr_t(0xbaadf00d);
>      intptr_t num = SafeFetchN(&srcvtable[i], bad1);
>      if ((num == bad1 && SafeFetchN(&srcvtable[i], bad2) == bad2)
>          // || i > 120 /* uncomment this line to test */
>          ) {
>        _info->set_vtable_size(i-1);
>        break;
>      }
>
> I've updated the webrev with this change and other recommendations by Coleen:
>
> http://cr.openjdk.java.net/~iklam/jdk10/8005165-platform-independent-cds-vtable.v03/ <http://cr.openjdk.java.net/~iklam/jdk10/8005165-platform-independent-cds-vtable.v03/>
>
> Thanks
> - Ioi
>
>>
>> On Wed, Mar 1, 2017 at 3:25 AM, Ioi Lam <[hidden email] <mailto:[hidden email]><mailto:[hidden email] <mailto:[hidden email]>>> wrote:
>>
>>    https://bugs.openjdk.java.net/browse/JDK-8005165 <https://bugs.openjdk.java.net/browse/JDK-8005165>
>>    <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/~iklam/jdk10/8005165-platform-independent-cds-vtable.v02/>
>>    <http://cr.openjdk.java.net/%7Eiklam/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 on [hidden email] <mailto:[hidden email]>
>>    <mailto:[hidden email] <mailto:[hidden email]>>).
>>
>>    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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR[S] 8005165 Platform-independent C++ vtables for CDS

Ioi Lam
Hi Jiangli,

Thanks for the suggestion. I tried implemented the code like this:

   static intptr_t* clone_vtable(const char* name, intptr_t* p) {
     ....
     for (int i=0; i<n; i++) {
       intptr_t p = srcvtable[i];
       intptr_t q = dstvtable[i];
       if (p != q) {
         dstvtable[i] = p;
         if (i == 0) {
           tty->print_cr("Updated: " INTPTR_FORMAT " -> " INTPTR_FORMAT,
q, p);
         }
       }
     }

but it turns out with Address Space Layout Randomization, which is now
enabled on most (all?) OS for security reasons, libjvm.so/libjvm.dll
will always be loaded at a different address:

[run1] Updated: 0x00007f040f27e7dc -> 0x00007fdddf0ac7dc
[run2] Updated: 0x00007f040f27e7dc -> 0x00007fba31c367dc
[run3] Updated: 0x00007f040f27e7dc -> 0x00007f585342d7dc

Also, we are writing just a small amount of data (9600 bytes on 64-bit,
4800 bytes on 32-bit), so I think the unlikely space saving is not worth
this extra effort.

Zeroing the vtables inside the archive is also for security reasons. It
makes sure that even if the VM fails to initialize the vtables due to a
bug, we will never jump to an address that might contain unexpected code.

Thanks
- Ioi


On 3/3/17 4:05 PM, Jiangli Zhou wrote:

> Hi Ioi,
>
> This is great!
>
> I think it might be more beneficial to not zero out the cloned vtables
> before writing into the archive. If the runtime libjvm.so is loaded at
> the same based address, then you could avoid re-cloning the vtables at
> runtime completely with just one quick comparison of the libjvm base
> address. In that case, the MD would be read-only also. That would
> improve both the memory and startup efficiency a bit further.
>   883   // The vtable clones contain addresses of the current process.
>   884   // We don't want to write these addresses into the archive.
>   885   MetaspaceShared::zero_cpp_vtable_clones_for_writing();
> Thanks,
> Jiangli
>
>> On Mar 2, 2017, at 7:40 PM, Ioi Lam <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>>
>>
>> On 3/1/17 2:22 AM, Thomas Stüfe wrote:
>>> Hi Ioi,
>>>
>>> I did not yet look closely at your change yet, just a small nit: To
>>> prevent the copying SafeFetch coding from accidentally tripping over
>>> a real "deadbeef" value which may be a valid part of the vtable -
>>> however completely unlikely this is - one could call SafeFetch twice
>>> with two different bad values,eg:
>>>
>>> const intptr_t bad1 = intptr_t(0xdeadbeef);
>>> const intptr_t bad2 = intptr_t(0xbeefdead);
>>> if (SafeFetchN(ptr, bad1) == bad1 && SafeFetchN(ptr, bad2) == bad2)
>>> { ... break out ... }
>>>
>>> Kind Regards, Thomas
>>>
>>
>> Hi Thomas, thanks for the suggestion. Now the code looks like this:
>>
>>      const intptr_t bad1 = intptr_t(0xdeadbeef);
>>      const intptr_t bad2 = intptr_t(0xbaadf00d);
>>      intptr_t num = SafeFetchN(&srcvtable[i], bad1);
>>      if ((num == bad1 && SafeFetchN(&srcvtable[i], bad2) == bad2)
>>          // || i > 120 /* uncomment this line to test */
>>          ) {
>>        _info->set_vtable_size(i-1);
>>        break;
>>      }
>>
>> I've updated the webrev with this change and other recommendations by
>> Coleen:
>>
>> 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/>
>>
>> Thanks
>> - Ioi
>>
>>>
>>> On Wed, Mar 1, 2017 at 3:25 AM, Ioi Lam <[hidden email]
>>> <mailto:[hidden email]><mailto:[hidden email]>> 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/>
>>>    <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 [hidden email]
>>> <mailto:[hidden email]>
>>>    <mailto:[hidden email]>).
>>>
>>>    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
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR[S] 8005165 Platform-independent C++ vtables for CDS

Jiangli Zhou

> On Mar 3, 2017, at 9:43 PM, Ioi Lam <[hidden email]> wrote:
>
> Hi Jiangli,
>
> Thanks for the suggestion. I tried implemented the code like this:
>
>   static intptr_t* clone_vtable(const char* name, intptr_t* p) {
>     ....
>     for (int i=0; i<n; i++) {
>       intptr_t p = srcvtable[i];
>       intptr_t q = dstvtable[i];
>       if (p != q) {
>         dstvtable[i] = p;
>         if (i == 0) {
>           tty->print_cr("Updated: " INTPTR_FORMAT " -> " INTPTR_FORMAT, q, p);
>         }
>       }
>     }
>
> but it turns out with Address Space Layout Randomization, which is now enabled on most (all?) OS for security reasons, libjvm.so/libjvm.dll will always be loaded at a different address:
>
> [run1] Updated: 0x00007f040f27e7dc -> 0x00007fdddf0ac7dc
> [run2] Updated: 0x00007f040f27e7dc -> 0x00007fba31c367dc
> [run3] Updated: 0x00007f040f27e7dc -> 0x00007f585342d7dc
>
> Also, we are writing just a small amount of data (9600 bytes on 64-bit, 4800 bytes on 32-bit), so I think the unlikely space saving is not worth this extra effort.
>
> Zeroing the vtables inside the archive is also for security reasons. It makes sure that even if the VM fails to initialize the vtables due to a bug, we will never jump to an address that might contain unexpected code.

Ok.

Thanks for trying that out.

Jiangli

>
> Thanks
> - Ioi
>
>
> On 3/3/17 4:05 PM, Jiangli Zhou wrote:
>> Hi Ioi,
>>
>> This is great!
>>
>> I think it might be more beneficial to not zero out the cloned vtables before writing into the archive. If the runtime libjvm.so is loaded at the same based address, then you could avoid re-cloning the vtables at runtime completely with just one quick comparison of the libjvm base address. In that case, the MD would be read-only also. That would improve both the memory and startup efficiency a bit further.
>>  883   // The vtable clones contain addresses of the current process.
>>  884   // We don't want to write these addresses into the archive.
>>  885   MetaspaceShared::zero_cpp_vtable_clones_for_writing();
>> Thanks,
>> Jiangli
>>
>>> On Mar 2, 2017, at 7:40 PM, Ioi Lam <[hidden email] <mailto:[hidden email]>> wrote:
>>>
>>>
>>>
>>> On 3/1/17 2:22 AM, Thomas Stüfe wrote:
>>>> Hi Ioi,
>>>>
>>>> I did not yet look closely at your change yet, just a small nit: To prevent the copying SafeFetch coding from accidentally tripping over a real "deadbeef" value which may be a valid part of the vtable - however completely unlikely this is - one could call SafeFetch twice with two different bad values,eg:
>>>>
>>>> const intptr_t bad1 = intptr_t(0xdeadbeef);
>>>> const intptr_t bad2 = intptr_t(0xbeefdead);
>>>> if (SafeFetchN(ptr, bad1) == bad1 && SafeFetchN(ptr, bad2) == bad2) { ... break out ... }
>>>>
>>>> Kind Regards, Thomas
>>>>
>>>
>>> Hi Thomas, thanks for the suggestion. Now the code looks like this:
>>>
>>>      const intptr_t bad1 = intptr_t(0xdeadbeef);
>>>      const intptr_t bad2 = intptr_t(0xbaadf00d);
>>>      intptr_t num = SafeFetchN(&srcvtable[i], bad1);
>>>      if ((num == bad1 && SafeFetchN(&srcvtable[i], bad2) == bad2)
>>>          // || i > 120 /* uncomment this line to test */
>>>          ) {
>>>        _info->set_vtable_size(i-1);
>>>        break;
>>>      }
>>>
>>> I've updated the webrev with this change and other recommendations by Coleen:
>>>
>>> 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/>
>>>
>>> Thanks
>>> - Ioi
>>>
>>>>
>>>> On Wed, Mar 1, 2017 at 3:25 AM, Ioi Lam <[hidden email] <mailto:[hidden email]><mailto:[hidden email] <mailto:[hidden email]>>> wrote:
>>>>
>>>>    https://bugs.openjdk.java.net/browse/JDK-8005165 <https://bugs.openjdk.java.net/browse/JDK-8005165>
>>>>    <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/>
>>>>    <http://cr.openjdk.java.net/%7Eiklam/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 on [hidden email] <mailto:[hidden email]>
>>>>    <mailto:[hidden email] <mailto:[hidden email]>>).
>>>>
>>>>    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
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR[S] 8005165 Platform-independent C++ vtables for CDS

coleen.phillimore
In reply to this post by Ioi Lam

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, [hidden email] 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.

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
>>
>> 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 [hidden email]).
>>>>>
>>>>> 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
>>>>>
>>>>>
>>>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR[S] 8005165 Platform-independent C++ vtables for CDS

Ioi Lam


On 3/5/17 7:17 AM, [hidden email] 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, [hidden email] 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.

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 [hidden email]).
>>>>>>
>>>>>> 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
>>>>>>
>>>>>>
>>>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR[S] 8005165 Platform-independent C++ vtables for CDS

coleen.phillimore


On 3/5/17 5:35 PM, Ioi Lam wrote:

>
>
> On 3/5/17 7:17 AM, [hidden email] 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, [hidden email] 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 [hidden email]).
>>>>>>>
>>>>>>> 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
>>>>>>>
>>>>>>>
>>>>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR[S] 8005165 Platform-independent C++ vtables for CDS

Thomas Stüfe-2
In reply to this post by coleen.phillimore
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, <[hidden email]> 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-ind
>> ependent-cds-vtable.v03/
>>
>>
>> On 3/2/17 8:48 AM, [hidden email] 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-ind
>>> ependent-cds-vtable.v02/src/share/vm/memory/metaspaceShare
>>> d.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.
>
> 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-ind
>>> ependent-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-ind
>>>>>> ependent-cds-vtable.v02/
>>>>>> Hi,
>>>>>>
>>>>>> This is the official review (follow up of the "Determining the size
>>>>>> of C++ vtables" thread [hidden email]).
>>>>>>
>>>>>> 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
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>
>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR[S] 8005165 Platform-independent C++ vtables for CDS

Ioi Lam
Hi Thomas,

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.
>
I also notice on our current Solaris build the first slot in the vtable
is the RTTI pointer, not a function address. This slot is different in
the TesterA and TesterB classes, so I had to skip over it. I think this
is safe because Metadata has about 10 virtual methods, so skipping over
the first one should not cause any harm.

> 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?

I think for portability, perhaps we can add a porting layer so the
vtables can be copied in a different way? I think this is better than
patching each object at run time.

Also, I would suggest not adding such a layer now, but do it when such a
need actually arises.

Thanks
- Ioi

>
> Kind Regards, Thomas
>
> On Sun, Mar 5, 2017 at 4:17 PM, <[hidden email]
> <mailto:[hidden email]>> 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, [hidden email]
>         <mailto:[hidden email]> 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
>                         [hidden email]
>                         <mailto:[hidden email]>).
>
>                         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
>
>
>
>
>
>
>

12
Loading...