RFR (L): 8024545: make develop and notproduct flag values available in product builds
Christian Thalinger
christian.thalinger at oracle.com
Thu Sep 26 11:00:49 PDT 2013
On Sep 26, 2013, at 9:26 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
> Christian,
>
> It will make reviewers life much easier if you start using webrev versions instead of always replacing one. It is last time I will review such changes without version.
Alright. You convinced me :-)
>
> It is passed JPRT so I hope it is final iteration. It looks fine.
Thank you.
>
> Vladimir
>
> On 9/25/13 2:17 PM, Christian Thalinger wrote:
>> Oh well. It turned out that SunStudio cannot handle strings properly in the static table. So I chose another approach which actually uses less memory: for develop and notproduct flags define both a const variable and a static variable with CONST_ as a prefix. The address of the latter is then used in the flags table. No need for an additional _value field.
>>
>> With that the binary size increase is:
>>
>> linux_i486_compiler2: 24k
>> linux_i486_minimal1: 17k
>>
>> I hope this is the last iteration:
>>
>> http://cr.openjdk.java.net/~twisti/8024545/webrev/
>>
>> The gist of this iteration is at the bottom of this page:
>>
>> http://cr.openjdk.java.net/~twisti/8024545/webrev/src/share/vm/runtime/globals.hpp.udiff.html
>>
>> On Sep 19, 2013, at 6:34 PM, Christian Thalinger <christian.thalinger at oracle.com> wrote:
>>
>>> Thank you, again, Vladimir.
>>>
>>> On Sep 19, 2013, at 4:51 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>>
>>>> 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.
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>
>>
More information about the hotspot-dev
mailing list