Re: RFR 8235359: Simplify method Class.getRecordComponents()

[Frederic Parain]

Looks good to me.

Fred


> On Dec 5, 2019, at 09:36, Harold Seigel  wrote:
> 
> Hi,
> 
> Please review this small change to simplify Class.getRecordComponents() by 
> changing the return type of getRecordComponents0() to RecordComponent[].
> 
> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8235359/webrev/index.html
> 
> JBS Bug: https://bugs.openjdk.java.net/browse/JDK-8235359
> 
> The fix was regression tested by running Mach5 tiers 1 and 2 tests and builds 
> on Linux-x64, Solaris, Windows, and Mac OS X, by running Mach5 tiers 3-5 
> tests on Linux-x64, and JCK lang and VM tests on Linux-x64.
> 
> Thanks, Harold
> 

Re: RFR JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

[Frederic Parain]

Much cleaner!

Thumbs up!

Fred

> On Aug 14, 2019, at 15:42, Mandy Chung  wrote:
> 
> I have further discussion with Coleen and walkthrough the vframe 
> implementation.  Here is what we confirm and agree.
> 
> vframeStream::bci might return an invalid bci whereas javaVFrame::bci returns 
> a valid bci (compiledVFrame::bci adjusts invalid bci to 0).
> 
> An invalid bci could happen when hitting a safepoint on bci #0 on a 
> synchronized method either before or after the monitor is locked (implicit 
> synchronization without explicit monitorenter).   That might occur when 
> StackOverflowError is thrown for example.  However, that should never happen 
> when StackWalker is called and the top frame would be in the stack walking 
> code.
> 
> +1/-1 adjustment intends to address invalid bci case but such case is not 
> applicable for StackWalker API.  With that, I agree with Coleen that VM 
> should set the bci value to StackFrameInfo::bci field and no adjustment 
> needed:
>http://cr.openjdk.java.net/~mchung/jdk14/8193325/webrev.05/
> 
> thanks
> Mandy
> 
> On 8/13/19 4:56 PM, coleen.phillim...@oracle.com wrote:
>> 
>> Hi, I saw my name in this thread and had a discussion with Mandy. I don't 
>> like that the VM and JDK having this special coordinated dance of +1/-1, and 
>> the reason for this is to differentiate the value of 0 in StackFrame meaning 
>> either uninitialized or invalid. If through some race, an unitialized value 
>> is observed, then the MemberName may not be filled in either.  In any case 
>> zero makes sense to return as the bci because it'll point to the line number 
>> beginning of the method, if used to get the line number.
>> 
>> The stackwalk API doesn't return invalid bci because it adjusts it:
>> 
>> int compiledVFrame::bci() const {
>>   int raw = raw_bci();
>>   return raw == SynchronizationEntryBCI ? 0 : raw;
>> }
>> 
>> At any rate, the 04 webrev looks good minus the +1/-1 dance and 
>> StackWalker.java comment.  Coupling the jdk and jvm like this feels like 
>> it's asking for bugs.  I like the simpler implementation that fixes the bug 
>> that we have.
>> 
>> Thanks,
>> Coleen
>> 
>> 
>> On 8/13/19 1:49 PM, Mandy Chung wrote:
>>> 
>>> 
>>> On 8/13/19 9:30 AM, Peter Levart wrote:
 Usually the StackFrameInfo(s) are consumed as soon as they are returned 
 from StackWalker API and never assigned to @Stable field. So there's no 
 purpose of @Stable for bci field use. Except documentation. But 
 documentation can be specified in the form of comments too.
 
>>> 
>>> There are several JDK classes whose fields are filled by VM and comment is 
>>> the form of documentation.  Until new use of StackFrame in the future shows 
>>> performance benefit, it's okay to stick with @Stable original intent and 
>>> keep the comment:
>>> 
>>> +private int bci;// zero and negative values 
>>> represent invalid BCI
>>> 
>>> Please also review CSR:
>>>https://bugs.openjdk.java.net/browse/JDK-8229487
>>> 
>>> Mandy
>> 
> 

Re: RFR JDK-8193325: StackFrameInfo::getByteCodeIndex returns wrong value if bci > 32767

[Frederic Parain]

This looks good to me, with two comments:

I don’t like the static final RETAIN_CLASS_REF for the same
reasons as Aleksey, but I can live with that.

The protocol between the JVM and the Java class is well explained
on the JVM side (javaClasses.cpp:4227). I think it would be valuable
to provide the same description on the Java side, the comment in
StackFrameInfo.java:42 describes only part of the protocol.

No need for another review from me.

Regards,

Fred
 

> On Aug 12, 2019, at 16:24, Mandy Chung  wrote:
> 
> Having a second thought, I'm keeping @Stable bci field while zero indicates 
> an invalid BCI that makes it obvious that this field will be updated.  VM 
> will set StackFrameInfo::bci to value+1.
> 
> http://cr.openjdk.java.net/~mchung/jdk14/8193325/webrev.03/
> 
> Mandy

Re: [10] RFR 8186046 Minimal ConstantDynamic support

[Frederic Parain]

I’m seeing no issue with rcx being aliased in this code.

Fred

> On Oct 30, 2017, at 15:44, Paul Sandoz  wrote:
> 
> Hi,
> 
> Thanks for reviewing.
> 
>> On 30 Oct 2017, at 11:05, Dmitry Samersoff  
>> wrote:
>> 
>> Paul,
>> 
>> templateTable_x86.cpp:
>> 
>> 564   const Register flags = rcx;
>> 565   const Register rarg = NOT_LP64(rcx) LP64_ONLY(c_rarg1);
>> 
>> Should we use another register for rarg under NOT_LP64 ?
>> 
> 
> I think it should be ok, it i ain’t an expert here on the interpreter and the 
> calling conventions, so please correct me.
> 
> Some more context:
> 
> +  const Register flags = rcx;
> +  const Register rarg = NOT_LP64(rcx) LP64_ONLY(c_rarg1);
> +  __ movl(rarg, (int)bytecode());
> 
> The current bytecode code is loaded into “rarg”
> 
> +  call_VM(obj, CAST_FROM_FN_PTR(address, InterpreterRuntime::resolve_ldc), 
> rarg);
> 
> Then “rarg" is the argument to the call to InterpreterRuntime::resolve_ldc, 
> after which it is no longer referred to.
> 
> +#ifndef _LP64
> +  // borrow rdi from locals
> +  __ get_thread(rdi);
> +  __ get_vm_result_2(flags, rdi);
> +  __ restore_locals();
> +#else
> +  __ get_vm_result_2(flags, r15_thread);
> +#endif
> 
> The result from the call is then loaded into flags.
> 
> So i don’t think it matters in this case if rcx is aliased.
> 
> Paul.
> 
>> -Dmitry
>> 
>> 
>> On 10/26/2017 08:03 PM, Paul Sandoz wrote:
>>> Hi,
>>> 
>>> Please review the following patch for minimal dynamic constant support:
>>> 
>>> http://cr.openjdk.java.net/~psandoz/jdk10/JDK-8186046-minimal-condy-support-hs/webrev/
>>>  
>>> 
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8186046 
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8186209 
>>> 
>>> 
>>> This patch is based on the JDK 10 unified HotSpot repository. Testing so 
>>> far looks good.
>>> 
>>> By minimal i mean just the support in the runtime for a dynamic constant 
>>> pool entry to be referenced by a LDC instruction or a bootstrap method 
>>> argument. Much of the work leverages the foundations built by invoke 
>>> dynamic but is arguably simpler since resolution is less complex.
>>> 
>>> A small set of bootstrap methods will be proposed as a follow on issue for 
>>> 10 (these are currently being refined in the amber repository).
>>> 
>>> Bootstrap method invocation has not changed (and the rules are the same for 
>>> dynamic constants and indy). It is planned to enhance this in a further 
>>> major release to support lazy resolution of bootstrap method arguments.
>>> 
>>> The CSR for the VM specification is here:
>>> 
>>> https://bugs.openjdk.java.net/browse/JDK-8189199 
>>> 
>>> 
>>> the j.l.invoke package documentation was also updated but please consider 
>>> the VM specification as the definitive "source of truth" (we may clean up 
>>> this area further later on so it becomes more informative, and that may 
>>> also apply to duplicative text on MethodHandles/VarHandles).
>>> 
>>> Any AoT-related work will be deferred to a future release.
>>> 
>>> —
>>> 
>>> This patch only supports x64 platforms. There is a small set of changes 
>>> specific to x64 (specifically to support null and primitives constants, as 
>>> prior to this patch null was used as a sentinel for resolution and certain 
>>> primitives types would never have been encountered, such as say byte).
>>> 
>>> We will need to follow up with the SPARC platform and it is 
>>> hoped/anticipated that OpenJDK members responsible for other platforms 
>>> (namely ARM and PPC) will separately provide patches.
>>> 
>>> —
>>> 
>>> Many of tests rely on an experimental byte code API that supports the 
>>> generation of byte code with dynamic constants.
>>> 
>>> One test uses class file bytes produced from a modified version of 
>>> asmtools.  The modifications have now been pushed but a new version of 
>>> asmtools need to be rolled into jtreg before the test can operate directly 
>>> on asmtools information rather than embedding class file bytes directly in 
>>> the test.
>>> 
>>> —
>>> 
>>> Paul.
>>> 
>> 
> 

Re: RFR 8156073 : 2-slot LiveStackFrame locals (long and double) are incorrect

[Frederic Parain]

Brent,

