Request for review: 8009778: NPG: ClassMetaspaceSize is used before set in set_ergonomics_flags()

[harold seigel]

Hi Yumin,

I don't think we want to do the FLAG_SET_ERGO() call if the flag is not 
the default because we don't want to say that we are setting 
ClassMetaspaceSize for ergonomic reasons in the non-default case.

Instead, I could add another function that just returns the default 
value for ClassMetaspaceSize (100*M) and doesn't do the IS_DEFAULT 
check.  This new method could then be called by both 
class_metaspace_size_for_compressed_ptrs() and by the FLAG_SET_ERGO() 
code when FLAG_IS_DEFAULT is true.

Does that sound ok?

Thanks, Harold

On 5/30/2013 4:18 PM, Yumin Qi wrote:
> Harold,
>
> (I am not a reviewer)
> *** 1461,1476 ****
>        if (UseCompressedKlassPointers) {
>          if (ClassMetaspaceSize > KlassEncodingMetaspaceMax) {
>            warning("Class metaspace size is too large for UseCompressedKlassPointers");
>            FLAG_SET_DEFAULT(UseCompressedKlassPointers, false);
>          } else if (FLAG_IS_DEFAULT(ClassMetaspaceSize)) {
> !         // 100,000 classes seems like a good size, so 100M assumes around 1K
> !         // per klass.   The vtable and oopMap is embedded so we don't have a fixed
> !         // size per klass.   Eventually, this will be parameterized because it
> !         // would also be useful to determine the optimal size of the
> !         // systemDictionary.
> !         FLAG_SET_ERGO(uintx, ClassMetaspaceSize, 100*M);
>          }
>        }
>      }
>      // Also checks that certain machines are slower with compressed oops
>      // in vm_version initialization code.
> --- 1470,1480 ----
>        if (UseCompressedKlassPointers) {
>          if (ClassMetaspaceSize > KlassEncodingMetaspaceMax) {
>            warning("Class metaspace size is too large for UseCompressedKlassPointers");
>            FLAG_SET_DEFAULT(UseCompressedKlassPointers, false);
>          } else if (FLAG_IS_DEFAULT(ClassMetaspaceSize)) {
> !         FLAG_SET_ERGO(uintx, ClassMetaspaceSize, class_metaspace_size_for_compressed_ptrs());
>          }
>        }
>      }
>
>     The ClassMetasoaceSize is default (2MB) so we call class_metaspace_size_for_compressed_ptrs(), but in this new function:
>
> + // Return the eventual class metaspace size if compressed klass ptrs are used.
> + inline uintx class_metaspace_size_for_compressed_ptrs() {
> +   // 100,000 classes seems like a good size, so 100M assumes around 1K per
> +   // klass.   The vtable and oopMap is embedded so we don't have a fixed size
> +   // per klass.   Eventually, this will be parameterized because it would
> +   // also be useful to determine the optimal size of the systemDictionary.
> +   return FLAG_IS_DEFAULT(ClassMetaspaceSize) ? 100*M : ClassMetaspaceSize;
> + }
> +
>
> There seems repeat to check if  ClassMetaspaceSize is default. Could we just do
>
> !         FLAG_SET_ERGO(uintx, ClassMetaspaceSize, (FLAG_IS_DEFAULT(ClassMetaspaceSize) ? 100*M : ClassMetaspaceSize));
>
> After this new function called, ClassMetaspaceSize is set to either 100MB or value from vmoption. Then in max_heap_for_compressed_oops():
>
> Do we need to change the code to use the new function?
>
> Thanks
> Yumin
>
>
>   
> On 5/30/2013 11:05 AM, harold seigel wrote:
>> Hi,
>>
>> Please review this fix for bug 8009778.
>>
>> The fix adds a function, class_metaspace_size_for_compressed_ptrs(), 
>> that returns what the value will be for ClassMetaspaceSize if 
>> CompressedKlassPointers are used.  This enables the correct value for 
>> ClassMetaspaceSize to be used by max_heap_for_compressed_oops() 
>> before ClassMetaspaceSize gets set in set_ergonomics_flags().
>>
>> The fix was tested with JCK lang and vm tests, UTE vm.quick.testlist 
>> and vm.mlvm.testlist tests, JPRT, and jtreg tests. Tests were run on 
>> Linux and Solaris. The debugger was used to step through the code to 
>> ensure that the fix behaved as expected.
>>
>> Open webrev at http://cr.openjdk.java.net/~hseigel/bug_8009778/ 
>> <http://cr.openjdk.java.net/%7Ehseigel/bug_8009778/>
>>
>> Bug link at http://bugs.sun.com/view_bug.do?bug_id=8009778
>>
>> JBS bug link at https://jbs.oracle.com/bugs/browse/JDK-8009778
>>
>> Thanks, Harold
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130530/e3113c18/attachment-0001.html