Re: RFR (XS): 8007901 SA: Don't read flag values as constants
I choose to follow the same pattern as for other flags, although I agree that
the extra accessor isn't really needed.
Thanks David!
/Staffan
On 18 feb 2013, at 05:23, David Holmes wrote:
> Thanks for clarifying this Staffan, I see now how this was setup wrong in the
> first place. You are using the right mechanism now.
>
> I'm not sure UseTLAB warrants its own accessor in VM anymore as the two uses
> of it can simply ask for the flag value directly. Your call.
>
> Thanks,
> David
>
> On 12/02/2013 8:19 PM, Staffan Larsen wrote:
>> The values are initialized at VM compile time in the
>> VMStructs::localHotSpotVMIntConstants array. I don't think we want to modify
>> SA so that values of int constants is made into a dynamic lookup. Or at
>> least, that is a fairly significant change to SA. The problem here was to
>> think that UseTLAB is a constant when it isn't.
>>
>> Yes, I thought about a regression test for this, but the valuable test would
>> be to verify that no command line flag values are read as constants, and I
>> don't see how I could write that.
>>
>> Thanks,
>> /Staffan
>>
>> On 12 feb 2013, at 01:20, David Holmes wrote:
>>
>>> On 10/02/2013 5:06 AM, Staffan Larsen wrote:
Please review this small patch to avoid reading flag values in SA as
constants. Reading them as constants means SA will only see the default
value for these flags. Instead the infrastructure in SA to read command
line flags should be used. In addition the value if EnableInvokeDynamic is
never used, so I removed that from SA.
>>>
>>> Isn't the real problem in the way db is initialized with the values for
>>> these flags? ie shouldn't db.lookupIntConstant("UseTLAB").intValue() be
>>> returning the actual value of UseTLAB as it occurs in the VM ?
>>>
>>> Also we need a regression test for this as obviously it ain't being tested!
>>> :(
>>>
>>> Thanks,
>>> David
>>>
webrev: http://cr.openjdk.java.net/~sla/8007901/webrev.00/
bug: http://bugs.sun.com/view_bug.do?bug_id=8007901 (not yet visible)
Thanks,
/Staffan
>>
Re: RFR (XS): 8007901 SA: Don't read flag values as constants
Thanks for clarifying this Staffan, I see now how this was setup wrong
in the first place. You are using the right mechanism now.
I'm not sure UseTLAB warrants its own accessor in VM anymore as the two
uses of it can simply ask for the flag value directly. Your call.
Thanks,
David
On 12/02/2013 8:19 PM, Staffan Larsen wrote:
The values are initialized at VM compile time in the
VMStructs::localHotSpotVMIntConstants array. I don't think we want to modify SA
so that values of int constants is made into a dynamic lookup. Or at least,
that is a fairly significant change to SA. The problem here was to think that
UseTLAB is a constant when it isn't.
Yes, I thought about a regression test for this, but the valuable test would be
to verify that no command line flag values are read as constants, and I don't
see how I could write that.
Thanks,
/Staffan
On 12 feb 2013, at 01:20, David Holmes wrote:
On 10/02/2013 5:06 AM, Staffan Larsen wrote:
Please review this small patch to avoid reading flag values in SA as constants.
Reading them as constants means SA will only see the default value for these
flags. Instead the infrastructure in SA to read command line flags should be
used. In addition the value if EnableInvokeDynamic is never used, so I removed
that from SA.
Isn't the real problem in the way db is initialized with the values for these flags? ie
shouldn't db.lookupIntConstant("UseTLAB").intValue() be returning the actual
value of UseTLAB as it occurs in the VM ?
Also we need a regression test for this as obviously it ain't being tested! :(
Thanks,
David
webrev: http://cr.openjdk.java.net/~sla/8007901/webrev.00/
bug: http://bugs.sun.com/view_bug.do?bug_id=8007901 (not yet visible)
Thanks,
/Staffan
Re: RFR (XS): 8007901 SA: Don't read flag values as constants
The values are initialized at VM compile time in the
VMStructs::localHotSpotVMIntConstants array. I don't think we want to modify SA
so that values of int constants is made into a dynamic lookup. Or at least,
that is a fairly significant change to SA. The problem here was to think that
UseTLAB is a constant when it isn't.
Yes, I thought about a regression test for this, but the valuable test would be
to verify that no command line flag values are read as constants, and I don't
see how I could write that.
Thanks,
/Staffan
On 12 feb 2013, at 01:20, David Holmes wrote:
> On 10/02/2013 5:06 AM, Staffan Larsen wrote:
>> Please review this small patch to avoid reading flag values in SA as
>> constants. Reading them as constants means SA will only see the default
>> value for these flags. Instead the infrastructure in SA to read command line
>> flags should be used. In addition the value if EnableInvokeDynamic is never
>> used, so I removed that from SA.
>
> Isn't the real problem in the way db is initialized with the values for these
> flags? ie shouldn't db.lookupIntConstant("UseTLAB").intValue() be returning
> the actual value of UseTLAB as it occurs in the VM ?
>
> Also we need a regression test for this as obviously it ain't being tested! :(
>
> Thanks,
> David
>
>> webrev: http://cr.openjdk.java.net/~sla/8007901/webrev.00/
>> bug: http://bugs.sun.com/view_bug.do?bug_id=8007901 (not yet visible)
>>
>> Thanks,
>> /Staffan
>>
Re: RFR (XS): 8007901 SA: Don't read flag values as constants
On 10/02/2013 5:06 AM, Staffan Larsen wrote:
Please review this small patch to avoid reading flag values in SA as constants.
Reading them as constants means SA will only see the default value for these
flags. Instead the infrastructure in SA to read command line flags should be
used. In addition the value if EnableInvokeDynamic is never used, so I removed
that from SA.
Isn't the real problem in the way db is initialized with the values for
these flags? ie shouldn't db.lookupIntConstant("UseTLAB").intValue() be
returning the actual value of UseTLAB as it occurs in the VM ?
Also we need a regression test for this as obviously it ain't being
tested! :(
Thanks,
David
webrev: http://cr.openjdk.java.net/~sla/8007901/webrev.00/
bug: http://bugs.sun.com/view_bug.do?bug_id=8007901 (not yet visible)
Thanks,
/Staffan
Re: RFR (XS): 8007901 SA: Don't read flag values as constants
Yes, you got it right. Thanks, /Staffan On 11 feb 2013, at 19:45, Mikael Vidstedt wrote: > > Nasty, so normally the code picks up the value from the VMIntConstant > vmstructs array which is the hardcoded default value, but with this fix it's > picked up from the command line options database which reflects the actual > current value. That makes sense and looks good. > > Cheers, > Mikael > > On 2013-02-09 11:06, Staffan Larsen wrote: >> Please review this small patch to avoid reading flag values in SA as >> constants. Reading them as constants means SA will only see the default >> value for these flags. Instead the infrastructure in SA to read command line >> flags should be used. In addition the value if EnableInvokeDynamic is never >> used, so I removed that from SA. >> >> webrev: http://cr.openjdk.java.net/~sla/8007901/webrev.00/ >> bug: http://bugs.sun.com/view_bug.do?bug_id=8007901 (not yet visible) >> >> Thanks, >> /Staffan >
Re: RFR (XS): 8007901 SA: Don't read flag values as constants
Nasty, so normally the code picks up the value from the VMIntConstant vmstructs array which is the hardcoded default value, but with this fix it's picked up from the command line options database which reflects the actual current value. That makes sense and looks good. Cheers, Mikael On 2013-02-09 11:06, Staffan Larsen wrote: Please review this small patch to avoid reading flag values in SA as constants. Reading them as constants means SA will only see the default value for these flags. Instead the infrastructure in SA to read command line flags should be used. In addition the value if EnableInvokeDynamic is never used, so I removed that from SA. webrev: http://cr.openjdk.java.net/~sla/8007901/webrev.00/ bug: http://bugs.sun.com/view_bug.do?bug_id=8007901 (not yet visible) Thanks, /Staffan
RFR (XS): 8007901 SA: Don't read flag values as constants
Please review this small patch to avoid reading flag values in SA as constants. Reading them as constants means SA will only see the default value for these flags. Instead the infrastructure in SA to read command line flags should be used. In addition the value if EnableInvokeDynamic is never used, so I removed that from SA. webrev: http://cr.openjdk.java.net/~sla/8007901/webrev.00/ bug: http://bugs.sun.com/view_bug.do?bug_id=8007901 (not yet visible) Thanks, /Staffan