hotspot/src/share/vm/prims/stackwalk.cpp:
 230 if (!expressions &&
 231 values->at(i)->type() == T_INT &&
 232 values->int_at(i) == 0 && // empty first slot of long?
 233 i+1 < values->size() &&
 234 values->at(i+1)->type() == T_INT) {

How does this test make the difference between:
  1 - a local variable of type long
  2 - two consecutive local variables of type int with the value
  of the first one being zero

(1) requires the fix, but (2) does not.

Another question: is it guaranteed that an unused slot from
a pair-slot is always zero? This is not written in the JVM spec,
and I don't remember that the JVM zeroes unused slots.

A last question: the fix tries to provide a view of the local
variables that matches the JVM spec rather than the implementation,
so if half of the long or double value is put in the first slot,
why the second slot is not stripped to only store the other half?

Regards,

Fred

On 08/17/2016 04:05 PM, Brent Christian wrote:

Hi,

Please review this StackWalker fix in hotspot for 8156073, "2-slot
LiveStackFrame locals (long and double) are incorrect"

Bug: https://bugs.openjdk.java.net/browse/JDK-8156073
Webrev: http://cr.openjdk.java.net/~bchristi/8156073/webrev.01/

The experimental LiveStackFrame[1] interface for StackWalker provides
access to stack frames' local variables, returned in an Object[] from
LiveStackFrame.getLocals().

Long and double primitives are returned using two array slots (similar
to JVMS section 2.6.1 [2]).  This works as indicated on 32-bit JDKs, but
needs some fixing on 64-bit.

AFAICT under 64-bit, StackValueCollection stores the entire
long/double in the 2nd (64-bit) slot:

jlong StackValueCollection::long_at(int slot) const {
#ifdef _LP64
  return at(slot+1)->get_int();
#else
...

The first slot goes unused and contains 0, instead of holding half of
the long value as stackwalker expects.  So stackwalker needs to take
care splitting the value between the two slots.

The fix examines StackValueCollection::long_at(i), checks for an
unexpected 0 value from StackValueCollection::int_at(i), and copies
the needed 32-bits (depending on native byte ordering) into the first
slot as needed.  (The 2nd slot gets truncated to 32-bits in
create_primitive_value_instance()).

Here's the fix in action on linux_x64 with the updated
LocalsAndOperands.java test.  Slots 4+5 contain a long, 6+7 contain a
double.

Before the fix:
...
  local 3: himom type java.lang.String
  local 4: 0 type I  <--
  local 5: 65535 type I
  local 6: 0 type I  <--
  local 7: 1293080650 type I
  local 8: 0 type I

After the fix:
...
  local 3: himom type java.lang.String
  local 4: 1023 type I   <--
  local 5: 65535 type I
  local 6: 1074340347 type I <--
  local 7: 1293080650 type I
  local 8: 0 type I

This change also fixes 8158879, "Add new test for verifying 2-slot
locals in LiveStackFrame".  Thanks to Fabio Tudone
(fa...@paralleluniverse.co) for the test case.  I only test unused
("dead") local variables with -Xint, because they are removed in
compiled frames.

An automated build & test run on all platforms looks good.

It would be good to also do more testing of LiveStackFrame.getStack(),
but right now I want to focus on getting this getLocals() fix in.
Testing of LiveStackFrame.getStack() will happen in a follow-up issue
(8163825).

Thanks,
-Brent

1.
http://hg.openjdk.java.net/jdk9/hs/jdk/file/1efce54b06b7/src/java.base/share/classes/java/lang/LiveStackFrame.java

2.
https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-2.html#jvms-2.6.1

RFR(XXXXXXXS): 8161034 Incorrect GPL header in jdk/src/java.base/windows/native/libjava/jni_util_md.c

[frederic parain]

Hello,

src/java.base/windows/native/libjava/jni_util_md.c file contains 
incorrect GPL header which fails make/scripts/lic_check.sh script check.


Failure is caused by missing comma after modification years. Please, 
help to review its addition:

http://cr.openjdk.java.net/~fparain/8161034/webrev.00/

lic_check.sh passes after this modification:
### Checking copyright notice in the file: 
jdk/src/java.base/windows/native/libjava/jni_util_md.c

###
SUCCESS: The license header for 
/export/fparain/WORK/JDK9/8161034/hs/jdk/src/java.base/windows/native/libjava/jni_util_md.c 
has been verified.

###

Thank you,

Fred

Re: RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical Sections

[Frederic Parain]

Goetz,

On 14/12/2015 16:11, Lindenmaier, Goetz wrote:

Hi Frederic,

thanks for your fast reply.
 From reading the code I guessed about what you explained, now
I understood more details, thanks!

I will not fiddle with handling the zones. My change is only about
handling larger pages, i.e. the sizes of the zones, so there is no
danger I will break the mechanism you implemented.


The take over is that the VM code should be able to manage
the reserved zone alone or the reserved zone and the yellow
zone together.
If you want to change it for a better name, more explicit,
I'm fine with that.

Yes, I would like to change it to 'yellow_reserved' wherever
both are handled at the same time.
There's places where red+yellow+reserved are handled as
one,  here I will use 'guard_zone'.  That's aliged with
create_stack_guard_pages() which guards all (red, yellow,
reserved) of them.


I believe that red+yellow+reserved are handled as one only
during stack initialization. Once configured, the only
moment when the red zone protection is changed is when the
VM is about to generate a crash dump.

Anyway, 'yellow_reserved' sounds good to me.


stack_yellow_zone_enabled() -> stack_guards_enabled()

Here, "guards" refers to the two guard zones: reserved
+ yellow.

Actually, it also includes that the red page is enabled, right?

One question, in os_linux.cpp, you meant AMD64, not Itanium, right?
27.18+#if defined(IA32) || defined(IA64)
I'll move that functionality to another place, so no need to fix it.


You're right, I meant AMD64.

Thanks,

Fred


-Original Message-
From: Frederic Parain [mailto:frederic.par...@oracle.com]
Sent: Montag, 14. Dezember 2015 15:44
To: Lindenmaier, Goetz <goetz.lindenma...@sap.com>; Karen Kinnear
<karen.kinn...@oracle.com>
Cc: hotspot-...@openjdk.java.net; core-libs-dev 
Subject: Re: RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical
Sections

Goetz,

The reserved zone is sometime considered as a subpart of
the yellow zone, and sometime as an independent entity.
Technically the reserved zone is placed just before the
yellow zone, but the way it is managed depends on the
context.

In most cases, there's no specially annotated methods on
the thread's call stack. In this configuration the
reserved zone is considered as being part of the yellow
zone. This means that the protection of the reserved
zone is always the same as the protection of the yellow
zone.

In some cases, a thread could have one or more methods
on its call stack with the ReservedStackAccess annotation.
In this configuration, the reserved zone is managed as
a separate entity. Initially protected like the yellow
zone, access to the reserved zone can be temporary granted
for the execution of some critical sections. This means
that the protection of the reserved zone can be removed
while the yellow zone is still protected.

The take over is that the VM code should be able to manage
the reserved zone alone or the reserved zone and the yellow
zone together. I've already renamed a method because of
this change:
stack_yellow_zone_enabled() -> stack_guards_enabled()

Here, "guards" refers to the two guard zones: reserved
+ yellow.

If you want to change it for a better name, more explicit,
I'm fine with that. We just have to preserve the different
operations required for stack overflow management.

Let's say R(x) and Y(x) represent the protection of
the reserved zone and the yellow zone respectively.
And let's say that x = P means "zone protected"
and x = G means "access granted"

The different configurations are:

(1) R(P) Y(P)  => initial configuration
(2) R(G) Y(P)  => first stack overflow with annotated
method on stack
(3) R(G) Y(G)  => stack overflow without annotated
method on stack, or second stack
overflow with annotated method
on stack

And the transitions are:

(1) -> (3) -> (1)
or
(1) -> (2) -> (1)
or
(1) -> (2) -> (3) -> (1)

I hope this would clarify the semantic of the reserved zone.
If it doesn't, let me know, I'll try to explain it differently.

Thanks,

Fred

On 14/12/2015 14:59, Lindenmaier, Goetz wrote:

Hi Frederic,

I'm now again working on my change "8139864: Improve handling of stack

protection zones."

Coleen had asked me to wait until this change of yours is submitted.

You changed the stack_yellow_zone accessor functions in thread.hpp to
handle both zones, yellow and reserved.
Therefore, reading the code, the reserved zone seems to be part of the

yellow zone.


In the description of the bug, it says "The new zone defined by the

proposed

solution is placed just before the yellow zone." This reads as if the zones

are

separate.

Would you mind if I rename the stack_yellow_zone accessor functions to
stack_yellow_reserved_zone?  This would make clear in the code that this
are two separate zones.

Re: RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical Sections

[Frederic Parain]

Karen,

Thank you for your review.

Fred

On 23/11/2015 20:10, Karen Kinnear wrote:

Frederic,

Looks good.

Many thanks.
Karen


On Nov 23, 2015, at 12:44 PM, Frederic Parain
<frederic.par...@oracle.com <mailto:frederic.par...@oracle.com>> wrote:

Karen,

Thank you for your review, my answers are in-lined below.

New Webrevs (including some fixes suggested by Paul Sandoz):

http://cr.openjdk.java.net/~fparain/8046936/webrev.01/hotspot/
http://cr.openjdk.java.net/~fparain/8046936/webrev.01/jdk/

On 20/11/2015 19:44, Karen Kinnear wrote:

Frederic,

Code review for web revs you sent out.
Code looks good. I am not as familiar with the compiler code.

I realize you need to check in at least a subset of the
java.util.concurrent changes for
the test to work, so maybe I should not have asked Doug about his
preference there.
Sorry.

I am impressed you found a way to make a reproducible test!

Comments/questions:
1. src/cpu/sparc/vm/interp_masm_sparc.cpp line 1144 “is” -> “if”


Fixed


2. IIRC, due to another bug with windows handling of our guard pages,
this
is disabled for Windows. Would it be worth putting a comment in the
bug , 8067946, that once this is fixed, the reserved stack logic on
windows
will need testing before enabling?


More than testing, the implementation would have to be completed on
Windows. I've added a comment to JDK-8067946.


3. In get_frame_at_stack_banging_point on Linux, BSD and solaris_x86 if
this is in interpreter code, you explicitly return the Java sender
of the current frame. I was expecting to see that on Solaris_sparc
and Windows
as well? I do see the assertion in caller that you do have a java frame.


It doesn't make sense to check the current frame (no bytecode has been
executed yet, so risk of partially executed critical section). I did the
change but not for all platforms. This is now fixed for Solaris_SPARC
and Windows too.


4. test line 83 “writtent” -> “written”


Fixed


5. I like the way you set up the preallocated exception and then set
the message - we may
try that for default methods in future.

6. I had a memory that you had found a bug in safe_for_sender - did
you already check that in?


I've fixed x86 platforms in JDK-8068655.
I've piggybacked the SPARC fix to this webrev (frame_sparc.cpp:635),
I had hoped it would have been silently accepted :-)



7. I see the change in trace.xml, and I see an include added to
SharedRuntime.cpp,
but I didn’t see where it was used - did I just miss it?


trace.xml changes define a new event.
This event is created at sharedRuntime.cpp::3006 and it is used
in the next 3 lines.

Thanks. I must have mistyped when I searched for it.


Thanks,

Fred

--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email:frederic.par...@oracle.com <mailto:frederic.par...@oracle.com>




--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Re: RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical Sections

[Frederic Parain]

Goetz,

The reserved zone is sometime considered as a subpart of
the yellow zone, and sometime as an independent entity.
Technically the reserved zone is placed just before the
yellow zone, but the way it is managed depends on the
context.

In most cases, there's no specially annotated methods on
the thread's call stack. In this configuration the
reserved zone is considered as being part of the yellow
zone. This means that the protection of the reserved
zone is always the same as the protection of the yellow
zone.

In some cases, a thread could have one or more methods
on its call stack with the ReservedStackAccess annotation.
In this configuration, the reserved zone is managed as
a separate entity. Initially protected like the yellow
zone, access to the reserved zone can be temporary granted
for the execution of some critical sections. This means
that the protection of the reserved zone can be removed
while the yellow zone is still protected.

The take over is that the VM code should be able to manage
the reserved zone alone or the reserved zone and the yellow
zone together. I've already renamed a method because of
this change:
  stack_yellow_zone_enabled() -> stack_guards_enabled()

Here, "guards" refers to the two guard zones: reserved
+ yellow.

If you want to change it for a better name, more explicit,
I'm fine with that. We just have to preserve the different
operations required for stack overflow management.

Let's say R(x) and Y(x) represent the protection of
the reserved zone and the yellow zone respectively.
And let's say that x = P means "zone protected"
and x = G means "access granted"

The different configurations are:

(1) R(P) Y(P)  => initial configuration
(2) R(G) Y(P)  => first stack overflow with annotated
  method on stack
(3) R(G) Y(G)  => stack overflow without annotated
  method on stack, or second stack
  overflow with annotated method
  on stack

And the transitions are:

(1) -> (3) -> (1)
or
(1) -> (2) -> (1)
or
(1) -> (2) -> (3) -> (1)

I hope this would clarify the semantic of the reserved zone.
If it doesn't, let me know, I'll try to explain it differently.

Thanks,

Fred

On 14/12/2015 14:59, Lindenmaier, Goetz wrote:

Hi Frederic,

I'm now again working on my change "8139864: Improve handling of stack protection 
zones."
Coleen had asked me to wait until this change of yours is submitted.

You changed the stack_yellow_zone accessor functions in thread.hpp to
handle both zones, yellow and reserved.
Therefore, reading the code, the reserved zone seems to be part of the yellow 
zone.

In the description of the bug, it says "The new zone defined by the proposed
solution is placed just before the yellow zone." This reads as if the zones are
separate.

Would you mind if I rename the stack_yellow_zone accessor functions to
stack_yellow_reserved_zone?  This would make clear in the code that this
are two separate zones.

I anyways have to adapt most of the calls to these accessors.

Best regards,
   Goetz.





-Original Message-
From: hotspot-dev [mailto:hotspot-dev-boun...@openjdk.java.net] On
Behalf Of Frederic Parain
Sent: Montag, 14. Dezember 2015 14:24
To: Karen Kinnear <karen.kinn...@oracle.com>
Cc: hotspot-...@openjdk.java.net; core-libs-dev 
Subject: Re: RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical
Sections

Karen,

Thank you for your review.

Fred

On 23/11/2015 20:10, Karen Kinnear wrote:

Frederic,

Looks good.

Many thanks.
Karen


On Nov 23, 2015, at 12:44 PM, Frederic Parain
<frederic.par...@oracle.com <mailto:frederic.par...@oracle.com>>

wrote:


Karen,

Thank you for your review, my answers are in-lined below.

New Webrevs (including some fixes suggested by Paul Sandoz):

http://cr.openjdk.java.net/~fparain/8046936/webrev.01/hotspot/
http://cr.openjdk.java.net/~fparain/8046936/webrev.01/jdk/

On 20/11/2015 19:44, Karen Kinnear wrote:

Frederic,

Code review for web revs you sent out.
Code looks good. I am not as familiar with the compiler code.

I realize you need to check in at least a subset of the
java.util.concurrent changes for
the test to work, so maybe I should not have asked Doug about his
preference there.
Sorry.

I am impressed you found a way to make a reproducible test!

Comments/questions:
1. src/cpu/sparc/vm/interp_masm_sparc.cpp line 1144 “is” -> “if”


Fixed


2. IIRC, due to another bug with windows handling of our guard pages,
this
is disabled for Windows. Would it be worth putting a comment in the
bug , 8067946, that once this is fixed, the reserved stack logic on
windows
will need testing before enabling?


More than testing, the implementation would have to be completed on
Windows. I've added a comment to JDK-8067946.


3. In get_frame_at_stack_banging_point on Linux, BSD and solaris_x86 if
this is in interpreter code, you explicitly return the Java

Re: RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical Sections

[Frederic Parain]

All fixed.

Thank you Dan.

Fred

On 02/12/2015 19:22, Daniel D. Daugherty wrote:


On 12/1/15 9:21 AM, Frederic Parain wrote:

Hi Dan,

Thank you for your detailed review.
My answers are in-lined below.

New webrev:

http://cr.openjdk.java.net/~fparain/8046936/webrev.02/hotspot/


