That makes me sad. The limit is clearly a detail of this particular implementation and should not be enshrined in the spec. Éamonn
2015-08-05 8:02 GMT-07:00 Jaroslav Bachorik <jaroslav.bacho...@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.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- >>> >>> >>> >
