Request for review: 8003424 - Enable Class Data Sharing for CompressedOops
harold seigel
harold.seigel at oracle.com
Wed Aug 14 05:18:20 PDT 2013
Hi Mikael,
Thanks for the review.
In test CDSCompressedKPtrs.java, RuntimeException gets thrown by
output.shouldContain(), if it does not find the specified text in the
test's output. In this test, it may not find "sharing" in the test
output if the JVM was unable to compatibly allocate the class
metaspace. If the class metaspace does not get allocated near the CDS
region then the JVM turns off CDS, disabling sharing. Since this is a
valid execution path, it shouldn't cause the test to fail.
I've added a comment and changed the test a bit to try and make it clearer:
} catch (RuntimeException e) {
// Report 'passed' if CDS was turned off because we could not
allocate
// the klass metaspace at an address that would work with CDS.
output.shouldContain("Could not allocate metaspace at a
compatible address");
output.shouldHaveExitValue(1);
}
Thanks, Harold
On 8/14/2013 4:39 AM, Mikael Gerdin wrote:
> Harold,
>
> On 2013-08-09 16:57, Harold Seigel wrote:
>> Hi,
>>
>> Please review this latest version of the bug fix for 8003424:
>> http://cr.openjdk.java.net/~hseigel/bug_8003424.3/
>>
>>
>> This new version includes the following changes:
>>
>> 1. macroAssembler_sparc.cpp
>>
>> a. Merged two versions of reinit_heapbase() into one.
>>
>> b. Improved decode_klass_not_null(src, dst) to not use G6 if
>> shift == 0.
>>
>>
>> 2. macroAssembler_x86.cpp
>>
>> a. Merged two versions of reinit_heapbase() into one.
>>
>> b. Improved encode_klass_not_null(src, dst) to not use R12.
>>
>> c. Replaced leaq with addq in decode_klass_not_null(src, dst).
>>
>>
>> 3. Improved variable names in heap.cpp.
>>
>>
>> 4. metaspace.cpp
>>
>> a. Rewrote MetaspaceAux::reserved_in_bytes() to make it more
>> understandable.
>>
>> b. Moved set_narrow_klass_base_and_shift() near
>> can_use_cds_with_metaspace_addr().
>>
>> c. Changed unneeded conditional in initialize_class_space() into an
>> assert.
>>
>>
>> 5. arguments.cpp
>>
>> a. Fixed problem with -Xshare:dump and disabled compression.
>>
>> b. Moved UseCompressedKlassPointers code into new function:
>> set_use_compressed_klass_ptrs().
>>
>> c. Fixed bug 8005933 that turned off CDS for -server even if
>> -Xshare:auto was explicitly specified.
>>
>>
>> 6. Increased default value of ClassMetaspaceSize to 1*G in globals.hpp.
>>
>>
>> 7. Fixed indentation issues in metaspace.cpp and other modules.
>
> The VM changes look good to me.
>
> I have a question about the test CDSCompressedKPtrs.java
>
> What RuntimeException do you expect and why is it ok that we can't use
> the shared archive if you get such an exception?
>
> I think at least a comment about why the test is supposed to pass even
> if we can't use the shared archive.
>
> /Mikael
>
>>
>>
>> Thanks, Harold
>>
>> ----- Original Message -----
>> From: harold.seigel at oracle.com
>> To: coleen.phillimore at oracle.com
>> Cc: hotspot-runtime-dev at openjdk.java.net
>> Sent: Friday, August 9, 2013 8:44:29 AM GMT -05:00 US/Canada Eastern
>> Subject: Re: Request for review: 8003424 - Enable Class Data Sharing for
>> CompressedOops
>>
>> The purpose of this assert is to verify that the two methods in the 'if'
>> clause closed the map file and disabled UseSharedSpaces if they detected
>> a problem when trying to use CDS.
>>
>> if (mapinfo->initialize() &&
>> MetaspaceShared::map_shared_spaces(mapinfo)) {
>> FileMapInfo::set_current_info(mapinfo);
>> } else {
>> assert(!mapinfo->is_open() && !UseSharedSpaces,
>> "archive file not closed or shared spaces not
>> disabled.");
>> }
>>
>> ----- Original Message -----
>> From: harold.seigel at oracle.com
>> To: coleen.phillimore at oracle.com
>> Cc: hotspot-runtime-dev at openjdk.java.net
>> Sent: Friday, August 9, 2013 8:29:59 AM GMT -05:00 US/Canada Eastern
>> Subject: Re: Request for review: 8003424 - Enable Class Data Sharing for
>> CompressedOops
>>
>> This is a bug that Stefan Karlsson also discovered. To fix it, I've
>> changed the code to this:
>>
>> if (DumpSharedSpaces) {
>> if (RequireSharedSpaces) {
>> warning("cannot dump shared archive while using shared archive");
>> }
>> UseSharedSpaces = false;
>> #ifdef _LP64
>> if (!UseCompressedOops || !UseCompressedKlassPointers) {
>> vm_exit_during_initialization(
>> "Cannot dump shared archive when UseCompressedOops or
>> UseCompressedKlassPointers is off.", NULL);
>> }
>>
>> Harold
>>
>>
>> ----- Original Message -----
>> From: coleen.phillimore at oracle.com
>> To: hotspot-runtime-dev at openjdk.java.net
>> Sent: Thursday, August 8, 2013 7:08:15 PM GMT -05:00 US/Canada Eastern
>> Subject: Re: Request for review: 8003424 - Enable Class Data Sharing for
>> CompressedOops
>>
>>
>> Yes, there should be a check for that too. Now I'll really let Harold
>> answer.
>>
>> Thanks,
>> Coleen
>>
>> On 08/08/2013 06:55 PM, Vladimir Kozlov wrote:
>> > Coleen, Harold
>> >
>> > > The InstanceKlass has offsets to fields in the instance, and they
>> > depend
>> > > on both compressed oops and compressed klass pointers. So both
>> > have to
>> > > be on for the offsets to be valid.
>> >
>> > So there is dependency on UseCompressedOops and
>> > UseCompressedKlassPointers flags during dump.
>> >
>> > Then why the check is done only for UseSharedSpaces and not for
>> > DumpSharedSpaces?:
>> >
>> > if (DumpSharedSpaces) {
>> > if (RequireSharedSpaces) {
>> > warning("cannot dump shared archive while using shared
>> archive");
>> > }
>> > UseSharedSpaces = false;
>> > + #ifdef _LP64
>> > + } else {
>> > + // UseCompressedOops and UseCompressedKlassPointers must be on
>> > for UseSharedSpaces.
>> > + if (!UseCompressedOops || !UseCompressedKlassPointers) {
>> > + no_shared_spaces();
>> > + }
>> > + #endif
>> > }
>> >
>> > Thanks,
>> > Vladimir
>> >
>> > On 8/8/13 3:34 PM, Coleen Phillimore wrote:
>> >>
>> >> Hi Vladimir,
>> >>
>> >> I have a couple of answers and I'll let Harold answer the rest.
>> >>
>> >> On 08/08/2013 05:37 PM, Vladimir Kozlov wrote:
>> >>> 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.
>> >>
>> >> Yes, they are. (methodOops weren't in the
>> InstanceKlass::_methods array
>> >> with Permgen but they are uncompressed now).
>> >>
>> >>> And klass pointers are only compressed in java object headers
>> (which
>> >>> are not part of CDS). Right?
>> >>
>> >> The InstanceKlass has offsets to fields in the instance, and they
>> depend
>> >> on both compressed oops and compressed klass pointers. So both
>> have to
>> >> be on for the offsets to be valid.
>> >>
>> >> But the base and compressed values are not stored anywhere in the
>> CDS
>> >> archive.
>> >>
>> >>> 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?
>> >>
>> >> I should let Harold answer this but I believe this is what his test
>> >> does. For CDS on 64 bit, both must be on. We didn't want to
>> create 4
>> >> different archives. And it wouldn't make too much sense since
>> CDS for
>> >> 64 bit is a footprint savings so why would you use it without
>> >> compressing oops and class pointers? It's not a startup savings on
>> >> server because we turn off interpreter bytecode quickening.
>> >>
>> >> Coleen
>> >>
>> >>>
>> >>>>>
>> >>>>> 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