RFR (L): 8024545: make develop and notproduct flag values available in product builds
Vladimir Kozlov
vladimir.kozlov at oracle.com
Thu Sep 26 09:26:58 PDT 2013
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.
It is passed JPRT so I hope it is final iteration. It looks fine.
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