Request for review: 8003424 - Enable Class Data Sharing for CompressedOops
Vladimir Kozlov
vladimir.kozlov at oracle.com
Thu Aug 15 11:28:18 PDT 2013
Good.
Thanks,
Vladimir
On 8/15/13 10:48 AM, 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