Re-reviewed by comparing 8046936.0[12].hotspot.patch in jfilemerge...

Just a couple of nits:

src/os/windows/vm/os_windows.cpp
 L2365: assert(fr->safe_for_sender(thread), "Safety check");
 Wrong indent; should be 6 spaces instead of 8; actually I think
 this one is a tab.

src/os_cpu/bsd_x86/vm/os_bsd_x86.cpp
 L381: assert(fr->safe_for_sender(thread), "Safety check");
 Wrong indent; this one also might be a tab

src/os_cpu/linux_x86/vm/os_linux_x86.cpp
 L194: assert(fr->safe_for_sender(thread), "Safety check");
 Wrong indent; this one also might be a tab

src/os_cpu/solaris_sparc/vm/os_solaris_sparc.cpp
 L267: assert(fr->safe_for_sender(thread), "Safety check");
 Wrong indent; this one also might be a tab

src/os_cpu/solaris_x86/vm/os_solaris_x86.cpp
 L255: assert(fr->safe_for_sender(thread), "Safety check");
 Wrong indent; this one also might be a tab


Thumbs up! I do not need to see a webrev for the above nits.

Dan





On 24/11/2015 17:26, Daniel D. Daugherty wrote:


src/cpu/sparc/vm/frame_sparc.cpp
 (old) L635:   if (fp() - sp() > 1024 +
m->max_stack()*Interpreter::stackElementSize) {
 (new) L635:   if (fp() - unextended_sp() > 1024 +
m->max_stack()*Interpreter::stackElementSize) {
 This looks like a bug fix independent of this fix.


Correct, this is the SPARC version of JDK-8068655.


src/share/vm/runtime/thread.hpp
 L953:   intptr_t*_reserved_stack_activation;
 L1382:   intptr_t* reserved_stack_activation() const { return
_reserved_stack_activation; }
 L1383:   void  set_reserved_stack_activation(intptr_t* addr) {

 I was expecting this type to be 'address' instead of
'intptr_t*'.

 Update: I've gone back through the changes and I still don't
 see a reason that this is 'intptr_t*'.


The _reserved_stack_activation has been declared as an 'intptr_t*'
just to be consistent with the _sp and _fp fields of the frame class.
However, this is not really a requirement, the content stored at the
_reserved_stack_activation address is never read. This address is just
a "marker" on the stack to quickly check if the thread has exited the
annotated code section or not. I've change the type to address, there's
slightly less casts, and it doesn't impact the ReservedStackArea logic.

Note: I've removed all further comments about _reserved_stack_activation
type in order to improve the e-mail readability.


 L1341: { return stack_yellow_zone_base();}
 '{' should be at the end of the previous line.
 Missing space after ';'.


Fixed


 L1343: { return StackReservedPages * os::vm_page_size(); }
 '{' should be at the end of the previous line.


Fixed


src/share/vm/runtime/thread.cpp
 L2543:   // The base notation is from the stacks point of view,
growing downward.
 L2565:   // The base notation is from the stacks point of view,
growing downward.
 Typo: "stacks point of view" -> "stack's point of view"


Fixed


 L2552:   } else {
 L2553: warning("Attempt to guard stack reserved zone failed.");
 L2554:   }
 L2555:   enable_register_stack_guard();

 Should enable_register_stack_guard() be called when we issued
 the warning on L2553?

 L2571:   } else {
 L2572: warning("Attempt to unguard stack reserved zone
failed.");
 L2573:   }
 L2574:   disable_register_stack_guard();

 Should disable_register_stack_guard() be called when we issued
 the warning on L2572?


enable_register_stack_guard() and disable_register_stack_guard() are
relics of the Itanium code (Itanium had a very different stack
management). These methods are currently empty on all OpenJDK and
Oracle platforms. May be another clean up opportunity here.
Regarding the placement of the calls, I followed the same pattern
as the other red/yellow pages management functions.


src/share/vm/runtime/sharedRuntime.cpp

 L784: java_lang_Throwable::set_message(exception_oop,
 L785: Universe::delayed_stack_overflow_error_message());
 Wrong indent; this should line up under the 'e' after the '('.


Fixed


 L2976:   if (fr.is_interpreted_frame()) {
 L2978: prv_fr = fr;
 L2982: prv_fr = fr;
 This line is in both branches of the if-statement on L2976.
 Is there a reason not to save prv_fr before L2976?


No particular reason, fixed.


 L2996  continue;
 Wrong indent; needs one more s

Re: RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical Sections

[Frederic Parain]

ackAccess
 Usually I hate literals like this, but this function has
 them in spades. :-(


I agree but I didn't find a better solution.


src/cpu/x86/vm/x86_64.ad
 L960:   MacroAssembler _masm();
 L965: MacroAssembler _masm();

 I think you can delete the _masm on L965.


Right, removed.


src/share/vm/opto/compile.cpp
 L675: _has_reserved_stack_access(target->has_reserved_stack_access()) {
 Wrong indent; should be a single space between ')' and '{'.


Fixed


test/runtime/ReservedStack/ReservedStackTest.java
 L26:  * @run main/othervm -XX:-Inline
-XX:CompileCommand=exclude,java/util/concurrent/locks/AbstractOwnableSynchronizer,setExclusiveOwnerThread
ReservedStackTest

 Should the comma before 'setExclusiveOwnerThread' be a period?
 Perhaps both formats work...


Both formats work, but I changed it to be a period, it's cleaner.


 L47:  *else
 Wrong indent; needs one more space before 'else'.

 L52:  * successfully update the status of the lock but he method
 Typo: 'update the' -> 'updates the'
 Typo: 'he method' -> 'the method'

 L60:  * first StackOverflowError is thrown, the Error is catched
 Typo: 'is catched' -> 'is caught'

 L61:  * and a few dozens frames are exited. Now the thread has
 Typo: 'a few dozens frames' -> 'a few dozen frames'

 L66:  * of its invocation, tries to acquire the next lock
 Typo: 'its invocation' -> 'its invocations'

 L81:  * stack to prevent false sharing. The test is using this
 Perhaps 'The test is using this'
  -> 'The test relies on this'

 to better match wording on L225-6.

 L82:  * to have different stack alignments and hit the targeted
 Grammar: 'and hit' -> 'when it hits'

 L102:  * exploit the  property that interpreter frames are (much)
 Typo: 'exploit' -> 'exploits'
 Delete extra space after 'the'.

 L123: //LOCK_ARRAY_SIZE value
 Add a space after '//'.

 L188: @jdk.internal.vm.annotation.ReservedStackAccess
 This isn't privileged code and -XX:-RestrictReservedStack
 isn't specified so what does this do?


It checks that by default the annotation is ignored for non-privileged
code, in case it is not ignored, the test would fail.



 L201:   System.exit(-1);
 Wrong indent; needs two more spaces.


All fixed.

Thank you very much!

Fred



On 20/11/2015 19:44, Karen Kinnear wrote:

Frederic,

Code review for web revs you sent out.
Code looks good. I am not as familiar with the compiler code.

I realize you need to check in at least a subset of the
java.util.concurrent changes for
the test to work, so maybe I should not have asked Doug about his
preference there.
Sorry.

I am impressed you found a way to make a reproducible test!

Comments/questions:
1. src/cpu/sparc/vm/interp_masm_sparc.cpp line 1144 “is” -> “if”


Fixed


2. IIRC, due to another bug with windows handling of our guard pages,
this
is disabled for Windows. Would it be worth putting a comment in the
bug , 8067946, that once this is fixed, the reserved stack logic on
windows
will need testing before enabling?


More than testing, the implementation would have to be completed on
Windows. I've added a comment to JDK-8067946.


3. In get_frame_at_stack_banging_point on Linux, BSD and solaris_x86 if
this is in interpreter code, you explicitly return the Java sender
of the current frame. I was expecting to see that on Solaris_sparc
and Windows
as well? I do see the assertion in caller that you do have a java frame.


It doesn't make sense to check the current frame (no bytecode has been
executed yet, so risk of partially executed critical section). I did the
change but not for all platforms. This is now fixed for Solaris_SPARC
and Windows too.


4. test line 83 “writtent” -> “written”


Fixed


5. I like the way you set up the preallocated exception and then set
the message - we may
try that for default methods in future.

6. I had a memory that you had found a bug in safe_for_sender - did
you already check that in?


I've fixed x86 platforms in JDK-8068655.
I've piggybacked the SPARC fix to this webrev (frame_sparc.cpp:635),
I had hoped it would have been silently accepted :-)



7. I see the change in trace.xml, and I see an include added to
SharedRuntime.cpp,
but I didn’t see where it was used - did I just miss it?


trace.xml changes define a new event.
This event is created at sharedRuntime.cpp::3006 and it is used
in the next 3 lines.

Thanks,

Fred





--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Re: RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical Sections

[Frederic Parain]

Karen,

Thank you for your review, my answers are in-lined below.

New Webrevs (including some fixes suggested by Paul Sandoz):

http://cr.openjdk.java.net/~fparain/8046936/webrev.01/hotspot/
http://cr.openjdk.java.net/~fparain/8046936/webrev.01/jdk/

On 20/11/2015 19:44, Karen Kinnear wrote:

Frederic,

Code review for web revs you sent out.
Code looks good. I am not as familiar with the compiler code.

I realize you need to check in at least a subset of the java.util.concurrent 
changes for
the test to work, so maybe I should not have asked Doug about his preference 
there.
Sorry.

I am impressed you found a way to make a reproducible test!

Comments/questions:
1. src/cpu/sparc/vm/interp_masm_sparc.cpp line 1144 “is” -> “if”


Fixed


2. IIRC, due to another bug with windows handling of our guard pages, this
is disabled for Windows. Would it be worth putting a comment in the
bug , 8067946, that once this is fixed, the reserved stack logic on windows
will need testing before enabling?


More than testing, the implementation would have to be completed on
Windows. I've added a comment to JDK-8067946.


3. In get_frame_at_stack_banging_point on Linux, BSD and solaris_x86 if
this is in interpreter code, you explicitly return the Java sender
of the current frame. I was expecting to see that on Solaris_sparc and Windows
as well? I do see the assertion in caller that you do have a java frame.


It doesn't make sense to check the current frame (no bytecode has been
executed yet, so risk of partially executed critical section). I did the
change but not for all platforms. This is now fixed for Solaris_SPARC
and Windows too.


4. test line 83 “writtent” -> “written”


Fixed


5. I like the way you set up the preallocated exception and then set the 
message - we may
try that for default methods in future.

6. I had a memory that you had found a bug in safe_for_sender - did you already 
check that in?


I've fixed x86 platforms in JDK-8068655.
I've piggybacked the SPARC fix to this webrev (frame_sparc.cpp:635),
I had hoped it would have been silently accepted :-)



7. I see the change in trace.xml, and I see an include added to 
SharedRuntime.cpp,
but I didn’t see where it was used - did I just miss it?


trace.xml changes define a new event.
This event is created at sharedRuntime.cpp::3006 and it is used
in the next 3 lines.

Thanks,

Fred

--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Re: RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical Sections

[frederic parain]

rom obvious why the annotation is placed on
NonfairSync.lock but FairSync.tryAcquire(), for example.


The annotation is used on methods with the problematic pattern
"atomic operation followed by method invocation".
Their execution could lead to data corruption if atomic operation
is executed successfully but the following method invocation
fails because of SOE.

In the NonFairSync class, this pattern is in the lock() method,
while the tryAcquire() is only a method invocation. So lock()
is annotated and tryAcquire() is not. Note that the method
invoked by try acquire is nonfairTryAcquire() which has the
problematic pattern and is annotated.

In the FairSync class, the situation is reversed: lock() is
just an method invocation and it is not annotated, and
tryAcquire() has the problematic pattern and is annotated.

I tried to put the annotation as close as possible to the
critical sections in order to minimize the size requirement
for the reserved area.


I would be tempted to place them on all the public lock/unlock methods
of the Lock implementations, rather than on the internal AQS-derived
functions, and AQS itself.


I've tried that :-) The amount of code being executed with
the ReservedStackAccess privilege tends to increase very
quickly and I was concerned about keeping the size of the
reserved area small.

Fred


On 6/11/2015 12:17 AM, Frederic Parain wrote:

Please review the changesets for JEP 270: Reserved Stack Areas for
Critical Sections

CR: https://bugs.openjdk.java.net/browse/JDK-8046936

Webrevs:
   hotspot:
http://cr.openjdk.java.net/~fparain/8046936/webrev.00/hotspot/
   JDK: http://cr.openjdk.java.net/~fparain/8046936/webrev.00/jdk/

The CR contains a detailed description of the issue and the design
of the solution as well as implementation details.

Tests:

 JPRT - hotspot & core
 RBT - vm.quick

Thanks

Fred







--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Re: Losing features of JVM_Open, e.g. CLOEXEC

[frederic parain]

On 30/10/2014 19:42, Martin Buchholz wrote:

Either the file descriptor leak issue is specific to
the zip package and can be fixed locally. Or the issue
impacts more native code and adding an infrastructure
to libjava should be discussed. Quickly grepping the
code, I found many naked calls to open, but only one
use of FD_CLOEXEC. Further investigations would be
required to track potential file descriptor leaks in
all libraries.

Did you create a bug to track this issue yet?


Nope.  But if I did, what component/sub-component would it go into.?
We don't even seem to have a place to report cross-library bugs!  We
have no infrastructure to work on the no infrastructure problem!


I'd suggest to create a first bug in core-libs to create
the infrastructure, and then other libs can refer to this
bug to fix their own code.
But it would be better to have core-libs team opinion before
doing this.

Regards,

Fred

--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Re: Losing features of JVM_Open, e.g. CLOEXEC

[frederic parain]

My bad. I missed the FD_CLOEXEC flag added by os::open()
when I replaced JVM_Open with open in zip native code.

However, I still think that removing those interfaces
was a good move. But I understand that the change it
introduces with zip file descriptors is an issue.

Reverting the changeset to re-introduce JVM_Open is
not a solution. Maintaining a VM interface only adding
a flag for only 2 call sites doesn't make sense to me.
Either the file descriptor leak issue is specific to
the zip package and can be fixed locally. Or the issue
impacts more native code and adding an infrastructure
to libjava should be discussed. Quickly grepping the
code, I found many naked calls to open, but only one
use of FD_CLOEXEC. Further investigations would be
required to track potential file descriptor leaks in
all libraries.

Did you create a bug to track this issue yet?

Regards,

Fred

On 30/10/2014 18:15, Martin Buchholz wrote:

Here's the state of the world:
- the Unix designers made a design mistake that file descriptors are
inherited by subprocesses by default.
- all library code (almost all the code in the universe, including the
JDK) needs to coexist with foreign code that might fork+exec at any
time
- to ensure that file descriptors don't leak into subprocesses,
(almost) all library calls that create file descriptors must ensure
that they have the close-on-exec bit set.  (yes, this has been broken
for a long time)
- this is difficult enough that all creations of file descriptors must
be wrapped using some kind of common infrastructure
- JVM_Open (aka os::open) was terrible infrastructure (essentially
undocumented) but at least it *was* infrastructure
- we need openjdk-level or at least core-libraries-level native
infrastructure, a place to put things like os::open and readFully.  A
project-wide commitment



--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Re: RFR(L): JDK-8057777 Cleanup of old and unused VM interfaces

[Frederic Parain]

Thank you Coleen, Harold, Alan and Sherman
for your reviews.

Fred

On 07/10/2014 15:06, Alan Bateman wrote:

On 06/10/2014 07:32, Frederic Parain wrote:

Thank you very much for catching the JVM_O_DELETE issue.
I moved the O_DELETE support into the ZipFile.c file
(code was the same for all non-Windows platforms) and
cleaned up the original code in HotSpot.

The new webrevs:

http://cr.openjdk.java.net/~fparain/805/jdk_v03/
http://cr.openjdk.java.net/~fparain/805/hotspot_v03/


Good spot by Sherman on the OPEN_DELETE issue, it makes me wonder what
the test coverage is on this.

The updated webrev addresses the points that I brought up and looks good
(nice to see many of these functions going away).

-Alan


--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Re: RFR(L): JDK-8057777 Cleanup of old and unused VM interfaces

[Frederic Parain]

Thank you very much for catching the JVM_O_DELETE issue.
I moved the O_DELETE support into the ZipFile.c file
(code was the same for all non-Windows platforms) and
cleaned up the original code in HotSpot.

The new webrevs:

http://cr.openjdk.java.net/~fparain/805/jdk_v03/
http://cr.openjdk.java.net/~fparain/805/hotspot_v03/

Regards,

Fred

On 10/03/2014 06:07 PM, Xueming Shen wrote:

On 10/3/14 8:19 AM, Alan Bateman wrote:

On 30/09/2014 07:40, Hideric Parain wrote:

Hi all,

Please review changes for bug JDK-805 Cleanup of old
and unused VM interfaces

CR:
https://bugs.openjdk.java.net/browse/JDK-805

This is basically a big cleanup of VM interfaces that are
not used anymore by the JDK but have been kept in our code
base for historical reasons (HotSpot Express for instance).
These changesets remove these interfaces from both the
JDK and the HotSpot side, and also perform some cleanup
on code that directly referenced the removed interfaces.

These changes do not modify the behavior of the Java
classes impacted by the cleanup.

VM interfaces removal has been approved by CCC and
a Release Note has been prepared that explicitly list
all the removed interfaces.

Testing: JPRT hotspot + core, vm.quick.testlist, jdk_core

Webrevs:
http://cr.openjdk.java.net/~fparain/805/

cc'ing core-libs-dev as part of this is clean-up in the library code too.

I think we should deprecate java.lang.Compiler and the
Runtime.traceXXX methods. They've been non-functional for a long time
and having them in the API is a bit mis-leading to anyone reading the
javadoc. I realize you are focused on the removing the old JVM_*
functions so we can follow-up on that via other issues of course.

Can ClassLoader#resolveClass0 can be removed completely? The null
check can be done in ClassLoader#resolveClass.

In the mapfile for libjava then the comment at line 281 says
ZipFile.c needs this one. As getLastErrorString is now exported for
use by libzip then the comment should probably be updated.

Otherwise this clean-up looks good to me and the jdk_core group of
tests is the right group to exercise this area.

-Alan



Hi,

ZipFile.c expects the JVM_Open() to handle the JVM_O_DELETE, with
explicit unlink on linux/solaris for example.
I would assume the open from the c library does not handle it and we
need to do it explicitly by ZipFile.c now?

-Sherman




--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Re: RFR(L): JDK-8057777 Cleanup of old and unused VM interfaces

[Frederic Parain]

Alan,

Thank you for your feedback, see my comments below.

On 03/10/2014 17:19, Alan Bateman wrote:

On 30/09/2014 07:40, Frederic Parain wrote:

Hi all,

Please review changes for bug JDK-805 Cleanup of old
and unused VM interfaces

CR:
https://bugs.openjdk.java.net/browse/JDK-805

This is basically a big cleanup of VM interfaces that are
not used anymore by the JDK but have been kept in our code
base for historical reasons (HotSpot Express for instance).
These changesets remove these interfaces from both the
JDK and the HotSpot side, and also perform some cleanup
on code that directly referenced the removed interfaces.

These changes do not modify the behavior of the Java
classes impacted by the cleanup.

VM interfaces removal has been approved by CCC and
a Release Note has been prepared that explicitly list
all the removed interfaces.

Testing: JPRT hotspot + core, vm.quick.testlist, jdk_core

Webrevs:
http://cr.openjdk.java.net/~fparain/805/

cc'ing core-libs-dev as part of this is clean-up in the library code too.

I think we should deprecate java.lang.Compiler and the Runtime.traceXXX
methods. They've been non-functional for a long time and having them in
the API is a bit mis-leading to anyone reading the javadoc. I realize
you are focused on the removing the old JVM_* functions so we can
follow-up on that via other issues of course.


Deprecating class and methods would require another CCC approval, so
I'd prefer to post-pone this change to another changeset.


Can ClassLoader#resolveClass0 can be removed completely? The null check
can be done in ClassLoader#resolveClass.


Done.


In the mapfile for libjava then the comment at line 281 says ZipFile.c
needs this one. As getLastErrorString is now exported for use by libzip
then the comment should probably be updated.


Done.


Otherwise this clean-up looks good to me and the jdk_core group of tests
is the right group to exercise this area.


Thank you, the new jdk webrev is here:
http://cr.openjdk.java.net/~fparain/805/jdk_v02/

Regards,

Fred

--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Re: RFR 6642881: Improve performance of Class.getClassLoader()

[Frederic Parain]

Hi Coleen,

It seems that there's still a reference to JVM_GetClassLoader
in file jdk/src/share/native/common/check_code.c. The code looks
like dead code, but it would be nice to clean it up.

Thanks,

Fred

On 06/24/2014 01:45 AM, Coleen Phillimore wrote:


Please review a change to the JDK code for adding classLoader field to
the instances of java/lang/Class.  This change restricts reflection from
changing access to the classLoader field.  In the spec,
AccessibleObject.setAccessible() may throw SecurityException if the
accessibility of an object may not be changed:

http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean)


