RFR 8041565: JMX ObjectName could be refactored to save memory

[Daniel Fuchs]

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- 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20150804/4c3ff4fd/attachment.html>