RFR 8041565: JMX ObjectName could be refactored to save memory

[Jaroslav Bachorik]

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