RFR (L): 8024545: make develop and notproduct flag values available in product builds

[Vladimir Kozlov]

Good.

Vladimir

On 9/19/13 4:08 PM, Christian Thalinger wrote:
> JPRT found some problems.  Apparently something does #define C2 'E' on Solaris so I had to prefix all kind flags with KIND_:
>
> +     // flag kind
> +     KIND_PRODUCT            = 1 << 4,
> +     KIND_MANAGEABLE         = 1 << 5,
> +     KIND_DIAGNOSTIC         = 1 << 6,
> +     KIND_EXPERIMENTAL       = 1 << 7,
> +     KIND_NOT_PRODUCT        = 1 << 8,
> +     KIND_DEVELOP            = 1 << 9,
> +     KIND_PLATFORM_DEPENDENT = 1 << 10,
> +     KIND_READ_WRITE         = 1 << 11,
> +     KIND_C1                 = 1 << 12,
> +     KIND_C2                 = 1 << 13,
> +     KIND_ARCH               = 1 << 14,
> +     KIND_SHARK              = 1 << 15,
> +     KIND_LP64_PRODUCT       = 1 << 16,
> +     KIND_COMMERCIAL         = 1 << 17,
>
> Two other problems found by Visual Studio were a bool return:
>
> + bool Flag::get_bool() const {
> +   if (is_constant_in_binary()) {
> +     return _value != 0;
> +   } else {
> +     return *((bool*) _addr);
> +   }
> + }
>
> and the size_t changes at the bottom of this file:
>
> http://cr.openjdk.java.net/~twisti/8024545/webrev/src/share/vm/runtime/globals.cpp.udiff.html
>
> Everything else (except the renames) are the same.  I tried to create an incremental webrev but I failed.  Sorry.
>
> On Sep 12, 2013, at 6:00 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>
>>
>> On Sep 12, 2013, at 5:24 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>
>>> On 9/12/13 5:11 PM, Christian Thalinger wrote:
>>>>
>>>> On Sep 12, 2013, at 4:30 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>>>
>>>>> Christian,
>>>>>
>>>>> Can you put _flags first to avoid padding between _addr and _value in 32-bit VM?
>>>>
>>>> Yes, good point.
>>>>
>>>>>
>>>>> It would be nice enumerate flag's type and use int instead of strings:
>>>>>
>>>>> enum {
>>>>> bool_flag = 1,
>>>>> intx_flag = 2,
>>>>>
>>>>> + #define RUNTIME_PRODUCT_FLAG_STRUCT(     type, name, value, doc) { type##_flag, XSTR(name), &name,              VALUE(value), NOT_PRODUCT_ARG(doc) Flag::Flags(Flag::DEFAULT | Flag::KIND_PRODUCT) },
>>>>
>>>> I was thinking of putting the type into the flags, too.  That would save another pointer word.  Should I give it a shot in this change or a separate one?
>>>
>>> Do it in separate changes after you push this.
>>> And if you remove _type field later you don't need to move _flags now.
>>> So you can push your current changes as it is. They are good.
>>
>> Thank you, Vladimir.  -- Chris
>>
>>>
>>> Thanks,
>>> Vladimir
>>>
>>>>
>>>> -- Chris
>>>>
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>> On 9/12/13 9:58 AM, Christian Thalinger wrote:
>>>>>>
>>>>>> On Sep 11, 2013, at 5:34 PM, David Holmes <david.holmes at oracle.com> wrote:
>>>>>>
>>>>>>> Hi Chris,
>>>>>>>
>>>>>>> There seems to be an awful lot "infrastructure" work needed to make something that sounds simple happen. :( All the changes to the scoping and the set/get methods tends to obscure the core change. My only suggestion here is that the "set" methods could perhaps factor this:
>>>>>>>
>>>>>>> +   if (is_constant_in_binary()) {
>>>>>>> +     fatal(err_msg("flag is constant: %s", _name));
>>>>>>>
>>>>>>> into a check_writable() method so that it isn't duplicated so much.
>>>>>>
>>>>>> Good point.  I made that change:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~twisti/8024545/webrev/src/share/vm/runtime/globals.cpp.udiff.html
>>>>>>
>>>>>>>
>>>>>>> I also wonder whether a ConstFlag sub/superclass would simplify this at all?
>>>>>>
>>>>>> Maybe it would but then we need more infrastructure to read the additional entries.  Might be a wash.  SA only knows about offsets in structs and the flags array is statically defined.
>>>>>>
>>>>>>>
>>>>>>> That aside I'm curious why the minimal VM size change is only 22K when client is 32K?
>>>>>>
>>>>>> The 32-bit product build also contains the server compiler.
>>>>>>
>>>>>> -- Chris
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>>
>>>>>>> On 11/09/2013 10:56 AM, Christian Thalinger wrote:
>>>>>>>> http://cr.openjdk.java.net/~twisti/8024545/webrev/
>>>>>>>>
>>>>>>>> 8024545: make develop and notproduct flag values available in product builds
>>>>>>>> Reviewed-by:
>>>>>>>>
>>>>>>>> Right now the internal flag table only contains flags which are defined in a product build. This does not include develop and notproduct flags. Sometimes it is useful to have access to these values for post-mortem core file analysis or to read these values for compiler settings for a Java-based compiler.
>>>>>>>>
>>>>>>>> This change enables develop and notproduct flag values to be read by the serviceability agent. The binary size is increased by 42k for a 64-bit product build and by 32k for a 32-bit product build.
>>>>>>>>
>>>>>>>> Before:
>>>>>>>>
>>>>>>>> $ java -cp /java/re/jdk/8/latest/binaries/linux-x64/lib/sa-jdi.jar sun.jvm.hotspot.CLHSDB 9399
>>>>>>>> Attaching to process 9399, please wait...
>>>>>>>> hsdb> flags -nd
>>>>>>>> InitialHeapSize = 495006528 5
>>>>>>>> MaxHeapSize = 7920943104 5
>>>>>>>> UseCompressedKlassPointers = true 5
>>>>>>>> UseCompressedOops = true 5
>>>>>>>> UseParallelGC = true 5
>>>>>>>> hsdb> flags InlineMathNatives
>>>>>>>> Couldn't find flag: InlineMathNatives
>>>>>>>>
>>>>>>>> After:
>>>>>>>>
>>>>>>>> $ java -cp $JAVA_HOME/lib/sa-jdi.jar sun.jvm.hotspot.CLHSDB 3726
>>>>>>>> Attaching to process 3726, please wait...
>>>>>>>> hsdb> flags -nd
>>>>>>>> InitialHeapSize = 495006528 5
>>>>>>>> MaxHeapSize = 7920943104 5
>>>>>>>> UseCompressedKlassPointers = true 5
>>>>>>>> UseCompressedOops = true 5
>>>>>>>> UseParallelGC = true 5
>>>>>>>> hsdb> flags InlineMathNatives
>>>>>>>> InlineMathNatives = true 0
>>>>>>>>
>>>>>>>> This patch has one behavioral difference; when printing flags with e.g. PrintFlagsFinal in a debug build it prints "develop" for develop flags:
>>>>>>>>
>>>>>>>>     uintx AdaptiveSizePolicyGCTimeLimitThreshold    = 5               {develop}
>>>>>>>>
>>>>>>>> The output for product builds is unchanged.
>>>>>>>>
>>>>>>
>>>>
>>
>