RFR[S] 8005165 Platform-independent C++ vtables for CDS
Jiangli Zhou
jiangli.zhou at oracle.com
Sat Mar 4 00:05:29 UTC 2017
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 <ioi.lam at oracle.com> 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 <ioi.lam at oracle.com <mailto:ioi.lam at oracle.com><mailto: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>
>> <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 hotspot-dev at openjdk.java.net <mailto:hotspot-dev at openjdk.java.net>
>> <mailto: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
More information about the hotspot-dev
mailing list