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 :-)
With that in mind I believe you should consider throwing
InternalError - or IllegalArgumentException - instead of
using 'assert' statements.
BTW have you run the JCK?
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-
