Request for review: 8003424 - Enable Class Data Sharing for CompressedOops

harold seigel harold.seigel at oracle.com
Wed Aug 14 05:21:58 PDT 2013


Thanks,
Harold

On 8/14/2013 8:21 AM, Mikael Gerdin wrote:
> 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