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-