RFR 8041565: JMX ObjectName could be refactored to save memory
Jaroslav Bachorik
jaroslav.bachorik at oracle.com
Wed Aug 12 16:05:07 UTC 2015
On 5.8.2015 08:04, Eamonn McManus wrote:
> That makes me sad. The limit is clearly a detail of this particular
> implementation and should not be enshrined in the spec.
> Éamonn
I re-requested CCC for not mentioning the exact limit in the docs and it
has been approved (thanks to Joe for quick turnaround).
The update webrev: http://cr.openjdk.java.net/~jbachorik/8041565/webrev.04
-JB-
>
>
> 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