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

Coleen Phillimore coleen.phillimore at oracle.com
Thu Aug 15 13:31:50 PDT 2013


Looks good!

Thank you for all the adhoc test runs.   Also, thanks to SQE for running 
a special full PIT test run, and thanks to the performance team for 
running performance tests on this change also.

Coleen

On 08/15/2013 01:48 PM, harold seigel wrote:
> Hi,
>
> Please review this updated version of the fix for 8003424: 
> http://cr.openjdk.java.net/~hseigel/bug_8003424.4/ 
> <http://cr.openjdk.java.net/%7Ehseigel/bug_8003424.4/>
>
> Other than removing an obsolete comment from universe.cpp, only the 
> tests have been changed since the previous webrev.  A few test errors 
> were fixed, '@run main' tags were added, and the handling of cases 
> where CDS cannot be used has been improved.
>
> 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