RFR 8041565: JMX ObjectName could be refactored to save memory
Eamonn McManus
eamonn at mcmanus.net
Wed Aug 5 15:04:06 UTC 2015
That makes me sad. The limit is clearly a detail of this particular
implementation and should not be enshrined in the spec.
Éamonn
2015-08-05 8:02 GMT-07:00 Jaroslav Bachorik <jaroslav.bachorik at oracle.com>:
> On 5.8.2015 16:53, Eamonn McManus wrote:
>>
>> I would remove the spec changes about the limit on the domain length,
>> which are a property of this particular implementation. It's perfectly
>> reasonable to blow up if the domain length is > 536,870,911, but
>> there's no reason for it to be in the spec.
>
>
> Well, CCC board had a different opinion. That's why this limit is documented
> in the spec.
>
> -JB-
>
>
>> Éamonn
>>
>>
>> 2015-08-05 4:48 GMT-07:00 Jaroslav Bachorik
>> <jaroslav.bachorik at oracle.com>:
>>>
>>> Eamonn, Daniel,
>>>
>>> thanks for the comments.
>>>
>>> I've updated the webrev to address them. Also, I've added a test to
>>> exercise
>>> the boolean flag en-/decoding in ObjectName.
>>>
>>> http://cr.openjdk.java.net/~jbachorik/8041565/webrev.03
>>>
>>>
>>> Cheers,
>>>
>>> -JB-
>>>
>>>
>>> On 4.8.2015 23:02, Daniel Fuchs wrote:
>>>>
>>>>
>>>> Hi Jaroslav,
>>>>
>>>> 379 * This field encodes _domain_pattern, _property_list_pattern
>>>> and
>>>> 380 * _property_value_pattern booleans.
>>>>
>>>> If I'm not mistaken it also encodes the domain length.
>>>>
>>>>
>>>> 1072 if ((l & FLAG_MASK) > 0 ) {
>>>>
>>>> Although it is not expected that 'l' could be negative, it might be
>>>> better to write this test as:
>>>>
>>>> if ((l & FLAG_MASK) != 0 ) {
>>>>
>>>> (+ I agree with Éamonn that l is not a great parameter name - nice to
>>>> see you back Éamonn :-)) best regards, -- daniel On 8/4/15 4:20 PM,
>>>> Jaroslav Bachorik wrote:
>>>>>
>>>>>
>>>>> Hi, reviving this review. On 14.4.2015 16:58, Jaroslav Bachorik wrote:
>>>>>>
>>>>>>
>>>>>> On 14.4.2015 14:56, Daniel Fuchs wrote:
>>>>>>>
>>>>>>>
>>>>>>> Hi Jaroslav, I like this change, but it does introduce an
>>>>>>> incompatibility, so it probably needs a CCC and some release notes.
>>>>>>> For instance, this test passes with the current version of
>>>>>>> ObjectName: public class StringLengthTest { final static int
>>>>>>> smax = Short.MAX_VALUE; final static int smore = smax + 126;
>>>>>>> public static void main(String[] args) throws
>>>>>>> MalformedObjectNameException { char[] chars = new
>>>>>>> char[smore]; Arrays.fill(chars, 0, smax, 'a');
>>>>>>> Arrays.fill(chars, smax, smore, 'b');
>>>>>>> System.out.println(new ObjectName(new String(chars), "type",
>>>>>>> "Test")); } } I'm not sure what it will do with your changes :-)
>>>>>>
>>>>>>
>>>>>> It will fail with assert (if enabled). Or truncate the domain name, I
>>>>>> suppose.
>>>>>>>
>>>>>>>
>>>>>>> With that in mind I believe you should consider throwing
>>>>>>> InternalError - or IllegalArgumentException - instead of using
>>>>>>> 'assert' statements.
>>>>>>
>>>>>>
>>>>>> This would probably be better.
>>>>>>>
>>>>>>>
>>>>>>> BTW have you run the JCK?
>>>>>>
>>>>>>
>>>>>> Yes. All passed. I don't think JCK is testing for unrealistic values
>>>>>> :) I don't see how one could come up with a domain name 32767
>>>>>> characters long.
>>>>>
>>>>>
>>>>> The proposed change has passed the CCC review. In case the domain name
>>>>> is longer than Integer.MAX_VALUE/4 a MalformedObjectNameException will
>>>>> be thrown. All the JMX tests and JCK are still passing.
>>>>> http://cr.openjdk.java.net/~jbachorik/8041565/webrev.02 -JB-
>>>>>>
>>>>>>
>>>>>> -JB-
>>>>>>>
>>>>>>>
>>>>>>> best regards, -- daniel On 13/04/15 17:07, Jaroslav Bachorik wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi Roger, On 13.4.2015 16:07, Roger Riggs wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hi Jaroslav, Minor comments: 1488+: In forms like:
>>>>>>>>> _pattern_flag &= (~PROPLIST_PATTERN & 0xff);" The &0xff seems
>>>>>>>>> unnecessary since the store is to a byte field.
>>>>>>>>
>>>>>>>>
>>>>>>>> Fixed: http://cr.openjdk.java.net/~jbachorik/8041565/webrev.01
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 1644: the ? and : operators should be surrounded by spaces. There
>>>>>>>>> are other style issues, such as then statements on the same line
>>>>>>>>> as the 'if' but that may be beyond the scope of this change.
>>>>>>>>
>>>>>>>>
>>>>>>>> I know but these style issue have been around since the file was
>>>>>>>> first committed. I didn't address them because it didn't feel right
>>>>>>>> to mix style changes with the actual functionality. Cheers, -JB-
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Otherwise looks fine (as a 9 reviewer, but not specifically a
>>>>>>>>> serviceability reviewer). Thanks, Roger On 4/13/2015 5:43 AM,
>>>>>>>>> Jaroslav Bachorik wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Please, review the following change Issue :
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8041565 Webrev:
>>>>>>>>>> http://cr.openjdk.java.net/~jbachorik/8041565/webrev.00 In
>>>>>>>>>> situations when there are 10s of thousands ObjectNname instances
>>>>>>>>>> around (enterprise setups etc.) the 3 separate internal boolean
>>>>>>>>>> fields can lead to a noticeable memory waste. Adding insult to
>>>>>>>>>> the injury, with the current field layout it is necessary to
>>>>>>>>>> align the instances by 4 bytes. When using JOL
>>>>>>>>>> (http://openjdk.java.net/projects/code-tools/jol/) to inspect the
>>>>>>>>>> object layout we can see this: Before optimization (JDK8u40): ---
>>>>>>>>>> javax.management.ObjectName object internals: OFFSET SIZE TYPE
>>>>>>>>>> DESCRIPTION VALUE 0 12 (object header)| N/A 12 4 int
>>>>>>>>>> ObjectName._domain_length N/A 16 1 boolean
>>>>>>>>>> ObjectName._domain_pattern N/A 17 1 boolean
>>>>>>>>>> ObjectName._property_list_pattern N/A 18 1 boolean
>>>>>>>>>> ObjectName._property_value_pattern N/A 19 1
>>>>>>>>>> (alignment/padding gap) N/A 20 4 String
>>>>>>>>>> ObjectName._canonicalName N/A 24 4 Property[]
>>>>>>>>>> ObjectName._kp_array N/A 28 4 Property[]
>>>>>>>>>> ObjectName._ca_array N/A 32 4 Map ObjectName._propertyList
>>>>>>>>>> N/A 36 4 (loss due to the next object alignment) Instance
>>>>>>>>>> size: 40 bytes (estimated, the sample instance is not available)
>>>>>>>>>> Space losses: 1 bytes internal + 4 bytes external = 5 bytes total
>>>>>>>>>> {noformat} After optimization (JDK9 internal build): ---
>>>>>>>>>> javax.management.ObjectName object internals: OFFSET SIZE TYPE
>>>>>>>>>> DESCRIPTION VALUE 0 12 (object header) N/A 12 2 short
>>>>>>>>>> ObjectName._domain_length N/A 14 1 byte
>>>>>>>>>> ObjectName._pattern_flag N/A 15 1 (alignment/padding gap)
>>>>>>>>>> N/A 16 4 String ObjectName._canonicalName N/A 20 4
>>>>>>>>>> Property[] ObjectName._kp_array N/A 24 4 Property[]
>>>>>>>>>> ObjectName._ca_array N/A 28 4 Map ObjectName._propertyList
>>>>>>>>>> N/A Instance size: 32 bytes (estimated, the sample instance is
>>>>>>>>>> not available) Space losses: 1 bytes internal + 0 bytes external
>>>>>>>>>> = 1 bytes total After optimization we can save 8 bytes per
>>>>>>>>>> instance which can translate to very interesting numbers on large
>>>>>>>>>> installations. To achieve this the domain name length is set to
>>>>>>>>>> be *short* instead of *int* and the three booleans kept for the
>>>>>>>>>> performance purposes are encoded into one byte value (as proposed
>>>>>>>>>> by the reporter, Jean-Francois Denise). All the regression and
>>>>>>>>>> JCK tests are passing after this change. Thanks, -JB-
>>>
>>>
>>>
>
More information about the serviceability-dev
mailing list