Only AccessibleObject.java has changes from the previous version of this
change.

open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/
bug link https://bugs.openjdk.java.net/browse/JDK-6642881

Thanks,
Coleen

On 6/19/14, 11:01 PM, David Holmes wrote:

On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote:

Hi Mandy,

On 19 jun 2014, at 22:34, Mandy Chung mandy.ch...@oracle.com wrote:


On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote:

Hi,

On 19 jun 2014, at 20:46, Coleen Phillimore
coleen.phillim...@oracle.com wrote:

On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote:

Have you considered hiding the Class.classLoader field from
reflection? I’m not sure it is necessary but I’m not to keen on
the idea of people poking at this field with Unsafe (which should
go away in 9 but …).

I don't know how to hide the field from reflection.  It's a
private final field so you need to get priviledges to make it
setAccessible.  If you mean injecting it on the JVM side, the
reason for this change is so that the JIT compilers can inline
this call and not have to call into the JVM to get the class loader.


There is sun.reflect.Reflection.registerFieldsToFilter() that hides
a field from being found using reflection. It might very well be
the case that private and final is enough, I haven’t thought this
through 100%. On the other hand, is there a reason to give users
access through the field instead of having to use
Class.getClassLoader()?


There are many getter methods that returns a private final field.
I'm not sure if hiding the field is necessary nor a good precedence
to set. Accessing a private field requires accessDeclaredMember
permission although it's a different security check (vs
getClassLoader
permission).  Arguably before this new classLoader field, one could
call Class.getClassLoader0() via reflection to get a hold of class
loader.

Perhaps you are concerned that the accessDeclaredMember permission
is too coarse-grained?  I think the security team is looking into
the improvement in that regards.


I think I’m a bit worried about two things, first as you wrote,
“accessDeclaredMember” isn’t the same as “getClassLoader”, but since
you could get the class loader through getClassLoader0() that
shouldn’t be a new issue.

The second thing is that IIRC there are ways to set a final field
after initialization. I’m not sure we need to care about that either
if you need Unsafe to do it.


Normal reflection can set a final field if you can successfully call
setAccessible(true) on the Field object.

David
-



cheers
/Joel





--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

hg: jdk8/tl/jdk: 7150256: Add back Diagnostic Command JMX API

[frederic . parain]

Changeset: e857b2a3ecee
Author:fparain
Date:  2013-06-05 08:41 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e857b2a3ecee

7150256: Add back Diagnostic Command JMX API
Reviewed-by: mchung, jbachorik

! make/java/management/Exportedfiles.gmk
! make/java/management/FILES_c.gmk
! make/java/management/mapfile-vers
! makefiles/mapfiles/libmanagement/mapfile-vers
+ src/share/classes/com/sun/management/DiagnosticCommandMBean.java
! src/share/classes/java/lang/management/ManagementFactory.java
+ src/share/classes/sun/management/DiagnosticCommandArgumentInfo.java
+ src/share/classes/sun/management/DiagnosticCommandImpl.java
+ src/share/classes/sun/management/DiagnosticCommandInfo.java
! src/share/classes/sun/management/ManagementFactoryHelper.java
! src/share/classes/sun/management/VMManagement.java
! src/share/classes/sun/management/VMManagementImpl.java
! src/share/javavm/export/jmm.h
+ src/share/native/sun/management/DiagnosticCommandImpl.c
! src/share/native/sun/management/VMManagementImpl.c
+ 
test/com/sun/management/DiagnosticCommandMBean/DcmdMBeanDoubleInvocationTest.java
+ test/com/sun/management/DiagnosticCommandMBean/DcmdMBeanInvocationTest.java
+ test/com/sun/management/DiagnosticCommandMBean/DcmdMBeanPermissionsTest.java
+ test/com/sun/management/DiagnosticCommandMBean/DcmdMBeanTest.java
! test/java/lang/management/MXBean/MXBeanBehavior.java
! 
test/java/lang/management/ManagementFactory/MBeanServerMXBeanUnsupportedTest.java

Re: RFR: 7150256: Add back Diagnostic Command JMX API

[frederic parain]

 to check it in the Java level first calls
VMManagement.isRemoteDiagnosticCommandsSupported before calling the
native method. GetOptionalSupport is called during class initialization
to cache the values in Java to avoid fetching the value multiple time.

test/java/lang/management/MXBean/MXBeanBehavior.java
   line 39: diamond operator
   Since the test is for MXBeans, it's more appropriate to exclude
non-MXBean from this test or rename the test to cover the new platform
mbeans.


On 4/18/2013 7:23 AM, frederic parain wrote:


