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. Éamonn
2015-08-05 4:48 GMT-07:00 Jaroslav Bachorik <jaroslav.bacho...@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- > >
