Re: RFR: JDK-8202319: Fix compilation warnings in Solaris debug builds for DevStudio 12.6

[serguei.spit...@oracle.com]

  
  
Hi Gary,
  
  Probably, the mailing list should include build-dev and
  hotspot-runtime-dev, but not serviceability-dev.
  
  Thanks,
  Serguei
  
  
  On 4/26/18 09:35, Gary Adams wrote:


  
  Getting the sources ready for the next Solaris developer studio toolchain.
Some additional warnings were found in the debug build.

   Issue: https://bugs.openjdk.java.net/browse/JDK-8202319
   Webrev: http://cr.openjdk.java.net/~gadams/8202319/webrev.00/

This update conditionally disables some new error checks, if the
new toolchain is used.



  

Re: RFR: (S): SA: clhsdb 'where -a' throws Assertion Failure with illegal code 236 when CDS is used

[Ioi Lam]

HI Jini,

[1] "_nofast_aload" should be "_nofast_aload_0": aload and aload_0 are 
two different bytecodes.


[2] Only the _nofast_aload_0 bytecode is tested. For completeness, do 
you think it makes sense to add test cases for these other 3 bytecodes?


    _nofast_getfield
    _nofast_putfield
    _nofast_iload


Thanks
- Ioi

On 4/26/18 11:15 AM, Jini George wrote:

Hello all,

Please review the following proposed fix for the issue:

https://bugs.openjdk.java.net/browse/JDK-8174995

Webrev: http://cr.openjdk.java.net/~jgeorge/8174995/webrev.00/

Issue: Clhsdb commands like 'where -a', 'printall' would throw an 
illegal code assertion failure when CDS is used.


Root cause and proposed fix: SA has been unaware of the new bytecodes 
introduced for rewriting at CDS dump time (_nofast* bytecodes). The 
fix is to make SA aware of these new _nofast* bytecodes.


Tests Run and Passed: SA tests on Mach5 (including the tests modified 
to test this fix).


Thank you,
Jini.

RFR: (S): SA: clhsdb 'where -a' throws Assertion Failure with illegal code 236 when CDS is used

[Jini George]

Hello all,

Please review the following proposed fix for the issue:

https://bugs.openjdk.java.net/browse/JDK-8174995

Webrev: http://cr.openjdk.java.net/~jgeorge/8174995/webrev.00/

Issue: Clhsdb commands like 'where -a', 'printall' would throw an 
illegal code assertion failure when CDS is used.


Root cause and proposed fix: SA has been unaware of the new bytecodes 
introduced for rewriting at CDS dump time (_nofast* bytecodes). The fix 
is to make SA aware of these new _nofast* bytecodes.


Tests Run and Passed: SA tests on Mach5 (including the tests modified to 
test this fix).


Thank you,
Jini.

Re: JDK-8171119: Low-Overhead Heap Profiling

[JC Beyler]

Hi all,

A question came up between myself and Serguei about how to protect the
newly allocated oop when the collector does the callback. We decided it
might be best to ask the mailing list for help/guidance/opinion?

 Consider the changes done in this file for example:
http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event.16/src/hotspot/share/gc/shared/collectedHeap.inline.hpp.udiff.html