DiagnosticCommandImpl.Wrapper class
If there is any issue initializing the diagnostic command, it
ignores the exception.  That makes it very hard to diagnose when things
go wrong.  This exports diagnostic commands supported by the running JVM
and so I would think any error would be a bug.


An exception when creating the Wrapper doesn't necessarily mean a bug.
The call to get the list of diagnostic commands and the call to get
the descriptions of these commands cannot be performed in an atomic
way. Because the diagnostic command framework allows dynamic
addition and removal of commands, a command might disappear between
the two calls. In this case, the creation of the wrapper for this
command will fail, but this shouldn't be considered as a bug.



Can you add the comment there describing why the exception is ignored.
I'm not sure under what circumstances the exception is expected or not.

The Wrapper constructor throws InstantiationException when it fails to
initialize the permission class but  getMBeanInfo() ignores
InstantiationException.  It seems a bug in the implementation to me?  If
IAE and UOE during initiatizing the diagnostic commands, it returns an
empty one that seems okay.Comments to explain that would help.




The new tests hardcode the port number that is unreliable and may fail
in nightly/jprt testing (e.g. the port is occupied by another test
run).   Some management tests have the same reliability issue and I'm
not sure what the state is right now.  It'd be good if the new tests can
avoid using hardcode port number.


I don't know how to avoid the hardcoding of the port without wrapping
the test in a shell scripts. Is there a template available to do
dynamic port allocation?


I skimmed on some existing tests but not able to find the example. I
think it's still a clean up task to be done.  It's fine with me if you
do this test cleanup later and we probably need to write a test library
to help this.

Mandy


--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Re: RFR: 7150256: Add back Diagnostic Command JMX API

[frederic parain]

Thank you Mandy,

I'll file the bugs and fix the exception cause.

Fred

On 03/05/2013 20:02, Mandy Chung wrote:

Frederic,

This looks good. Thanks for the update and also move the native method
and implementations to DiagnosticCommandImpl.c and removing the check
for rdcmd_support which is much cleaner.

Minor comment:
DiagnosticCommandImpl.Wrapper constructor - it'd be good to catch the
exception causing the instantiation of permission instance to fail and
set it to the cause of InstantiationException.  Looks like
InstantiationException doesn't take cause in the constructor and you
would have to call initCause() before throwing it.

No need to generate the new webrev for this minor fix.  Ship it when
you're ready.

As we discussed offline, you will file bugs to follow up the following
issues:
1. PlatformManagedObject should be updated to include DynamicMBean as a
platform mbean and DiagnosticCommandMBean will implement
PlatformManagedObject.  ManagementFactory may provide new method to
obtain platform dynamic mbean.

2. Investigate what DiagnosticCommandImpl.getAttributes and
setAttributes should do per DynamicMBean spec.  The current
implementation throws UOE which seem to be okay.  It's good to confirm
what is specified or not specified per DynamicMBean spec

Thanks
Mandy

On 5/3/2013 9:43 AM, frederic parain wrote:

Hi all,

After an intense work with Mandy, here's a new webrev which
fix the issue with the PlatformManagedObject specification.
The DiagnosticCommandMBean is not a PlatformManagedObject
anymore, it's just a DynamicMBean registered to the platform
MBean server. Many smaller fixes have also been done based
on provided feedback.

http://cr.openjdk.java.net/~fparain/7150256/webrev.07/

Thanks,

Fred

On 24/04/2013 22:07, Mandy Chung wrote:

Hi Frederic,

I reviewed the jdk webrev that is looking good.  I reviewed
com.sun.management.DiagnosticCommandMBean spec almost half a year ago.
Reviewing it now with a fresh memory has some benefit that I have a few
comments on the spec.

java.lang.management.PlatformManagedObject is specified as JMX MXBean
while dynamic mbean is not a MXBean.  You have modified
ManagementFactory.getPlatformManagementInterfaces() to return the
DiagnosticCommandMBean which I agree it is the right thing to do.

The PlatformManagedObject spec should be revised to cover dynamic mbeans
for this extension.  The primary requirement for the platform mbeans is
to be interoperable so that a JMX client can access the platform mbeans
in a JVM running with a different JRE version (old or new) in which the
types are not required to be present in the JMX client.

ManagementFactory.getPlatformMXBean(DiagnosticCommandMBean.class) and
the getPlatformMXBeans method should throw IAE.  I think the existing
implementation already does that correctly but better to have a test to
catch that.  The spec says @throws IAE if the mxbeaninterface is not a
platform management interface.  The method name is very explict about
MXBean and so the @throws javadoc should be clarified it's not a
platform MXBean.

What will be the way to obtain DiagnosticCommandMBean?

Your DiagnosticCommandAsPlatformMBeanTest calls
ManagementFactory.getPlatformMXBean(DiagnosticCommandMBean.class) and
expect it to work. I think it should throw IAE instead. Nit: the
HOTSPOT_DIAGNOSTIC_MXBEAN_NAME field was leftover from your previous
version and should be removed.

DiagnosticCommandMBean specifies the meta-data of the diagnostic command
and the option/argument the command takes.  Have you considering
defining interfaces/abstract class for DiagnosticCommandInfo and
DiagnosticCommandArgumentInfo for a client to implement for parsing the
meta-data and maybeproviding factory methods? It's very easy to make
mistake without any API support.  The spec is definitely not easy to
read :)

dcmd.arg.type may be non-Java type.  What are the examples?  For Java
types, I suggest to use the type signatures as defined in JNI and
consistent with the standard representation:
http://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/types.html#wp276



dcmdArgInfo.position - would it be better to define a value (e.g. -1) if
dcmdArgInfo.option is set to true so that assertion can be added if
desired.

dcmd.permissionClass - s/class name/fully-qualified class name

The comment on java.lang.management spec needs to be addressed for this
change.  As for the comments on DiagnosticCommandMBean, it's fine with
me to integrate your current DiagnosticCommandMBean and follow up to
make the spec enhancement, if appropriate, as a separate changeset.

Is DiagnosticCommandMBean target for 7u?  It has @since 8.

The javadoc for DiagnosticCommandMBean should include more links such as
platform mbeanserver
(java.lang.management.ManagementFactory.getPlatformMBeanServer)
descriptor, parameter, etc, and the methods such as getName,
getDescription, etc
jmx.mbean.info.changed (MBeanInfo)

replace code../code with {@code ..}
line 32

Re: RFR: 7150256: Add back Diagnostic Command JMX API

[frederic parain]

I'll fix it and file a new bug.

Fred

On 03/05/2013 20:30, Mandy Chung wrote:

On 5/3/2013 11:19 AM, Daniel Fuchs wrote:

Hi,

On 5/3/13 8:02 PM, Mandy Chung wrote:

2. Investigate what DiagnosticCommandImpl.getAttributes and
setAttributes should do per DynamicMBean spec.  The current
implementation throws UOE which seem to be okay.  It's good to confirm
what is specified or not specified per DynamicMBean spec


By analogy with what the MBeanServerConnection says and what
the StandardMBean class does, I'd say that the expected behavior
would be to return an empty AttributeList for both methods.


Thanks Daniel.  I initially thought that these 2 methods should return
an empty AttributeList.  MBeanServerConnection specifies clearly that
missing attribute will be omitted and the caller will use the returned
value to determine any missing attribute.  It makes sense to expect
DynamicMBean.get/setAttributes the same behavior as MBS forwards the
call to the mbeans.

Frederic - you can simply fix this to return an empty AttributeList. Can
you also file a bug for DynamicMBean to clarify the expected behavior in
the specification?

Thanks
Mandy



See:
http://docs.oracle.com/javase/7/docs/api/javax/management/MBeanServer.html#getAttributes%28javax.management.ObjectName,%20java.lang.String[]%29


and
http://docs.oracle.com/javase/7/docs/api/javax/management/MBeanServer.html#setAttributes%28javax.management.ObjectName,%20javax.management.AttributeList%29





-- daniel



Thanks
Mandy






--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Re: RFR: 7150256: Add back Diagnostic Command JMX API

[frederic parain]

MBeanServer.setAttributes() returns an AttributeList
but the return type of DynamicMBean.setAttributes() is
void. Does it mean that in our case, setAttributes()
should simply be a no-op method?

Fred

On 03/05/2013 20:19, Daniel Fuchs wrote:

Hi,

On 5/3/13 8:02 PM, Mandy Chung wrote:

2. Investigate what DiagnosticCommandImpl.getAttributes and
setAttributes should do per DynamicMBean spec.  The current
implementation throws UOE which seem to be okay.  It's good to confirm
what is specified or not specified per DynamicMBean spec


By analogy with what the MBeanServerConnection says and what
the StandardMBean class does, I'd say that the expected behavior
would be to return an empty AttributeList for both methods.

See:
http://docs.oracle.com/javase/7/docs/api/javax/management/MBeanServer.html#getAttributes%28javax.management.ObjectName,%20java.lang.String[]%29


and
http://docs.oracle.com/javase/7/docs/api/javax/management/MBeanServer.html#setAttributes%28javax.management.ObjectName,%20javax.management.AttributeList%29


-- daniel



Thanks
Mandy




--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Re: RFR: 7150256: Add back Diagnostic Command JMX API

[frederic parain]

You're right. I'll update setAttributes() to return
an empty AttributeList.

Fred

On 03/05/2013 20:53, Mandy Chung wrote:

DynamicMBean:
 public AttributeList setAttributes(AttributeList attributes);

Maybe you looked at setAttribute method.  It's easily misread with and
without 's'.

On 5/3/2013 11:40 AM, frederic parain wrote:

MBeanServer.setAttributes() returns an AttributeList
but the return type of DynamicMBean.setAttributes() is
void. Does it mean that in our case, setAttributes()
should simply be a no-op method?

Fred

On 03/05/2013 20:19, Daniel Fuchs wrote:

Hi,

On 5/3/13 8:02 PM, Mandy Chung wrote:

2. Investigate what DiagnosticCommandImpl.getAttributes and
setAttributes should do per DynamicMBean spec.  The current
implementation throws UOE which seem to be okay.  It's good to confirm
what is specified or not specified per DynamicMBean spec


By analogy with what the MBeanServerConnection says and what
the StandardMBean class does, I'd say that the expected behavior
would be to return an empty AttributeList for both methods.

See:
http://docs.oracle.com/javase/7/docs/api/javax/management/MBeanServer.html#getAttributes%28javax.management.ObjectName,%20java.lang.String[]%29



and
http://docs.oracle.com/javase/7/docs/api/javax/management/MBeanServer.html#setAttributes%28javax.management.ObjectName,%20javax.management.AttributeList%29



-- daniel



Thanks
Mandy








--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Re: RFR: 7150256/8004095: Add back Remote Diagnostic Commands

[frederic parain]

Karen,

Thank you for the review.

1. 2. and 3. have been fixed.

Regarding 4.

Most (all?) permissions have a name argument.
However, if p._name is null, the string (null) is printed.
This is not wrong, but not very pretty:
Permission: MyPermission((null)).

I've changed the code (both 105 and 109) with a short conditional:
   ... p._name == NULL ? null : p._name ...

which gives a better output:

Permission: MyPermission(null)

Fred

On 02/05/2013 18:37, Karen Kinnear wrote:

Frederic,

Code looks good - actually it looks very clean. Ship it.

Couple of minor comments that don't require re-review:

1. nmtDCmd.hpp/cpp - copyrights 2012 - 2012, 2013

2. jmm.h
   line 213: True is - True if

3. diagnosticFramework.hpp
   Thank you for the comments!
   line 298 rational - rationale

4. diagnosticCommand.cpp
   lines 105/109 - what prints if p._name is null?

thanks,
Karen

On Apr 30, 2013, at 12:26 PM, frederic parain wrote:


Hi all,

This is a second request for review to add back
Remote Diagnostic Commands.

This work adds a new platform MBean providing
remote access to the diagnostic command framework
via JMX (already accessible locally with the jcmd
tool).

There's two CR number because this work is made of two
parts pushed to two different repositories.

JDK changeset CR 7150256
http://cr.openjdk.java.net/~fparain/7150256/webrev.06/

HotSpot changeset: CR 8004095
http://cr.openjdk.java.net/~fparain/8004095/webrev.06/

Questions from previous review have been answered
in initial review threads. Changesets also include
some minor changes coming from internal audit and
feedback sent in private e-mails.

However, one issue is still pending: some unit tests
use a hard coded port number, which could cause test
failures if several instances of the same test are
run on the same machine. I propose to postpone the
fix of this issue after the JDK8 feature freeze
(leaving for vacations soon, I won't have time to
fix tests before the feature freeze).

Thanks,

Fred

--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com




--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

RFR: 7150256/8004095: Add back Remote Diagnostic Commands

[frederic parain]

Hi all,

This is a second request for review to add back
Remote Diagnostic Commands.

This work adds a new platform MBean providing
remote access to the diagnostic command framework
via JMX (already accessible locally with the jcmd
tool).

There's two CR number because this work is made of two
parts pushed to two different repositories.

JDK changeset CR 7150256
http://cr.openjdk.java.net/~fparain/7150256/webrev.06/

HotSpot changeset: CR 8004095
http://cr.openjdk.java.net/~fparain/8004095/webrev.06/

Questions from previous review have been answered
in initial review threads. Changesets also include
some minor changes coming from internal audit and
feedback sent in private e-mails.

However, one issue is still pending: some unit tests
use a hard coded port number, which could cause test
failures if several instances of the same test are
run on the same machine. I propose to postpone the
fix of this issue after the JDK8 feature freeze
(leaving for vacations soon, I won't have time to
fix tests before the feature freeze).

Thanks,

Fred

--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Re: RFR: 7150256: Add back Diagnostic Command JMX API

[frederic parain]

Mandy,

Thanks for the review

New webrevs are available here:

7150256:
http://cr.openjdk.java.net/~fparain/7150256/webrev.05/

8004095:
http://cr.openjdk.java.net/~fparain/8004095/webrev.05/

My answers to your questions are in-lined below.

On 21/12/2012 00:40, Mandy Chung wrote:

Hi Frederic,

It's good to see the management interface for diagnostic commands is back.

On 12/17/12 6:58 AM, Frederic Parain wrote:


http://cr.openjdk.java.net/~fparain/7150256/webrev.01/


Please send your makefiles change to build-dev and build-infra for
review to ensure necessary change is made in the new build-infra work.


makefiles have been reviewed and approved by the build team.


I didn't have time to a detailed review and I think it looks okay. Some
comments:

DiagnosticCommandImpl.Wrapper class
If there is any issue initializing the diagnostic command, it
ignores the exception.  That makes it very hard to diagnose when things
go wrong.  This exports diagnostic commands supported by the running JVM
and so I would think any error would be a bug.


An exception when creating the Wrapper doesn't necessarily mean a bug.
The call to get the list of diagnostic commands and the call to get
the descriptions of these commands cannot be performed in an atomic
way. Because the diagnostic command framework allows dynamic
addition and removal of commands, a command might disappear between
the two calls. In this case, the creation of the wrapper for this
command will fail, but this shouldn't be considered as a bug.


DiagnosticCommandArgumentInfo is non-public class but its constructor
and methods are still public.


Fixed.


The new tests hardcode the port number that is unreliable and may fail
in nightly/jprt testing (e.g. the port is occupied by another test
run).   Some management tests have the same reliability issue and I'm
not sure what the state is right now.  It'd be good if the new tests can
avoid using hardcode port number.


I don't know how to avoid the hardcoding of the port without wrapping
the test in a shell scripts. Is there a template available to do
dynamic port allocation?

Thanks,

Fred

--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

RFR: 7150256: Add back Diagnostic Command JMX API

[Frederic Parain]

Hi,

Please review the following change for CR 7150256.

This changeset is the JDK side of two parts project.

The goal of this feature is to allow remote invocations of
Diagnostic Commands via JMX.

Diagnostic Commands are actually invoked from the jcmd
tool, which is a local tool using the attachAPI. It was
not possible to remotely invoke these commands.
This changeset creates a new PlatformMBean, remotely
accessible via the Platform MBean Server, providing
access to Diagnostic Commands too.

The set of supported Diagnostic Commands varies with
the JVM version, command line options and even some
runtime operations. The new PlatformMBean, called
DiagnosticCommandsMBean is not statically defined,
but computes its content dynamically at runtime.
This is why the MBeanInfo returned by this new
MBean contains a lot of meta-data describing
each supported Diagnostic Command (name, help
message, syntax, etc.)

Because this feature allows remote invocations, it
is important to be able to control who is invoking
and what is invoked. Java Permission checks have
been introduced before a remote invocation request
is forwarded to the JVM. So, each Diagnostic Command
can require a Java Permission to be invoked remotely.
The name of the required Java Permission is provided
in the meta-data in the MBeanInfo.
The security configuration is done using a Security
Manager and the regular JMX configurations. Some
existing Diagnostic Commands have been extended to
specify which Java Permission they require.

The webrev is here:

http://cr.openjdk.java.net/~fparain/7150256/webrev.00/

The second part of this project is the support for this
feature in the HotSpot VM, and is tracked by CR 8004095
(separate request for review will be sent for it).

Thanks,

Fred

--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

hg: jdk8/tl/jdk: 7156831: The jcmd man page is not included in generated bundles

[frederic . parain]

Changeset: 62228dc7a4ac
Author:fparain
Date:  2012-03-28 02:20 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/62228dc7a4ac

7156831: The jcmd man page is not included in generated bundles
Reviewed-by: dholmes, sla, dsamersoff

! make/common/Release.gmk

hg: jdk8/tl/jdk: 7074616: java.lang.management.ManagementFactory.getPlatformManagementInterfaces fails

[frederic . parain]

Changeset: 0e4f259f0a1f
Author:fparain
Date:  2012-03-14 09:03 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0e4f259f0a1f

7074616: java.lang.management.ManagementFactory.getPlatformManagementInterfaces 
fails
Reviewed-by: dholmes, sla

! src/share/classes/java/lang/management/ManagementFactory.java
+ 
test/java/lang/management/ManagementFactory/GetPlatformManagementInterfaces.java

Re: RFR: 6988220: java.lang.ObjectName use of String.intern() causes major performance issues at scale

[Frederic Parain]

I'm in favor of not adding complexity to ObjectName.equals().
The goal of this CR is to remove a bottleneck created by the use
of String.intern() in ObjectName's constructors. This CR doesn't
aim to optimize the ObjectName.equals() method.

An application can define an optimized method to compare two
ObjectNames based on its knowledge of the patterns used by its
ObjectNames.

However, there's no simple way to workaround the bottleneck created
by the String.intern() call. The Ops-Center team has experimented
the removal of String.intern() by providing their own implementation
of ObjectName and playing with the bootclasspath, but this is not
a solution easy to deploy in a production environment.

Fred

On 2/23/12 8:44 PM, Eamonn McManus wrote:

I am not sure it is worth the complexity of extra checks. Do you have
data showing that ObjectName.equals usually returns false? In a
successful HashMap lookup, for example, it will usually return true
since the equals method is used to guard against collisions, and
collisions are rare by design. Meanwhile, String.equals is intrinsic in
HotSpot so we may assume that it is highly optimized, and you are giving
up that optimization if you use other comparisons.

Éamonn


On 23 February 2012 10:52, Olivier Lagneau olivier.lagn...@oracle.com
mailto:olivier.lagn...@oracle.com wrote:

Hi Frederic,

Performance and typo comments.

Regarding performance of ObjectName.equals method, which is certainely
a frequent call on ObjectNames, I think that using inner fields
(Property array for canonical name and domain length) would be more
efficient
than using String.equals() on these potentially very long strings.

Two differents objectNames may often have the same length with
different key/properties values, and may often be different only
on the last property of the canonical name.

The Property array field ca_array (comparing length and property
contents)
and domain length are good candidates to filter out more efficiently
different objectNames, knowing that String.equals will compare every
single char of the two char arrays.

So for performance purpose, I suggest to filter out different
objectNames
by doing inner comparisons in the following order : length of the two
canonical names, then domain_length, then ca_array size, then its
content,
and lastly if all of this fails to filter out, then use String.equals.

  _canonicalName = (new String(canonical_chars, 0, prop_index));

Typo : useless parentheses.

Thanks,
Olivier.

-- Olivier Lagneau, olivier.lagn...@oracle.com
mailto:olivier.lagn...@oracle.com
Oracle, Grenoble Engineering Center - France
Phone : +33 4 76 18 80 09 tel:%2B33%204%2076%2018%2080%2009 Fax :
+33 4 76 18 80 23 tel:%2B33%204%2076%2018%2080%2023




Frederic Parain said  on date 2/23/2012 6:01 PM:

No particular reason. But after thinking more about it,
equals() should be a better choice, clearer code, and
the length check in equals() implementation is likely
to help performance of ObjectName's comparisons as
ObjectNames are often long with a common section at the
beginning.

I've updated the webrev:
http://cr.openjdk.java.net/~__fparain/6988220/webrev.01/
http://cr.openjdk.java.net/~fparain/6988220/webrev.01/

Thanks,

Fred

On 2/23/12 4:58 PM, Vitaly Davidovich wrote:

Hi Frederic,

Just curious - why are you checking string equality via
compareTo()
instead of equals()?

Thanks

Sent from my phone

On Feb 23, 2012 10:37 AM, Frederic Parain
frederic.par...@oracle.com mailto:frederic.par...@oracle.com
mailto:frederic.parain@__oracle.com
mailto:frederic.par...@oracle.com wrote:

This a simple fix to solve CR 6988220:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6988220
http://bugs.sun.com/__bugdatabase/view_bug.do?bug___id=6988220
http://bugs.sun.com/__bugdatabase/view_bug.do?bug___id=6988220
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6988220

The use of String.intern() in the ObjectName class prevents
the class the scale well when more than 20K ObjectNames are
managed. The fix simply removes the use of String.intern(),
and uses regular String instead. The Object.equals() method
is modified too to make a regular String comparison. The
complexity of this method now depends on the length of
the ObjectName's canonical name, and is not impacted any
more by the number of ObjectName instances being handled.

The Webrev:
http://cr.openjdk.java.net/~fparain/6988220/webrev.00/
http

Re: RFR: 6988220: java.lang.ObjectName use of String.intern() causes major performance issues at scale

[Frederic Parain]

Making String.intern() more scalable doesn't seem to be a
solution for short (or medium?) time frame. Even if the
computation cost of ObjectName.equals() is increased by
this fix, there's no performance measurement in favor or
against this change. I've looked for benchmarks stressing
the ObjectName class, but I haven't found one yet.

Using java.util Set or Map introduces new issues, like
the memory footprint and new synchronizations to protect
consistency of the collection.

The Ops-Center team is stuck with this scalability issue
for two years now. They have identified the problem in JDK6
and we're not able to provide them with a solution for JDK7
or JDK8.

David, I agree that I have no data about the impact on other
use-cases, but I know that the use of String.intern() cannot
be easily workaround. We can remove the use of String.intern()
and if the performance of the new ObjectName.equals() method
really becomes a performance bottleneck for other use-cases,
them we can re-work this method to improve its performance.
But I'd prefer not starting complex optimizations on a method
without having real indication that it causes real performance
issues.

Fred

On 2/23/12 11:23 PM, Keith McGuigan wrote:


Making String.intern() more scalable has been on our list of
things-to-do for a long, long time. But, it's not trivial. Simply
increasing the size of the hashtable is no good because we'd be upping
our footprint unconditionally, so really we want a growing hashtable
which is a bit more effort (though not impossible, of course, it just
hasn't bubbled up to the top of the priority list).

Another problem with using 'intern()' is that when you intern a string
you're placing it into the permgen, and space there is at a premium. (no
perm gen project will hopefully fix this soon).

If you really want to use == instead of equals(), you can use a
java.util set or map data structure and stash all of your strings in
there. Then you'll have canonicalized references that == will work upon,
and you won't run into the intern() scalability (or concurrency) issues.

--
- Keith


On 2/23/2012 4:53 PM, David Holmes wrote:

Hi Fred,

java.lang.ObjectName? :) Need to fix that.

So often we intern strings precisely so that equals() can use == to
improve performance.

It seems to me that this is a case of fixing something for one
use-case without knowing what the impact will be on other use-cases!

Is there perhaps a solution that makes String.intern more scalable?

David
-

On 24/02/2012 1:36 AM, Frederic Parain wrote:

This a simple fix to solve CR 6988220:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6988220

The use of String.intern() in the ObjectName class prevents
the class the scale well when more than 20K ObjectNames are
managed. The fix simply removes the use of String.intern(),
and uses regular String instead. The Object.equals() method
is modified too to make a regular String comparison. The
complexity of this method now depends on the length of
the ObjectName's canonical name, and is not impacted any
more by the number of ObjectName instances being handled.

The Webrev:
http://cr.openjdk.java.net/~fparain/6988220/webrev.00/

I've tested this fix with the jdk_lang and jdk_management
test suites.

Thanks,

Fred



--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Re: RFR: 6988220: java.lang.ObjectName use of String.intern() causes major performance issues at scale

[Frederic Parain]

On 2/23/12 7:46 PM, David Schlosnagle wrote:

Was the main bottleneck the contention on the interned string pool
that prevented concurrent addition of ObjectNames? Are there other
places within the JDK where use of intern() should be analyzed for
similar scalability bottlenecks? I'm also curious what the heap
implications are of no longer using interned strings.


I haven't looked for similar use of intern() within the JDK.
However, the scalability issue of String.intern() is known for
a long term, but the fix is not that simple, as Keith explained,
this is why it has been delayed for a long time due to other
higher priority tasks.


A minor nit is that the equals method could be simplified slightly,
making it more clear that the canonical names must match for equality:

