Request for review: 8003424 - Enable Class Data Sharing for CompressedOops
Mikael Gerdin
mikael.gerdin at oracle.com
Wed Aug 14 01:39:04 PDT 2013
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