I would like to suggest some cosmetic changes. The hex constants could have underscores, for example 0x8000_0000. The FLAG_MASK constant could be expressed as the OR of the three flags rather than as a literal, and the DOMAIN_LENGTH_MASK could be ~FLAG_MASK. In setDomainLength, the parameter name l isn't very good because it looks like 1. Éamonn
2015-08-04 7:20 GMT-07:00 Jaroslav Bachorik <jaroslav.bacho...@oracle.com>: > 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- >>>>> >>>>> >>>> >>> >> >