RFR 8041565: JMX ObjectName could be refactored to save memory

Wed Aug 5 15:02:19 UTC 2015

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-
>>
>>