For example, for obj_allocate, the change becomes:
 oop CollectedHeap::obj_allocate(Klass* klass, int size, TRAPS) {
   debug_only(check_for_valid_allocation_state());
   assert(!Universe::heap()->is_gc_active(), "Allocation during gc not
allowed");
   assert(size >= 0, "int won't convert to size_t");
+
+  HandleMark hm(THREAD);
+  Handle result;
+  {
+JvmtiSampledObjectAllocEventCollector collector;
   HeapWord* obj = common_mem_allocate_init(klass, size, CHECK_NULL);
   post_allocation_setup_obj(klass, obj, size);
   NOT_PRODUCT(Universe::heap()->check_for_bad_heap_word_value(obj, size));
-  return (oop)obj;
+result = Handle(THREAD, (oop) obj);
+  }
+  return result();
 }

The question is: does anyone see an issue here in terms of performance or
something we missed? When I measured it via the Dacapo run, I saw no
performance degradation but I wanted to double check with you all if this
would become a big no no for the final webrev?

Were other benchmarks show that there is no overhead incurred, would this
be ok?

Thanks for your help,
Jc

On Tue, Apr 17, 2018 at 9:51 PM Jeremy Manson 
wrote:

> Great, thanks!
>
> Jeremy
>
> On Tue, Apr 17, 2018 at 2:01 PM serguei.spit...@oracle.com <
> serguei.spit...@oracle.com> wrote:
>
>> Hi Jeremy,
>>
>> We had a private discussion with Jc on this and decided the same.
>> We would like to sample all allocations if possible and on a low level.
>>
>> Thanks,
>> Serguei
>>
>>
>> On 4/17/18 12:38, Jeremy Manson wrote:
>>
>> +1 to sampling anything the thread is allocating.  With the bytecode
>> rewriting based version of this, I had complaints about missing allocations
>> from JNI and APIs like Class.newInstance.  (I don't know how the placement
>> of the collectors would affect this, but it did matter).
>>
>> Jeremy
>>
>> On Thu, Apr 12, 2018 at 2:23 PM JC Beyler  wrote:
>>
>>> Hi Karen,
>>>
>>> I apologize for sending too many webrevs. I try/tend to iterate fast and
>>> move in an iterative fashion. I also try to solve most, if not all, of the
>>> current items that are requested in one go. Perhaps I failed in doing that
>>> recently? I apologize for that.
>>>
>>> So I promise to not send a new webrev in this email or until I'm pretty
>>> sure I got all the current (And any incoming comment/reviews) handled :-)
>>>
>>> For the points you brought up:
>>>a) What are we sampling? In my mind, I'd rather have the sampler be
>>> sampling anything the thread is allocating and not only sample bytecode
>>> allocations. It turns out that I was focusing on that first to get it up.
>>> As I was stuck in figuring out how to get the VM collector and the sampling
>>> collector to co-exist, there was a bit of issues there.
>>>   - That has been solved by now delaying the posting of a sampled
>>> object if a VM collector is present. So now that I've better understood
>>> interactions between collectors and when you could post an event, I'm way
>>> more able to talk about the feasibility and validity of the next item about
>>> bigger objects.
>>>
>>>b) You bring up an excellent point of if we have a multi-array object
>>> or a more complex object (such as a cloned object for example), if the
>>> sampler is tripped on an internal allocation, should we send that smaller
>>> allocation or should we send the bigger object
>>>- Because we get the stacktrace and we only use the oop to figure out
>>> GC information about the liveness of the object in our use-case in the
>>> JVMTI agent, this changes nothing really in practice. I do see value in
>>> sending the multi-array object as a whole to a user.
>>>   - If that is what you think is best, I can work on getting that
>>> supported and the multi-array test would then prove that if part of the
>>> multi-array is sampled, the sampler returns the whole multi-array.
>>>
>>> Hopefully that answers your concern on me sending too many webrevs, to
>>> which I sincerely apologize. Probably a learning curve of different
>>> approaches of reviews. And I hope that my other answers do show the
>>> direction you were hoping to see.
>>>
>>> Thanks again for all your help,
>>> Jc
>>>
>>> On Thu, Apr 12, 2018 at 8:15 AM Karen Kinnear 
>>> wrote:
>>>
 JC,


 On Apr 11, 2018, at 8:17 PM, JC Beyler  wrote:

 Hi Karen,

 I put up a new webrev that is feature complete in my mind in terms of
 implementation. There could be a few tid-bits of optimizations here and
 there but I believe everything is now there in terms of 

RFR: JDK-8202319: Fix compilation warnings in Solaris debug builds for DevStudio 12.6

[Gary Adams]

Getting the sources ready for the next Solaris developer studio toolchain.
Some additional warnings were found in the debug build.

   Issue: https://bugs.openjdk.java.net/browse/JDK-8202319
   Webrev: http://cr.openjdk.java.net/~gadams/8202319/webrev.00/

This update conditionally disables some new error checks, if the
new toolchain is used.

Re: RFR: JDK-8187498: Add a -Xmanagement flag as syntactic sugar for -Dcom.sun.management.jmxremote.* properties

[mandy chung]

On 4/23/18 1:20 PM, Harsha Wardhana B wrote:

Hi All,

After internal discussions, many of the concerns below were addressed 
and final spec is published at,


https://bugs.openjdk.java.net/browse/JDK-8199584

Below is the implementation of the above spec.

http://cr.openjdk.java.net/~hb/8187498/webrev.05/


src/java.base/share/classes/sun/launcher/resources/launcher.properties

112 \ --start-management-agent option=value[:option=value:]\n\

