Request for review: 8003424 - Enable Class Data Sharing for CompressedOops
Vladimir Kozlov
vladimir.kozlov at oracle.com
Thu Aug 8 14:37:12 PDT 2013
On 8/7/13 8:34 AM, harold seigel wrote:
> Hi Vladimir,
>
> On 8/5/2013 6:59 PM, Vladimir Kozlov wrote:
>> Harold,
>>
>> Note, on SPARC set() could generate up to 8 instructions to load
>> 64-bit constant into register. So I am not sure that it will be faster
>> than load.
> We thought it would be faster because it doesn't need to load anything
> from memory.
For good value (0x800000000) it should use only 2-3 instructions.
I think you can keep using set().
>>
>> I can't find where in CDS you store information about compressed klass
>> pointers and compressed oops parameters which were used during dump?
>> How you verify that correct parameters/flags are ussed when such CDS
>> is used later?
> No compressed klass pointers nor compressed oops get written to the
> archive. So, the compressed klass pointers and compressed oops
> parameters do not need to be recorded in the archive.
So you are saying that all klass pointers (references to klasses) in
metadata structures in metaspace are full. And klass pointers are only
compressed in java object headers (which are not part of CDS). Right?
And you don't care about UseCompressedOops and
UseCompressedKlassPointers flags settings when you dump it into archive.
The only limit is:
Metaspace::class_metaspace_size() < (uint64_t)(max_juint) - cds_total
Then I don't understand why you can't use/load CDS archive when coop or
compklass are off:
> If coop is turned off then CDS cannot be used. CDS will only be
> supported if both UseCompressedOops and UseCompressedKlassPointers are
> TRUE.
Also based on code in arguments.cpp it is allowed to specify
-XX:+UseCompressedOops -XX:-UseCompressedKlassPointers on command line:
1466 // Turn on UseCompressedKlassPointers too
1467 if (FLAG_IS_DEFAULT(UseCompressedKlassPointers)) {
1468 FLAG_SET_ERGO(bool, UseCompressedKlassPointers, true);
1469 }
Did you tested such combination?
>>
>> set_narrow_klass_base_and_shift() and
>> can_use_cds_with_metaspace_addr() have the same code for
>> UseSharedSpaces. It would be nice to have only one copy. Call
>> can_use_cds_with_metaspace_addr() from
>> set_narrow_klass_base_and_shift() ?
> I could add new inlined methods that determine the higher_address and
> lower_base when UseSharedSpaces is true and have them called from
> set_narrow_klass_base_and_shift() and
> can_use_cds_with_metaspace_addr(). Would that be worth doing?
I tried several approaches but your implementation is more clean. So I
agree to keep what you have now.
Also I found strange assert which should always fail (old code in
global_initialize() in metaspace.cpp):
2959 if (UseSharedSpaces) {
...
2969 } else {
2970 assert(!mapinfo->is_open() && !UseSharedSpaces,
2971 "archive file not closed or shared spaces not
disabled.");
Thanks,
Vladimir
>>
>> I see that you unconditionally set comp klass ptr base and shift for
>> DumpSharedSpaces. Should you do the check similar to
>> can_use_cds_with_metaspace_addr() to find if you can do that? You
>> don't even check UseCompressedKlassPointers.
> I don't think the checks are needed because the information written to
> the archive is not affected by the size of the class metaspace.
>
> Also, I recently added code to arguments.cpp that will generate an error
> if UseCompressedOops or UseCompressedKlassPointers is turned off and
> DumpSharedSpaces is on.
>>
>> Why you have next limitation? What if coop can't be used because of
>> big heap?:
>>
>> + // UseCompressedOops and UseCompressedKlassPointers must be on
>> for UseSharedSpaces.
>> + if (!UseCompressedOops || !UseCompressedKlassPointers) {
>> + no_shared_spaces();
> If coop is turned off then CDS cannot be used. CDS will only be
> supported if both UseCompressedOops and UseCompressedKlassPointers are
> TRUE.
>>
>> Could you move UseCompressedKlassPointers code in arguments.cpp into
>> separate method similar to set_use_compressed_oops()?
> Done.
>>
>> About new tests.
>> The first test in CDSCompressedKPtrsError misses
>> -XX:+UseCompressedOops flag.
> Fixed.
>>
>> Could you also test cross flags combinations?:
>>
>> -XX:-UseCompressedKlassPointers -XX:+UseCompressedOops
>> -XX:+UseCompressedKlassPointers -XX:-UseCompressedOops
> Done.
>>
>> It would be nice to have test to check ClassMetaspaceSize value range.
>> You have limited it to [1Mb, 3Gb]. BTW, why 3Gb? It is neither maxint
>> or maxuint.
> A member of our runtime SQE group is writing additional tests. I'll ask
> him to write a ClassMetaspaceSize range test.
>
> We chose 3Gb because we thought it was a sufficiently large size.
>>
>>
>> About code style, indention. We align next line to corresponding part
>> of previous line if we need to split a line. And sometimes it is
>> better to keep one long line. I understand some of them were before
>> your changes but, please, clean up at lest ones you touched.
> Done.
>>
>> Cases in metaspace.cpp:
>>
>> 2263 assert(raw_word_size >= min_size,
>> 2264 err_msg("Should not deallocate dark matter " SIZE_FORMAT "<"
>> SIZE_FORMAT, word_size, min_size));
>>
>>
>> 2485 if (Metaspace::using_class_space() &&
>> 2486 (Metaspace::class_space_list() != NULL)) {
>>
>>
>> 2574 size_t reserved = (mdtype == Metaspace::ClassType) ?
>> 2575 (Metaspace::using_class_space() ?
>> 2576 Metaspace::class_space_list()->virtual_space_total() : 0) :
>> 2577 Metaspace::space_list()->virtual_space_total();
>>
>>
>> 2694 out->print_cr(" class: " SIZE_FORMAT " specialized(s) "
>> SIZE_FORMAT ", "
>> 2695 SIZE_FORMAT " small(s) " SIZE_FORMAT ", "
>> 2696 SIZE_FORMAT " medium(s) " SIZE_FORMAT
>> ", "
>> 2697 "large count " SIZE_FORMAT,
>> 2698 cls_specialized_count, cls_specialized_waste,
>> 2699 cls_small_count, cls_small_waste,
>> 2700 cls_medium_count, cls_medium_waste,
>> cls_humongous_count);
>>
>>
>> 2872 while (!metaspace_rs.is_reserved() && (addr + 1*G > addr) &&
>> 2873 can_use_cds_with_metaspace_addr(addr + 1*G, cds_base)) {
>>
>>
>> 2889 vm_exit_during_initialization(err_msg(
>> 2890 "Could not allocate metaspace: %d bytes",
>> 2891 class_metaspace_size()));
>>
>>
>> 3107 return using_class_space() ?
>> 3108 class_vsm()->sum_used_in_chunks_in_use() : 0;
>>
>>
>> 3157 if (is_class && using_class_space()) {
>> 3158 class_vsm()->deallocate(ptr, word_size);
>>
>>
>> 3305 return space_list()->contains(ptr) ||
>> 3306 (using_class_space() && class_space_list()->contains(ptr));
>>
>> metaspace.hpp:
>>
>> 270 return _allocated_capacity_words[Metaspace::NonClassType] +
>> 271 (Metaspace::using_class_space() ?
>> 272 _allocated_capacity_words[Metaspace::ClassType] : 0);
>>
>> 285 return _allocated_used_words[Metaspace::NonClassType] +
>> 286 (Metaspace::using_class_space() ?
>> 287 _allocated_used_words[Metaspace::ClassType] : 0);
>>
>> universe.cpp:
>>
>> 849 assert((intptr_t)Universe::narrow_oop_base() <=
>> (intptr_t)(Universe::heap()->base() -
>> 850 os::vm_page_size()) ||
>> 851 Universe::narrow_oop_base() == NULL, "invalid value");
>>
>>
>> Thanks,
>> Vladimir
>>
>> On 8/2/13 1: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.
>>>
>>> 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
>>>>>>>>
>>>>>>>>
>>>>>
>>>
>
More information about the hotspot-runtime-dev
mailing list