RFR 8041565: JMX ObjectName could be refactored to save memory
Jaroslav Bachorik
jaroslav.bachorik at oracle.com
Thu Aug 13 16:13:11 UTC 2015
On 13.8.2015 02:53, Daniel Fuchs wrote:
> Hi Jaroslav,
>
> On 12/08/15 18:05, Jaroslav Bachorik wrote:
>> 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
>
> Why are index and in_index now declared as short? Is that a left over
> from the original webrev?
>
> 445 short index = 0;
> 501 short in_index;
Thanks for catching this!
Updated: http://cr.openjdk.java.net/~jbachorik/8041565/webrev.05
I'd better wait for Eamonn before proceeding with integration.
-JB-
>
> should these be reverted to 'int' ?
>
> Otherwise - this looks good...
>
> -- daniel
>
>
>>
>> -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