option and value should be  and  to represent user-supplied 
name/value.

 113 \ start the default management agent with semi-colon separated\n\
 114 \ list of options. Multiple values for an option should be separated\n\
115 \ by comma. See the java tool guide for a list of options for\n\
116 \ the default management agent.\n\ typo: "semi-colon separated list" 
should be colon-separated list "See the java tool guide" - should be 
"See the Java Monitoring and Management Guide for details" 
src/jdk.management.agent/share/classes/jdk/internal/agent/Agent.java I 
think the change from single class import to import java.util.* and 
java.io.* may not be done intentionally (I guess it might be done by 
IDE). I prefer to keep the orginal single class import.
665 if (args.equals("--start-management-agent") || 
args.equals("--start-management-agent=")) { 666 // Options are mandatory 
667 error(INVALID_OPTION, args); 668 }
709 Optional argStr = vmArguments.stream() 710 .filter(a -> 
a.startsWith("--start-management-agent")) 711 .reduce((a, b) -> b); The 
only form passed to the VM is --start-management-agent= since VM 
option does not take white-space separated argument. I think line 666 
and 710 should check for "--start-management-agent=" only. 676 String 
minusDflag = managementFlags.get(name); minusDflag can be renamed to 
"propertyName" which is clearer. 714 props.forEach((a, b) -> { 715 
String oldVaue = System.getProperty((String) a); 716 if (oldVaue != null 
&& !oldVaue.isEmpty()) { 717 error(INVALID_OPTION, "management options 
can be set via -D flags or via --start-management-agent but not both"); 
718 } 719 System.setProperty((String) a, (String) b); 720 }); 721 }  714 
props.forEach((a, b) -> {

715 String oldVaue = System.getProperty((String) a);
716 if (oldVaue != null && !oldVaue.isEmpty()) {
717 error(INVALID_OPTION, "management options can be set via -D flags or 
via --start-management-agent but not both");

718 }
I think checking if -D is set should be done for each constant defined 
in ConnectorBootstrap.PropertyNames class instead of each key in props 
since it could set -Dcom.sun.management.jmxremote.port it didn't set 
port in --start-management-agent. Do you have such test case covered?   719 System.setProperty((String) a, (String) b);I don't expect 
--start-management-agent will set the system properties like if 
-Dcom.sun.management.config.file=config.file which will not set 
additional system properties, right? It's also not specified in CSR. I see the existing code calling System.getProperty is not modified.  I think

that may need to be updated too?

In addition, as specified in CSR, e.e.g

--start-management-agent 
port=1234:configFile=management.properties:ssl=true:authenticate=false

The value specified via the command line takes precedence over the value
specified in the config file.  port=1234 and ssl=true will take precedence
even if those properties are set in management.properties.

It seems that this is not covered (or I missed it from the webrev).

ConnectorBootstrap.PropertyNames  defines the property names.  It may be
better to extend this class to take the short name and value validator
into account (replace the managementMap and validatorMap).  You may
want to refactor it out as an outer class if needed.

I will review the test when the webrev is updated (in case the test will
need update).

Mandy

Re: RFR: JDK-8174994: SA: clhsdb printmdo throws WrongTypeException when attached to a process with CDS

[Jini George]

Thank you, Yasumasa.

- Jini.

On 4/26/2018 11:41 AM, Yasumasa Suenaga wrote:

Hi Jini,

I have no further comment.


Yasumasa



2018-04-26 13:21 GMT+09:00 Jini George :

Thank you, Yasumasa. I hope to implement the consolidation with
TestSAServer.java (and have the SA core file debug testing template done) as
a part of a separate enhancement:
https://bugs.openjdk.java.net/browse/JDK-8202297

Let me know if this is not OK with you.

Thanks,
Jini.


On 4/25/2018 6:14 PM, Yasumasa Suenaga wrote:


Hi Jini,



2018-04-18 15:05 GMT+09:00 Jini George :



:


I plan to file an enhancement request to address this issue (wrt
systemd-coredump) separately since this would apply to other
coredump
generating test cases also like:
test/hotspot/jtreg/compiler/ciReplay/TestSAServer.java.




I guessed the tests for coredumps will be handled in another issue (with
TestSAServer.java).
Is it okay to implement coredump test in this changeset?

IMHO, it looks to implement as new test basis (e.g. LingeredAppForCoredump
- ulimit check, set, etc...).


Thanks,

Yasumasa



On 2018/04/25 12:26, Jini George wrote:


Thank you very much, David for looking into this. I have incorporated all
the comments and the revised webrev is at:

http://cr.openjdk.java.net/~jgeorge/8174994/webrev.02/index.html

Thanks,
Jini.

On 4/24/2018 3:29 PM, David Holmes wrote:


Hi Jini,

Not a full review as I'm not familiar enough with this code.

My main comment, again, relates to
test/hotspot/jtreg/serviceability/sa/ClhsdbCDSCore.java and that it should
not fail (throw Error) if there is no core file generated etc. In that case
the test should be skipped with a clear message (as elsewhere). Otherwise
this test will fail locally for me every time I run the serviceability
tests!

I also have a few style issues.

Don't compare boolean functions with true or false i.e.

if (isX() == true) -> if (isX())
if (isX() == false) -> if (!isX())

this occurs in most of the Java files. It is especially noticeable when
you mix styles ie:

+ if (VM.getVM().isSharingEnabled()) {  <= implicit check of true
+   // Check if the value falls in the _md_region
+   FileMapInfo cdsFileMapInfo = VM.getVM().getFileMapInfo();
+   if (cdsFileMapInfo.inCopiedVtableSpace(loc1) == true) { <=
explicit check
+  return cdsFileMapInfo.getTypeForVptrAddress(loc1);
+   }
+ }

---


src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/memory/FileMapInfo.java

   139 vTableTypeMap.put
   140 (copiedVtableAddress.addOffsetTo(VM.getVM().getAddressSize()),
metadataTypeArray[i]);
   141 // The '+ 1' below is to skip the entry containing the
size of this metadata's vtable.
   142 copiedVtableAddress =
   143   copiedVtableAddress.addOffsetTo((metadataVTableSize + 1)
* VM.getVM().getAddressSize());

If you store VM.getVM().getAddressSize() in a local you only need call
it once, and the other lines of code will be shorter.

On line 139/140 keep the opening parenthesis with the method name ie:

  vTableTypeMap.put(

but with shorter lines you should be able to reformat that more cleanly
anyway.


   146   } // FileMapHeader
   147 } // FileMapInfo

We generally  don't comment the end of blocks.

---

test/hotspot/jtreg/serviceability/sa/ClhsdbCDSCore.java

   96 } catch (Throwable t) {
97 throw new Error("Can't execute the java cds
process.");
98 }

Set 't' as the cause of the new Error so we can see why it failed.

Thanks,
David

On 24/04/2018 7:03 PM, Jini George wrote:


Hello!

The webrev including the check for the "|" at the beginning of the
core_pattern file is at:

http://cr.openjdk.java.net/~jgeorge/8174994/webrev.01/

This webrev also includes a fix for a latent bug on MacOSX, where
corefile debugging was failing due to SA trying to read in the incorrect
mangled symbol name for "Arguments::SharedArchivePath". Clang seems to have
prefixed an extra '_' to change the mangled name from
'_ZN9Arguments17SharedArchivePathE' to '__ZN9Arguments17SharedArchivePathE'
for MachO files. This fix for this is in
src/jdk.hotspot.agent/macosx/native/libsaproc/ps_core.c.

The difference between the earlier patch and this one can be seen at:

http://cr.openjdk.java.net/~jgeorge/8174994/differential.patch

Thank you,
Jini.


On 4/18/2018 10:37 PM, Jini George wrote:


I agree with the need of testing as much as we can. I could do
something on the lines of how other debuggers like LLDB test: if we can't
find the core file location, check for "|" at the beginning of a line in the
/proc/sys/kernel/core_pattern file -- and fail with a message stating that
the system is using a crash reporting tool.

Thank you,
Jini.

On 4/18/2018 12:40 PM, David Holmes wrote:


My 2c ...

We have to have tests that can test core file attaching capability -
else we don't know it works. So we have to try and generate a core file.

But, we have to expect that in 

Re: RFR: JDK-8174994: SA: clhsdb printmdo throws WrongTypeException when attached to a process with CDS

[Yasumasa Suenaga]

Hi Jini,

I have no further comment.


Yasumasa



2018-04-26 13:21 GMT+09:00 Jini George :
> Thank you, Yasumasa. I hope to implement the consolidation with
> TestSAServer.java (and have the SA core file debug testing template done) as
> a part of a separate enhancement:
> https://bugs.openjdk.java.net/browse/JDK-8202297
>
> Let me know if this is not OK with you.
>
> Thanks,
> Jini.
>
>
> On 4/25/2018 6:14 PM, Yasumasa Suenaga wrote:
>>
>> Hi Jini,
>>
>>
 2018-04-18 15:05 GMT+09:00 Jini George :
>>
>>
>>:
>>
> I plan to file an enhancement request to address this issue (wrt
> systemd-coredump) separately since this would apply to other
> coredump
> generating test cases also like:
> test/hotspot/jtreg/compiler/ciReplay/TestSAServer.java.
>>
>>
>>
>> I guessed the tests for coredumps will be handled in another issue (with
>> TestSAServer.java).
>> Is it okay to implement coredump test in this changeset?
>>
>> IMHO, it looks to implement as new test basis (e.g. LingeredAppForCoredump
>> - ulimit check, set, etc...).
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>>
>> On 2018/04/25 12:26, Jini George wrote:
>>>
>>> Thank you very much, David for looking into this. I have incorporated all
>>> the comments and the revised webrev is at:
>>>
>>> http://cr.openjdk.java.net/~jgeorge/8174994/webrev.02/index.html
>>>
>>> Thanks,
>>> Jini.
>>>
>>> On 4/24/2018 3:29 PM, David Holmes wrote:

 Hi Jini,

 Not a full review as I'm not familiar enough with this code.

 My main comment, again, relates to
 test/hotspot/jtreg/serviceability/sa/ClhsdbCDSCore.java and that it should
 not fail (throw Error) if there is no core file generated etc. In that case
 the test should be skipped with a clear message (as elsewhere). Otherwise
 this test will fail locally for me every time I run the serviceability
 tests!

 I also have a few style issues.

 Don't compare boolean functions with true or false i.e.

 if (isX() == true) -> if (isX())
 if (isX() == false) -> if (!isX())

 this occurs in most of the Java files. It is especially noticeable when
 you mix styles ie:

 + if (VM.getVM().isSharingEnabled()) {  <= implicit check of true
 +   // Check if the value falls in the _md_region
 +   FileMapInfo cdsFileMapInfo = VM.getVM().getFileMapInfo();
 +   if (cdsFileMapInfo.inCopiedVtableSpace(loc1) == true) { <=
 explicit check
 +  return cdsFileMapInfo.getTypeForVptrAddress(loc1);
 +   }
 + }

 ---


 src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/memory/FileMapInfo.java

   139 vTableTypeMap.put
   140 (copiedVtableAddress.addOffsetTo(VM.getVM().getAddressSize()),
 metadataTypeArray[i]);
   141 // The '+ 1' below is to skip the entry containing the
 size of this metadata's vtable.
   142 copiedVtableAddress =
   143   copiedVtableAddress.addOffsetTo((metadataVTableSize + 1)
 * VM.getVM().getAddressSize());

 If you store VM.getVM().getAddressSize() in a local you only need call
 it once, and the other lines of code will be shorter.

 On line 139/140 keep the opening parenthesis with the method name ie:

  vTableTypeMap.put(

 but with shorter lines you should be able to reformat that more cleanly
 anyway.


   146   } // FileMapHeader
   147 } // FileMapInfo

 We generally  don't comment the end of blocks.

 ---

 test/hotspot/jtreg/serviceability/sa/ClhsdbCDSCore.java

   96 } catch (Throwable t) {
97 throw new Error("Can't execute the java cds
 process.");
98 }

 Set 't' as the cause of the new Error so we can see why it failed.

 Thanks,
 David

 On 24/04/2018 7:03 PM, Jini George wrote:
>
> Hello!
>
> The webrev including the check for the "|" at the beginning of the
> core_pattern file is at:
>
> http://cr.openjdk.java.net/~jgeorge/8174994/webrev.01/
>
> This webrev also includes a fix for a latent bug on MacOSX, where
> corefile debugging was failing due to SA trying to read in the incorrect
> mangled symbol name for "Arguments::SharedArchivePath". Clang seems to 
> have
> prefixed an extra '_' to change the mangled name from
> '_ZN9Arguments17SharedArchivePathE' to 
> '__ZN9Arguments17SharedArchivePathE'
> for MachO files. This fix for this is in
> src/jdk.hotspot.agent/macosx/native/libsaproc/ps_core.c.
>
> The difference between the earlier patch and this one can be seen at:
>
> http://cr.openjdk.java.net/~jgeorge/8174994/differential.patch
>
> Thank you,
> Jini.
>
>
> On 4/18/2018 10:37 PM, Jini George wrote: