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

[Ioi Lam]

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 <ioi.lam at oracle.com 
> <mailto:ioi.lam at oracle.com>> 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 hotspot-dev at openjdk.java.net
>     <mailto:hotspot-dev at openjdk.java.net>).
>
>     The new code has the same assumption as the existing code in JDK
>     10: for a C++ object that contains virtual methods (e.g.,
>     ConstantPool), we assume the first intptr_t slot of the object is
>     a _vptr, which points to a vtable, which consists of no more than
>     150 intptr_t's.
>
>     ConstantPool*p -->[ _vptr    ] -------> [ vtable slot 0 ]
>                        [ field #0 ]          [ vtable slot 1 ]
>                        [ field #1 ]          [ vtable slot 2 ]
>                        [ field #2 ]          [ vtable slot 3 ]
>                        [ ....     ]          [ vtable slot 4]
>                                              [ vtable slot 5 ]
>                                              [ ...           ]
>
>     + In the existing code, we were pointing the vtable slots to
>       code that's generated by HotSpot.
>
>     + In the new code, we copy the vtable slots from an existing
>       vtable (generated by the C++ linker).
>
>     Per Thomas Stüfe's advice, I don't try to determine the size of
>     the vtable (as that would add one more compiler requirement where
>     new virtual methods added by a subclass must be placed at a higher
>     offset in the vtable).
>
>     Instead, I have added code in non-product builds to ensure that
>     the vtables are no longer than 150 entries. You can run with
>     "-XX:+PrintSharedSpaces -Xshare:dump" to print out the actual size
>     of the vtables for your particular platform:
>
>       ConstantPool has 12 virtual methods
>       InstanceKlass has 113 virtual methods
>       InstanceClassLoaderKlass has 113 virtual methods
>       InstanceMirrorKlass has 113 virtual methods
>       InstanceRefKlass has 113 virtual methods
>       Method has 12 virtual methods
>       ObjArrayKlass has 114 virtual methods
>       TypeArrayKlass has 114 virtual methods
>
>     As mentioned in the code comments, if you have an esoteric C++
>     compiler, the verify_sufficient_size() function will probably
>     fail, but hopefully that would give you some hints for porting
>     this code.
>
>     To avoid accidentally touching an unmapped page, the code uses
>     SafeFetchN for copying the vtable contents, and would shrink the
>     vtable to less than 150 entries if necessary. I can't test this
>     for real, but I've added some code to simulate an error:
>
>         for (int i=0; i<n; i++) {
>           const intptr_t bad = intptr_t(0xdeadbeef);
>           intptr_t num = SafeFetchN(&srcvtab[i], bad);
>           if (num == bad
>               // || i > 120 /* uncomment this line to test */
>               ) {
>             _info->set_vtab_size(i-1);
>             break;
>           }
>           dstvtab[i] = num;
>         }
>
>     Results:
>
>     + Removed 850 lines of CPU-dependent code
>
>     + CDS image is about 50K smaller
>
>     + Previously Metadata objects must live in the read-write section
>     in the CDS
>       archive, because their _vptr was updated at run time. Now _vptr
>     is no longer
>       updated, so ConstantPool can be moved to the read-only section
>     (see JDK-8171392).
>
>     Thanks
>     - Ioi
>
>
>
>