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

[Thomas Stüfe]

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 <ioi.lam at oracle.com> 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 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
>
>
>
>