RFR 8041565: JMX ObjectName could be refactored to save memory
Eamonn McManus
eamonn at mcmanus.net
Tue Aug 4 15:24:09 UTC 2015
I would like to suggest some cosmetic changes. The hex constants could
have underscores, for example 0x8000_0000. The FLAG_MASK constant
could be expressed as the OR of the three flags rather than as a
literal, and the DOMAIN_LENGTH_MASK could be ~FLAG_MASK. In
setDomainLength, the parameter name l isn't very good because it looks
like 1.
Éamonn
2015-08-04 7:20 GMT-07:00 Jaroslav Bachorik <jaroslav.bachorik at oracle.com>:
> 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