@Override
public boolean equals(Object object)  {
 // same object case
 if (this == object) return true;

 // object is not an object name case
 if (!(object instanceof ObjectName)) return false;
 ObjectName on = (ObjectName) object;
 return _canonicalName.equals(on._canonicalName);
}


Thanks,

Fred

--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

RFR: 6988220: java.lang.ObjectName use of String.intern() causes major performance issues at scale

[Frederic Parain]

This a simple fix to solve CR 6988220:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6988220

The use of String.intern() in the ObjectName class prevents
the class the scale well when more than 20K ObjectNames are
managed. The fix simply removes the use of String.intern(),
and uses regular String instead. The Object.equals() method
is modified too to make a regular String comparison. The
complexity of this method now depends on the length of
the ObjectName's canonical name, and is not impacted any
more by the number of ObjectName instances being handled.

The Webrev:
http://cr.openjdk.java.net/~fparain/6988220/webrev.00/

I've tested this fix with the jdk_lang and jdk_management
test suites.

Thanks,

Fred

--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Re: RFR: 6988220: java.lang.ObjectName use of String.intern() causes major performance issues at scale

[Frederic Parain]

No particular reason. But after thinking more about it,
equals() should be a better choice, clearer code, and
the length check in equals() implementation is likely
to help performance of ObjectName's comparisons as
ObjectNames are often long with a common section at the
beginning.

I've updated the webrev:
http://cr.openjdk.java.net/~fparain/6988220/webrev.01/

Thanks,

Fred

On 2/23/12 4:58 PM, Vitaly Davidovich wrote:

Hi Frederic,

Just curious - why are you checking string equality via compareTo()
instead of equals()?

Thanks

Sent from my phone

On Feb 23, 2012 10:37 AM, Frederic Parain frederic.par...@oracle.com
mailto:frederic.par...@oracle.com wrote:

This a simple fix to solve CR 6988220:
http://bugs.sun.com/__bugdatabase/view_bug.do?bug___id=6988220
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6988220

The use of String.intern() in the ObjectName class prevents
the class the scale well when more than 20K ObjectNames are
managed. The fix simply removes the use of String.intern(),
and uses regular String instead. The Object.equals() method
is modified too to make a regular String comparison. The
complexity of this method now depends on the length of
the ObjectName's canonical name, and is not impacted any
more by the number of ObjectName instances being handled.

The Webrev:
http://cr.openjdk.java.net/~__fparain/6988220/webrev.00/
http://cr.openjdk.java.net/~fparain/6988220/webrev.00/

I've tested this fix with the jdk_lang and jdk_management
test suites.

Thanks,

Fred

--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17 tel:%2B33%204%2076%2018%2081%2017
Email: frederic.par...@oracle.com mailto:frederic.par...@oracle.com



--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Re: Request for Review: 7144833 sun/tools/jcmd/jcmd-Defaults.sh failing intermittently

[Frederic Parain]

Thanks Alan,

I've fixed the indentation in the BEGIN block.

Fred

On 02/15/12 10:04 AM, Alan Bateman wrote:

On 14/02/2012 11:01, Frederic Parain wrote:

This is a request for review for 7144833:
sun/tools/jcmd/jcmd-Defaults.sh failing intermittently

Bug:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7144833

The problem occurs when one or several lines printed by the jcmd tool
match several rules in the awk script checking the output. The counter
counting the number of lines matching the expected pattern is
incremented each time the line matches a rule. If the line matches
several rules, the counter is incremented several times. When this
occurs, the test at the end of the script checking that the number
of lines matching the pattern is equal to the number of lines of the
output fails.

The proposed fix is a modification of the awk script. Each rule
checking a pattern now sets a variable to 1 when a match is found,
and the variable is added to the counter only once per line.

Here's the Webrev:

http://cr.openjdk.java.net/~fparain/7144833/webrev.00/

The webrev also contains the removal of the sun/tools/jps/jps-Vvml.sh
test from the problem list. The test has been fixed (see CR 6986875)
but the test was still in the exclusion list.

Looks good to me although you might want to check the indentation in the
BEGIN block.

Also thanks for updating the ProblemList file.

-Alan


--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

hg: jdk8/tl/jdk: 7144833: sun/tools/jcmd/jcmd-Defaults.sh failing intermittently

[frederic . parain]

Changeset: 59884f656b7d
Author:fparain
Date:  2012-02-15 09:29 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/59884f656b7d

7144833: sun/tools/jcmd/jcmd-Defaults.sh failing intermittently
Reviewed-by: alanb

! test/ProblemList.txt
! test/sun/tools/jcmd/jcmd_Output1.awk

hg: jdk8/tl/jdk: 7145925: Removing remote access to diagnostic commands in the HotSpotDiagnosticMBean

[frederic . parain]

Changeset: 20d39a0e6fdc
Author:fparain
Date:  2012-02-15 10:46 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/20d39a0e6fdc

7145925: Removing remote access to diagnostic commands in the 
HotSpotDiagnosticMBean
Reviewed-by: acorn, mchung, phh

! make/java/management/mapfile-vers
- src/share/classes/com/sun/management/DiagnosticCommandArgumentInfo.java
- src/share/classes/com/sun/management/DiagnosticCommandInfo.java
! src/share/classes/com/sun/management/HotSpotDiagnosticMXBean.java
! src/share/classes/sun/management/HotSpotDiagnostic.java
! src/share/native/sun/management/HotSpotDiagnostic.c
- test/com/sun/management/HotSpotDiagnosticMXBean/ExecuteDiagnosticCommand.java
- test/com/sun/management/HotSpotDiagnosticMXBean/GetDiagnosticCommandInfo.java
- test/com/sun/management/HotSpotDiagnosticMXBean/GetDiagnosticCommands.java

hg: jdk8/tl/jdk: 7120974: ManagementPermission control needs clarification

[frederic . parain]

Changeset: ac26d04e76c3
Author:fparain
Date:  2012-02-01 03:52 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ac26d04e76c3

7120974: ManagementPermission control needs clarification
Reviewed-by: mchung, dholmes

! src/share/classes/java/lang/management/ManagementPermission.java

hg: jdk8/tl/jdk: 7104647: Adding a diagnostic command framework

[frederic . parain]

Changeset: 0194fe5ca404
Author:fparain
Date:  2012-01-04 03:49 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0194fe5ca404

7104647: Adding a diagnostic command framework
Reviewed-by: mchung, dholmes

! make/common/Release.gmk
! make/java/management/mapfile-vers
! make/launchers/Makefile
! make/sun/tools/Makefile
+ src/linux/doc/man/jcmd.1
+ src/share/classes/com/sun/management/DiagnosticCommandArgumentInfo.java
+ src/share/classes/com/sun/management/DiagnosticCommandInfo.java
! src/share/classes/com/sun/management/HotSpotDiagnosticMXBean.java
! src/share/classes/sun/management/HotSpotDiagnostic.java
! src/share/classes/sun/tools/attach/HotSpotVirtualMachine.java
+ src/share/classes/sun/tools/jcmd/Arguments.java
+ src/share/classes/sun/tools/jcmd/JCmd.java
! src/share/javavm/export/jmm.h
! src/share/native/sun/management/HotSpotDiagnostic.c
+ src/solaris/doc/sun/man/man1/jcmd.1
+ test/com/sun/management/HotSpotDiagnosticMXBean/ExecuteDiagnosticCommand.java
+ test/com/sun/management/HotSpotDiagnosticMXBean/GetDiagnosticCommandInfo.java
+ test/com/sun/management/HotSpotDiagnosticMXBean/GetDiagnosticCommands.java
! test/sun/tools/common/CommonSetup.sh
+ test/sun/tools/jcmd/dcmd-script.txt
+ test/sun/tools/jcmd/help_help.out
+ test/sun/tools/jcmd/jcmd-Defaults.sh
+ test/sun/tools/jcmd/jcmd-f.sh
+ test/sun/tools/jcmd/jcmd-help-help.sh
+ test/sun/tools/jcmd/jcmd-help.sh
+ test/sun/tools/jcmd/jcmd-pid.sh
+ test/sun/tools/jcmd/jcmd_Output1.awk
+ test/sun/tools/jcmd/jcmd_pid_Output1.awk
+ test/sun/tools/jcmd/jcmd_pid_Output2.awk
+ test/sun/tools/jcmd/usage.out

Re: Request for Review (XXL): 7104647: Adding a diagnostic command framework

[Frederic Parain]

Hi Dan,

Thank you for the review.

The new webrev is here:
http://cr.openjdk.java.net/~fparain/7104647/webrev.hotspot.05/

And my answers are in-lined below.

On 12/12/11 09:08 PM, Daniel D. Daugherty wrote:

src/share/vm/services/attachListener.cpp
The new include should be in alpha-order (between lines 36  37).
I'm pretty sure that's the new style...


Fixed


Block comment on lines 152-155 should be in '//' style and not
in JavaDoc style (/** ... */)


Fixed


src/share/vm/services/jmm.h
lines 192-208 - HotSpot convention is to mimic the existing style
in a file. For these structs, the field names should all be
lined up (see 181-188 for an example).


Fixed


src/share/vm/services/management.cpp
line 2126 calls JNIHandles::make_local(), but this is a JVM_ENTRY
wrapped function which means it depends on VM_ENTRY_BASE which
has a HandleMarkCleaner in it. Since this is a make_local()
call, won't the local JNIHandle get scrubbed on the way back
to the caller?


Threads have two areas to store handles, one for internal
handles and one for JNI handles. The HandleMarkCleaner
applies only to the internal handles. The purpose of
the make_local() is to create a JNI handle from the
internal handle, so when the internal area is cleaned
up by the HandleMarkCleaner, the JNI handle is still
valid in the context of the caller.


src/share/vm/services/diagnosticArgument.hpp
lines 45-50 - too much indent; should be 4 spaces


Fixed


src/share/vm/services/diagnosticArgument.cpp
HotSpot style is generally 'if (' and not 'if('


Fixed


src/share/vm/services/diagnosticCommand.hpp
lines 28-36 - includes should be in alpha order


Fixed


lines 44, 64 - Parameter defaults in new code is generally frowned
upon in HotSpot. They're used when adding a parameter to existing
code to avoid changing existing calls where the default is OK.
(I happen to disagree with that last part and I think that all
calls should be updated just to make sure your reviewers see
all uses, but I'm just one cranky voice... :-))


I've removed all default parameters I found.


src/share/vm/services/diagnosticCommand.cpp
line 28: includes should be in alpha order


Fixed


lines 31-33 - should be some indent here


Fixed


line 74: It would be useful to display the command that
can't be found:

help foo
Help unavailable: 'foo': No such command


Done.


line 101: just FYI, a ResourceMark without a Thread parameter can
be expensive. Not an issue for HelpDCmd::num_arguments().


I was not aware of the performance penalty. However, this method
is called only once in the lifetime of the VM, so I didn't fixed it.


line 103: HotSpot style is generally 'if (' and not 'if('


Fixed


src/share/vm/services/diagnosticFramework.hpp
line 38: typo: 'provides method' - 'provides methods'
line 40: typo: 'keywor' - 'keyword'


Fixed.


lines 43-46 - fields nicely indented here, but not in other new
header files. Any particular reason?


Yes, I got several reviews about the style :-)


lines 48, 154, 218, 286, 324 - Parameter defaults again.


Removed


line 55: is_stop should be:
return !is_empty()  strncmp(stop, _cmd, _cmd_len) == 0;
!strncmp() is frowned upon and spaces between params


OK. Fixed


line 82 - returns a local variable; that shouldn't work.


The method doesn't really return a local variable, this is a
return by value. The return statement invokes the copy constructor
to duplicate the local object into the caller context. Here, the
default copy constructor is called but it's okay because the CmdLine
class has be designed to work this way. I've added a comment in the
code to make this mechanism more explicit.


line 155: missing a space after '='


Fixed


lines 256, 258: HotSpot style is generally 'if (' and not 'if('


Fixed


line 304: typo: 'DCmdFActory' - 'DCmdFactory'


Fixed


line 306: typo: 'command to be executed' - 'command from being executed'


Fixed


src/share/vm/services/diagnosticFramework.cpp
line 55: _cmd_len = cmd_end - line;
This length includes any leading white space that was skipped
on lines 42-44. It seems odd that '_cmd' points to the first
non-whitespace in the command, but _cmd_len includes the
leading white space. If you change the _cmd_len calculation,
then you have to also change the logic on line 58 so that
args_len is also not too long.


You're right, this is definitively odd.
I've fixed _cmd_len and updated line 58.


lines 79, 104: typo: 'simple' - 'single'


Fixed


line 318 - what is 'idx' used for?


Remaining code from a previous version.
Unused now, so I removed it.


line 367: HotSpot style is generally 'if (' and not 'if('
lines 371, 372, 380, 403, 416: space after comma


Fixed


Thank you,

Fred

--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Re: Request for Review (XXL): 7104647: Adding a diagnostic command framework

