Request for review: 8003424 - Enable Class Data Sharing for CompressedOops
harold seigel
harold.seigel at oracle.com
Thu Aug 15 10:48:29 PDT 2013
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