Request for review: 8003424 - Enable Class Data Sharing for CompressedOops
Mikael Gerdin
mikael.gerdin at oracle.com
Wed Aug 14 05:21:29 PDT 2013
Harold,
On 2013-08-14 14:18, harold seigel wrote:
> 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);
> }
I see. I suspected that this was the case but it wasn't clear earlier. I
think that the comment is sufficient to communicate this.
/Mikael
>
> 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