Looks fine to me (emcmanus). On Aug 13, 2015 11:13 AM, "Jaroslav Bachorik" <jaroslav.bacho...@oracle.com> wrote:
> 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.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- >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>> >>> >> >
