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
-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-