[Frederic Parain]

Hi Serguei,

Thanks for the review.
I've applied the changes and the new webrev is here:
http://cr.openjdk.java.net/~fparain/7104647/webrev.hotspot.05/

Regards,

Fred

On 12/13/11 10:43 AM, serguei.spit...@oracle.com wrote:

Hi Frederik,

Your fix is already in a good shape!

src/share/vm/services/management.cpp:

   It is better to have different diagnostic messages at lines 2181 and 2186
   so that it is easy to find out what of the two lines caused an exception.
   The messages would be better to be more specific.
   The same applies to the lines 2221 and 2226.
   Indent must be aligned with the first argument above.

2178   oop cmd = JNIHandles::resolve_external_guard(command);
2179   if (cmd == NULL) {
2180 THROW_MSG(vmSymbols::java_lang_NullPointerException(),
2181Command line cannot be null.);
2182   }
2183   char* cmd_name = java_lang_String::as_utf8_string(cmd);
2184   if (cmd_name == NULL) {
2185 THROW_MSG(vmSymbols::java_lang_NullPointerException(),
2186Command line cannot be null.);
2187   }
...
2219   if (cmd == NULL) {
2220 THROW_MSG_NULL(vmSymbols::java_lang_NullPointerException(),
2221Command line cannot be null.);
   }
2223   char* cmdline = java_lang_String::as_utf8_string(cmd);
2224   if (cmdline == NULL) {

2225 THROW_MSG_NULL(vmSymbols::java_lang_NullPointerException(),
2226Command line cannot be null.);
2227   }

The lines 2189 and 2194 look redundant:

src/share/vm/services/diagnosticFramework.cpp:

2188   DCmd* dcmd = NULL;
2189   {
2190 DCmdFactory*factory = DCmdFactory::factory(cmd_name, strlen(cmd_name));
2191 if (factory != NULL) {
2192   dcmd = factory-create_resource_instance(NULL);
2193 }
2194   }

Indent is not correct at the lines 2205-2211:

2204   for (int i = 0; i  array-length(); i++) {
2205   infoArray[i].name = array-at(i)-name();
2206   infoArray[i].description = array-at(i)-description();
2207   infoArray[i].type = array-at(i)-type();
2208   infoArray[i].default_string = array-at(i)-default_string();
2209   infoArray[i].mandatory = array-at(i)-is_mandatory();
2210   infoArray[i].option = array-at(i)-is_option();
2211   infoArray[i].position = array-at(i)-position();
2212   }


Space is missed after 'while':

320   while(arg != NULL) {
326   while(arg != NULL) {


Thanks,
Serguei



On 12/12/11 7:56 AM, Frederic Parain wrote:

Minor updates:
http://cr.openjdk.java.net/~fparain/7104647/webrev.hotspot.04/

Fred

On 12/12/11 03:29 PM, Frederic Parain wrote:

Hi Paul,

Thank you for the review.
I've applied all your recommendations except the refactoring
in diagnosticCommandFramework.cpp (too few lines can be really
factored out without passing many arguments).

New webrev is here:
http://cr.openjdk.java.net/~fparain/7104647/webrev.hotspot.03/

Regards,

Fred

On 12/ 8/11 07:26 PM, Paul Hohensee wrote:

For the hotspot part at
http://cr.openjdk.java.net/~fparain/7104647/webrev.hotspot.00/

Most of my comments are style-related. Nice job on the implementation
architecture.

In attachListener.cpp:

You might want to delete the first return JNI_OK; line, since the
code
under
HAS_PENDING_EXCEPTION just falls through.

In jmm.h:

Be nice to indent (JNIEnv on lines 318, 319 and 325 the same as the
existing declarations. Add a newline just before it on line 322.


In diagnosticFramework.hpp:

Fix indenting for lines 63-66, insert blank before size_t on line 48.

Hotspot convention is that getter method names don't include a get_
prefix.
So, e.g., DCmdArgIter::get_key_addr() s/b DCmdArgIter::key_addr().
Similarly, getters such as is_enabled() should retrieve a field named
_is_enabled
rather than one named enabled. You follow the _is_enabled
convention
in other places such as GenDCmdArgument. Or you could use enabled() to
get the value of the _enabled field.

Also generally, I'd use accessor methods in the implementation of more
complex member methods rather than access the underlying fields
directly.
E.g. in GenDCmdArgument::read_value, I'd use is_set() and
set_is_set(true),
(set_is_set() is not actually defined, but should be) rather than
directly
accessing _is_set. Though sometimes doing this is too much of a pain
with fields whose type is a template argument, as in the
DCmdArggumentchar*::parse_value() method in diagnosticArgument.cpp.

For easy readability, it'd be nice to line up field names (the ones
with an
_ prefix) at the same column.

On line 200, instanciated - instantiated

On line 218, I'd use heap_allocated rather than heap for the formal
arg name.

On line 248, you could indent the text to start underneath
outputStream.
I generally find that indenting arguments lines (formal or actual) so
they line
up with the first argument position make the code more readable, but
I'm
not
religious about it.

On line 265, instanciated - instantiated

DCmdFactorys are members of a singly-linked list, right? If so, it'd be
good to
have

Re: Request for Review (XXL): 7104647: Adding a diagnostic command framework

[Frederic Parain]

On 12/7/11 2:32 AM, Mandy Chung wrote:

On 12/2/2011 5:57 AM, Frederic Parain wrote:


http://cr.openjdk.java.net/~fparain/7104647/webrev.jdk.00/

L112: Since you are in this file, can you fix the typo
java.security.SecurityException to java.lang.SecurityException?


Fixed.


The execute method doesn't throw any exception other than
IAE. What happens if the specified command execution fails
in the target VM? If no exception is thrown, how does the
caller detect if the command succeeds or not and handle it
gracefully? It seems that this mechanism should provide
a way to detect if a command succeeds or fails programmatically
rather than burying the error message in the returned string.


This changeset contains the framework, and the framework
only throws NPE or IAE. However, a diagnostic command
impementation is free to throw any other exception. If such
an exception is thrown, it is simply printed on the output
stream if the command has been invoked through the attachAPI,
or it is propagated like any other exception through JMX
if the command has been invoked from the HotSpotDiagnosticMXBean.


HotSpotDiagnostic.java
L45-49: jvm is not used and can be removed.


Removed.


L133: do you need to check if command == null and throw NPE?
or NPE will be thrown somewhere?


The native code is protected against null commands, it will
throw the NPE.


L159: NPE should be thrown instead of IAE, right? Unless
the javadoc for the execute method is modified to reflect that.


Fixed.


L176: minor: I wonder if it's any simpler for this native method
to take the DiagnosticCommandInfo[] argument (i.e. the caller
method does the array allocation than in the native method
implementation).


It doesn't really help, because native code has allocations
to performs anyway (for argument info).


ManagementFactoryHelper.java
L270: The new parameter jvm doesn't seem to be needed.


Fixed.


jmm.h
L316-326: the parameters should be lined up with the first one.
L316-317 was aligned improperly and it's good to fix them
in this batch.


Fixed


HotSpotDiagnostic.c
L44, 51, 76-85: indentation needs fixing.


Fixed.


L158, 172: this should never happen unless the hotspot/jdk is
mismatched. But would it better to throw an exception with
an informative message to help diagnostic in case the mismatch
does happen?


If a mismatch is detected, the implementation now throws an
UnsupportedOperationException(Diagnostic commands are not supported by 
this VM).



jcmd.1 (man page)
Should PerfCounter.print be listed in the man page? Or
treat it like other diagnostic commands that are
implementation-dependent?


PerfCounter is a built-in command, it doesn't appear in the
list of commands returned by the 'help' command. This is why
it is documented in the jcmd man page.

I'll upload new webrevs tomorrow morning.

Thanks,

Fred

--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Request for Review (XXL): 7104647: Adding a diagnostic command framework

[Frederic Parain]

 getDiagnosticCommandInfo() methods return one or several
diagnostic command descriptions using the DiagnosticCommandInfo class.

The two execute() methods allow the user the invoke a diagnostic command
in different ways.

The DiagnosticCommandInfo class is describing a diagnostic command with
the following information:

public class DiagnosticCommandInfo {

public String getName();

public String getDescription();

public String getImpact();

public boolean isEnabled();

public ListDiagnosticCommandArgumentInfo getArgumentsInfo();
}

The getName() method returns the name of the diagnostic command. This
name is the one to use in execute() methods to invoke the diagnostic
command.

The getDescription() method returns a general description of the
diagnostic command.

The getImpact() method returns a description of the intrusiveness of
diagnostic command.

The isEnabled() method returns true if the method is enabled, false if
it is disabled. A disabled method cannot be executed.

The getArgumentsInfo() returns a list of descriptions for the options or
arguments recognized by the diagnostic command. Each option/argument is
described by a DiagnosticCommandArgumentInfo instance:

public class DiagnosticCommandArgumentInfo {

public String getName();

public String getDescription();

public String getType();

public String getDefault();

public boolean isMandatory();

public boolean isOption();

public int getPosition();
}

If the DiagnosticCommandArgumentInfo instance describes an option,
isOption() returns true and getPosition() returns -1. Otherwise, when
the DiagnosticCommandArgumentInfo instance describes an argument,
isOption() returns false and getPosition() returns the expected position
for this argument. The position of an argument is defined relatively to
all arguments passed on the command line, options are not considered
when defining an argument position. The getDefault() method returns the
default value of the argument if a default has been defined, otherwise
it returns null.

3-2 The implementation

The framework has been designed in a way that prevents diagnostic
command developers to worry about the JMX interface. In addition to
the methods described in section 1-2, a diagnostic command developer has
to provide three methods:

int get_num_arguments()

which returns the number of option and arguments supported by the command.

GrowableArrayconst char ** get_argument_name_array()

which provides the name of the arguments supported by the command.

GrowableyArrayDCmdArgumentInfo** get_argument_info_array()

which provides the description of each argument with a DCmdArgumentInfo
instance. DCmdArgumentInfo is a C++ class used by the framework to
generate the sun.com.management.DcmdArgumentInfo instances. This is done
automatically and the diagnostic command developer doesn't need to know
how to create Java objects from the runtime.

4 - The Diagnostic Commands

To avoid name collisions between diagnostic commands coming from
different projects, use of a flat name space should be avoided and
a more structured organization is recommended. The framework itself
doesn't depend on this organization, so it will be a set of rules
defining a convention in the way commands are named.

Diagnostic commands can easily organized in a hierarchical way, so the
template for a command name can be:

domain.[sub-domain.]command

This template can be extended with sub-sub-domains and so on.

A special set of commands without domain will be reserved for the
commands related to the diagnostic framework itself, like the help
command.


Thanks,

Fred

--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

hg: jdk7/tl/jdk: 7036199: Adding a notification to the implementation of GarbageCollectorMXBeans

[frederic . parain]

Changeset: e0c3fd538f1f
Author:fparain
Date:  2011-05-16 17:28 +0200
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/e0c3fd538f1f

7036199: Adding a notification to the implementation of GarbageCollectorMXBeans
Summary: Add a JMX notification to GarbageCollectorMXBeans
Reviewed-by: acorn, mchung

! make/java/management/mapfile-vers
+ src/share/classes/com/sun/management/GarbageCollectionNotificationInfo.java
+ src/share/classes/sun/management/GarbageCollectionNotifInfoCompositeData.java
! src/share/classes/sun/management/GarbageCollectorImpl.java
! src/share/classes/sun/management/GcInfoCompositeData.java
! src/share/classes/sun/management/MemoryManagerImpl.java
! src/share/classes/sun/management/VMManagement.java
! src/share/classes/sun/management/VMManagementImpl.java
! src/share/javavm/export/jmm.h
! src/share/native/sun/management/GarbageCollectorImpl.c
! src/share/native/sun/management/VMManagementImpl.c
+ 
test/com/sun/management/GarbageCollectorMXBean/GarbageCollectionNotificationContentTest.java
+ 
test/com/sun/management/GarbageCollectorMXBean/GarbageCollectionNotificationTest.java

hg: jdk7/tl/jdk: 7031754: javax.management docs need to be updated to replace Java SE 6 occurrences

[frederic . parain]

Changeset: 8daf9e0c9a2e
Author:fparain
Date:  2011-05-13 13:20 +0200
URL:   http://hg.openjdk.java.net/jdk7/tl/jdk/rev/8daf9e0c9a2e

7031754: javax.management docs need to be updated to replace Java SE 6 
occurrences
Summary: Remove references to a specific version of the Java Platform
Reviewed-by: mchung, kamg

! src/share/classes/javax/management/loading/package.html
! src/share/classes/javax/management/modelmbean/package.html
! src/share/classes/javax/management/monitor/package.html
! src/share/classes/javax/management/openmbean/package.html
! src/share/classes/javax/management/package.html
! src/share/classes/javax/management/relation/package.html
! src/share/classes/javax/management/remote/package.html