Request for review: 8003424 - Enable Class Data Sharing for CompressedOops
Coleen Phillimore
coleen.phillimore at oracle.com
Mon Aug 5 12:21:13 PDT 2013
On 08/02/2013 04:31 PM, harold seigel wrote:
> Hi,
>
> Please review this updated webrev for bug 8003424:
> http://cr.openjdk.java.net/~hseigel/bug_8003424.2/
> <http://cr.openjdk.java.net/%7Ehseigel/bug_8003424.2/>
>
> The updated webrev has a lot of changes from the previous webrev,
> including the following:
>
> 1. Class metaspace information is now output when
> -XX:+PrintCompressedOopsMode is specified.
>
> 2. decode_klass_not_null(Register dst, Register src) no longer
> clobbers R12.
>
If decode_klass_not_null(src, dest) doesn't use r12, isn't the size
calculated by *instr_size_for_decode_klass_not_null* now wrong? Or is
this size calculation only valid for the case where you only have one
register? Or can this size be a max size?
http://cr.openjdk.java.net/~hseigel/bug_8003424.2/src/share/vm/memory/heap.cpp.frames.html
In this file can you spell out segm_r_aligned to something more
descriptive - like reserved_segment_alignment, reserved_segments_size,
committed_segments_size.
http://cr.openjdk.java.net/~hseigel/bug_8003424.2/src/share/vm/memory/metaspace.cpp.udiff.html
*+ if (PrintCompressedOopsMode || (PrintMiscellaneous && Verbose)) {*
*+ gclog_or_tty->print_cr("Narrow klass base: " PTR_FORMAT ", Narrow klass shift: " SIZE_FORMAT,*
I see this conditional comes from universe.cpp but I don't see why this
should be printed for the OR condition: PrintMiscellaneous && Verbose.
2941 #ifdef _LP64
2942 Universe::set_narrow_klass_base((address)_space_list->current_virtual_space()->bottom()
Put a comment why you do this. It's so UseCompressedKlassPointers
decoding "works" while you are creating the shared archive. The oops
are thrown away right now so it doesn't matter that the encoded value
uses a different base during dumping than it will use during
UseSharedSpaces. When we share String oops, we'll have to fix this
code but I don't know how to fix this yet. Maybe add code that always
requires that the CDS archive base is the lower of the two bases. Not
a problem with your code.
Maybe you should assert that UseCompressedOops and KlassPtrs are set
here though.
3012 if (using_class_space()) {
3013 _class_space_list = new VirtualSpaceList(rs);
3014 }
This function initialize_class_space() is always called when you are
using_class_space() so why this conditional? Shouldn't this also be
moved to 2917 under where it's called (or above) in the ifdef LP64 since
it's LP64 only now?
2557 if ((mdtype == Metaspace::ClassType) && !Metaspace::using_class_space()) {
2558 return 0;
2559 }
One way to shorten this expression that appears a few times, is to add
an inlined overloaded using_class_space(MetadataType).
This looks really good. I've been through it a few times and I don't
see any problems, other than these suggestions here.
Thanks!
Coleen
>
> 3. Method using_class_vsm() was renamed to using_class_space().
>
> 4. Type narrowKlass was added and replaces narrowOop, where
> appropriate.
>
> 5. The Metaspace:: qualifier was removed, where it was unnecessary.
>
> 6. Metaspace::initialize_class_space() was made private.
>
> 7. Method max_heap_for_compressed_oops(), in arguments.cpp, was
> updated.
>
> Performance tests were run by Jenny using specjvm2008, specjbb2005,
> and specjbb2013. The results showed no significant performance
> difference in terms of scores and gc behavior.
>
> Additional testing was done on Solaris Sparc and Linux x86 using
> SpecJBB2005 with -Xmx34g and -XX:ObjectAlignmentInBytes=16 & 32.
>
> Please let me know what additional tests should be run.
>
> Thanks!
> Harold
>
> On 7/24/2013 4:36 PM, Vladimir Kozlov wrote:
>> Hi Harold,
>>
>> > Usually the narrow_klass_base will be 32G and the
>> > LogKlassAlignmentInBytes will be 3 (8 bytes alignment) which means
>> that
>> > narrow_klass_base >> 3 will exceed maxint. So, we will need R12,
>> unless
>> > we do multiple addq(r, narrow_klass_base_shifted()). Does that
>> make sense?
>>
>> My bad, I miscalculated. But we have "perfect value" 0x800000000 and
>> I am tempted to use may be bts (bit set) to avoid R12 usage (assuming
>> or with check that class metaspace size < 32Gb).
>>
>> > Is there one prefix byte per instruction, or just for certain
>> instructions?
>>
>> "Not all instructions require a REX prefix in 64-bit mode. A prefix
>> is necessary only if an instruction references one of the extended
>> registers or uses a 64-bit operand."
>>
>> I want you also run tests with -XX:ObjectAlignmentInBytes=16 and =32
>> and big java heap. Recently we got several bugs which indicated that
>> we did not separate cleanly oops and klass pointers en/decoding.
>>
>> Thanks,
>> Vladimir
>>
>> On 7/24/13 11:35 AM, harold seigel wrote:
>>> Hi Vladimir,
>>>
>>> Thank you for the review! Please see inline comments.
>>>
>>> Thanks, Harold
>>>
>>> On 7/19/2013 7:56 PM, Vladimir Kozlov wrote:
>>>> On 7/18/13 3:03 PM, Vladimir Kozlov wrote:
>>>>> Nice clean up, Harold
>>>>>
>>>>> Could you add the size of class metaspace in output with
>>>>> PrintCompressedOopsMode (and with verbose). I don't want to
>>>>> remember an
>>>>> other very long flag name: TraceMetavirtualspaceAllocation.
>>> Will be done in next webrev.
>>>>>
>>>>> Also it would be nice to print these info line (and compressed
>>>>> oops info
>>>>> line) in hs_err files.
>>> I filed an enhancement bug for this: 8021264
>>> <https://jbs.oracle.com/bugs/browse/JDK-8021264>.
>>>>>
>>>>> About "effect(KILL cr); /// ???? is this KILL needed?" in
>>>>> x86_64.ad.
>>>>> encode_klass_not_null() and decode_klass_not_null() use arithmetic
>>>>> instructions which modify flags: subq, addq, shrq, xorptr. Also note
>>>>> that code in .ad file does not check the encoding mode for narrow
>>>>> klass/oop so it should be conservative and assume that arithmetic
>>>>> instructions are used.
>>> OK. Thanks.
>>>>>
>>>>> If Universe::narrow_klass_shift() == 0 you have klass base below
>>>>> 4Gb so
>>>>> the value is 32 bit. You can use it directly in narrow klass
>>>>> encoding/decoding without destroying R12.
>>>>
>>>> Correction. You can do it only if base < 2Gb max positive int (sign
>>>> bit is extended so you will get wrong result with address > 2gb).
>>> But the base will normally be 32Gb. So this case won't happen very
>>> often.
>>>>
>>>> You definitely should not use R12 in decode_klass_not_null(Register
>>>> dst, Register src) method where you have free 'dst' register.
>>> Will be fixed in next webrev.
>>>>
>>>> About CompressedKlassPointersBase 0x800000000 (32*G). Actually it
>>>> should depend on ObjectAlignmentInBytes. For =16 it should be 64Gb.
>>>> The idea was to avoid using R12 by using shifted base:
>>>>
>>>> decode:
>>>> if (Universe::narrow_klass_base_shifted() != 0) {
>>>> if (Universe::set_narrow_klass_shift() == 0) {
>>>> shrq(r, LogKlassAlignmentInBytes);
>>>> }
>>>> addq(r, Universe::narrow_klass_base_shifted());
>>>> shlq(r, LogKlassAlignmentInBytes);
>>>> }
>>>>
>>>> Universe::narrow_klass_base_shifted() is set only if
>>>> (Universe::narrow_klass_base >> LogKlassAlignmentInBytes) <= maxint
>>>> (max positive int).
>>> Usually the narrow_klass_base will be 32G and the
>>> LogKlassAlignmentInBytes will be 3 (8 bytes alignment) which means that
>>> narrow_klass_base >> 3 will exceed maxint. So, we will need R12, unless
>>> we do multiple addq(r, narrow_klass_base_shifted()). Does that make
>>> sense?
>>>>
>>>> And you have general memory reservation problem. At least on Solaris,
>>>> as I remember, OS will always try to keep protected 4Kb pages between
>>>> different requested memory regions. That is why in jdk7 we first
>>>> reserve one huge space and then cut it on java heap, perm gen and
>>>> protection page below heap to catch implicit NULL exceptions with
>>>> compressed oops.
>>>>
>>>> So, please, verify that you are getting 'cds_address + cds_total'
>>>> address or CompressedKlassPointersBase without CDS and with compressed
>>>> oops and with Xmx20Gb.
>>> I verifed that we get either cds_address+cds_total as the address, or
>>> CompressedKlassPointersBase as the address without CDS on Linux,
>>> Windows
>>> 7, Mac OS, and Solaris 5.11. This is true with default heap sizes and
>>> -Xmx32G.
>>>
>>> On Solaris 5.10, I don't get CompressedKlassPointersBase or the desired
>>> CDS address for class metaspace regardless of heap size.
>>>>
>>>>>
>>>>> instr_size_for_decode_klass_not_null() is ugly but unfortunately we
>>>>> can't do other way. I assume you use max size because depending on
>>>>> registers used in instructions you will or will not get prefix byte
>>>>> (r8-r15).
>>> Is there one prefix byte per instruction, or just for certain
>>> instructions?
>>>>>
>>>>> I will look on changes in more details may be late.
>>>>
>>>> What 'vsm' stands for in using_class_vsm()? Don't use abbreviations
>>>> which are not obvious.
>>> I changed using_class_vsm() to using_class_space().
>>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>> On 7/18/13 10:54 AM, harold seigel wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review this fix for bug 8003424.
>>>>>>
>>>>>> Open webrev at http://cr.openjdk.java.net/~hseigel/bug_8003424/
>>>>>> <http://cr.openjdk.java.net/%7Ehseigel/bug_8003424/>
>>>>>> Open bug links at:
>>>>>>
>>>>>> http://bugs.sun.com/view_bug.do?bug_id=8003424
>>>>>> http://bugs.sun.com/view_bug.do?bug_id=8016729
>>>>>> http://bugs.sun.com/view_bug.do?bug_id=8011610
>>>>>>
>>>>>> JBS Bug links at
>>>>>>
>>>>>> https://jbs.oracle.com/bugs/browse/JDK-8003424
>>>>>> https://jbs.oracle.com/bugs/browse/JDK-8016729
>>>>>> https://jbs.oracle.com/bugs/browse/JDK-8011610
>>>>>>
>>>>>>
>>>>>> This fix provides support for using compressed klass pointers
>>>>>> with CDS.
>>>>>> It also changes the class metaspace allocation on 64-bit
>>>>>> platforms when
>>>>>> UseCompressedKlassPointers is true. Instead of allocating the class
>>>>>> metaspace as part of the Java Heap allocation and locating it at the
>>>>>> base of that allocation, the metaspace will now be allocated
>>>>>> separately,
>>>>>> above the Java heap. This will enable future expansion of the
>>>>>> metaspace
>>>>>> because it won't be backed up against the Java heap. If CDS is
>>>>>> being
>>>>>> used, then the CDS region will be allocated between the Java heap
>>>>>> and
>>>>>> the metaspace.
>>>>>>
>>>>>> The new class metaspace allocation code tries to allocate memory at
>>>>>> 32G,
>>>>>> or above the CDS region, if it is present. Because of this,
>>>>>> encoding
>>>>>> and decoding of compressed klass pointers will always require use
>>>>>> of a
>>>>>> base register. So, encoding and decoding of compressed klass
>>>>>> pointers
>>>>>> will differ from compressed oops.
>>>>>>
>>>>>> There are no class metaspace allocation changes if
>>>>>> UseCompressedKlassPointers is turned off or if running on a 32-bit
>>>>>> platform.
>>>>>>
>>>>>> The code changes also include some cleanup and will fix bugs
>>>>>> 8016729,
>>>>>> 8011610, and 8003424.
>>>>>>
>>>>>>
>>>>>> Many of the modules in this webrev contain a lot of changes. Modules
>>>>>> macroAssembler_sparc.cpp and macroAssembler_x86.cpp were changed to
>>>>>> support the new encoding and decoding of compressed klass pointers.
>>>>>>
>>>>>> Module metaspace.cpp was changed significantly to support the new
>>>>>> allocation for metaspace and the CDS region and to determine
>>>>>> compressed
>>>>>> klass pointer encoding base and shifting values.
>>>>>>
>>>>>> Most of the changes to module universe.cpp were to remove code
>>>>>> related
>>>>>> to allocating metaspace and remove code that considered compressed
>>>>>> klass
>>>>>> pointers when determining the compressed oops compression mechanism.
>>>>>>
>>>>>> Modules klass.inline.hpp and oop.inline.hpp were changed as part
>>>>>> of a
>>>>>> cleanup requested by Coleen.
>>>>>>
>>>>>>
>>>>>> These changes were tested with JCK Lang and VM tests, JTReg tests,
>>>>>> JPRT,
>>>>>> GCBasher, refworkload, ute vm.quick.testlist and vm.mlvm.testlist
>>>>>> tests. Most of the test were run with -Xshare:on and -Xshare:off
>>>>>> (with
>>>>>> and without CDS), and were run on Solaris Sparc, 32-Bit Linux and
>>>>>> 64-Bit
>>>>>> Linux. Jtreg tests were run on Windows 7 and Mac OS. JCK tests were
>>>>>> run on Solaris x86.
>>>>>>
>>>>>> The performance impact of these changes is TBD.
>>>>>>
>>>>>> Thanks, Harold
>>>>>>
>>>>>>
>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130805/6915d2e3/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list