Re: RFR(XS): 8215042: Move serviceability/sa tests from tier1 to tier3.

[JC Beyler]

Hi Leonid,

I cannot comment on whether it is a good idea to put the tests in tier3 but
the webrev does look good to achieve that :) So LGTM as far as it seems to
do what you intended :)

Thanks,
Jc

On Fri, Dec 7, 2018 at 9:53 PM Leonid Mesnik 
wrote:

> Hi
>
> Could you please review following fix which moves SA tests from tier1 to
> tier3. There are some bugs which cause intermittent failures of any test.
> SA tests fail intermittently are not stable enough for tier1.
> However failures are not very frequent. Also I don't think that putting
> all test in Problemlist.txt is very good idea because it left SA without
> any testing at all.
> So now all SA tests which are included in  hotspot_tier3_runtime group.
>
> webrev: http://cr.openjdk.java.net/~lmesnik/8215042/webrev.00/
> bug: https://bugs.openjdk.java.net/browse/JDK-8215042
>
> Leonid
>


-- 

Thanks,
Jc

RFR(XS): 8215042: Move serviceability/sa tests from tier1 to tier3.

[Leonid Mesnik]

Hi

Could you please review following fix which moves SA tests from tier1 to tier3. 
There are some bugs which cause intermittent failures of any test. SA tests 
fail intermittently are not stable enough for tier1.  
However failures are not very frequent. Also I don't think that putting all 
test in Problemlist.txt is very good idea because it left SA without any 
testing at all. 
So now all SA tests which are included in  hotspot_tier3_runtime group.

webrev: http://cr.openjdk.java.net/~lmesnik/8215042/webrev.00/ 

bug: https://bugs.openjdk.java.net/browse/JDK-8215042 


Leonid

Re: RFR (round 5), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector

[Roland Westrelin]

> http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/05/

C2 changes look good to me.

Roland.

Re: RFR (T) 8215034: Remove old HOTSWAP conditionals

[serguei . spitsyn]

Hi Coleen,

+1

Thanks,
Serguei

On 12/7/18 2:06 PM, Jiangli Zhou wrote:

Looks good and trivial.

Thanks,

Jiangli


On 12/7/18 1:57 PM, coleen.phillim...@oracle.com wrote:

I was in the area and found these few remaining conditionals.

Tested with tier1 on Oracle platforms.

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

Thanks,
Coleen

Re: RFR (T) 8215034: Remove old HOTSWAP conditionals

[coleen . phillimore]

Thanks Dan, Jiangli, and Serguei!
Coleen

On 12/7/18 5:50 PM, serguei.spit...@oracle.com wrote:

Hi Coleen,

+1

Thanks,
Serguei

On 12/7/18 2:06 PM, Jiangli Zhou wrote:

Looks good and trivial.

Thanks,

Jiangli


On 12/7/18 1:57 PM, coleen.phillim...@oracle.com wrote:

I was in the area and found these few remaining conditionals.

Tested with tier1 on Oracle platforms.

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

Thanks,
Coleen

Re: RFR (T) 8215034: Remove old HOTSWAP conditionals

[Jiangli Zhou]

Looks good and trivial.

Thanks,

Jiangli


On 12/7/18 1:57 PM, coleen.phillim...@oracle.com wrote:

I was in the area and found these few remaining conditionals.

Tested with tier1 on Oracle platforms.

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

Thanks,
Coleen

Re: RFR (T) 8215034: Remove old HOTSWAP conditionals

[Daniel D. Daugherty]

On 12/7/18 4:57 PM, coleen.phillim...@oracle.com wrote:

I was in the area and found these few remaining conditionals.

Tested with tier1 on Oracle platforms.

open webrev at http://cr.openjdk.java.net/~coleenp/8215034.01/webrev


src/hotspot/share/code/codeCache.cpp
    No comments.

src/hotspot/share/code/codeCache.hpp
    No comments.

src/hotspot/share/interpreter/templateInterpreter.hpp
    No comments.

src/hotspot/share/utilities/globalDefinitions.hpp
    No comments.

Thumbs up! Agree this is 'trivial'.

Dan



bug link https://bugs.openjdk.java.net/browse/JDK-8215034

Thanks,
Coleen

RFR (T) 8215034: Remove old HOTSWAP conditionals

[coleen . phillimore]

I was in the area and found these few remaining conditionals.

Tested with tier1 on Oracle platforms.

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

Thanks,
Coleen

Re: [PATCH] JDK-8214122 Prevent name mangling of jdwpTransport_OnLoad in Windows 32-bit DLL name decoration

[Alexey Ivanov]

Hi Ali,

On 06/12/2018 22:49, Ali İnce wrote:

Hi Magnus, Alexey,

I believe we won’t be able to get further opinions from 
serviceability-dev.


Unfortunately, no one has replied as of now.
Have you found any issues with jdwpTransport_OnLoad after removing JNICALL?

Andrew Luo suggested using a similar mechanism as is used for jvm.dll 
by using symbol name files mapped by platform (files are under 
make/hotspot/symbols and interestingly windows is almost the only 
platform for which a symbol file doesn’t exist).


Andrew says the .def files are auto-generated for Windows. There's a set 
of shared functions.
JVM cannot change the calling convention, jvm.dll is the public 
interface to JVM.


Another issue, again opened against AdoptOpenJDK 32-bit builds is 
somehow related with the same problem - but this time it is jimage.dll 
depending on zip.dll expecting the ZIP_InflateFully function to be 
decorated with JNICALL - whereas it was removed earlier from the 
implementation and the runtime image just fails with access violation 
just because jimage source code still declared it with JNICALL 
(https://github.com/AdoptOpenJDK/openjdk-build/issues/763).


The usage of ZIP_InflateFully from zip.dll by jimage.dll was overlooked 
during work on https://bugs.openjdk.java.net/browse/JDK-8201226.


I can take care of it.
(I could not build 32 bit Windows. It seem to have overcome the problem 
by adding --disable-warnings-as-errors option to configure.)


However, the report says the resulting image crashes in 64 bit windows 
too which shouldn't be affected by JNICALL mismatch.


I’ve also searched for GetProcAddress calls across the source code - 
and I think it’s important to work on the consistency of JNICALL 
usages across the whole source code.


There are many places where libraries are loaded dynamically or a 
function may be unavailable on older versions of the platform.
Have you identified any other place where functions from JDK DLLs are 
looked up by names?


Another noteworthy thing I’ve noticed is there are still JNICALL 
modifiers for example in 
https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/master/src/jdk.crypto.cryptoki/windows/native/libj2pkcs11/j2secmod_md.c#L49 - 
which I guess will also be reported as broken since these functions 
are again name decorated.


This is a JNI method. It should use JNICALL modifier. If it wasn't 
found, the class sun.security.pkcs11.Secmod would fail to load. I guess 
JVM handles name mangling somehow for native method implementation.


Such functions were never explicitly exported using mapfiles or /export 
options on Windows, at least in the client area. For Linux and Solaris, 
adding or removing a native method required editing mapfiles.


If we’re still determined to remove JNICALL, what about just removing 
__stdcall from JNICALL declaration at 
https://github.com/AdoptOpenJDK/openjdk-jdk11u/blob/master/src/java.base/windows/native/include/jni_md.h#L31 ? Wouldn’t 
that be a more consistent move?


We can't do that.
Implementation of Java native methods must use __stdcall calling convention.



Regards,

Ali

On 22 Nov 2018, at 17:29, Magnus Ihse Bursie 
> wrote:


I think we are in complete agreement. What's missing is some expert 
opinion from serviceability-dev if there is any problem with changing 
the signature. I'd preferably have that.


If no one knows, I'd say, change it, and see if we still pass the 
relevant tests, and if so, ship it.


/Magnus

22 nov. 2018 kl. 18:04 skrev Alexey Ivanov >:




On 21/11/2018 12:16, Magnus Ihse Bursie wrote:


On 2018-11-20 16:49, Alexey Ivanov wrote:


Hi Ali, Magnus,

I absolutely agree this change has to be reviewed by someone from 
serviceability.


There are several options:

1. Add -export:jdwpTransport_OnLoad to LDFLAGS for Windows
http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023935.html
so it partially reverts the changes from
https://bugs.openjdk.java.net/browse/JDK-8200178

2. Remove JNICALL (__stdcall) modifier, eventually changing the 
calling convention to __cdecl.

http://mail.openjdk.java.net/pipermail/build-dev/2018-November/023969.html

3. Use inline /export option via #pragma:
#pragma comment(linker, "/export:jdwpTransport_OnLoad= 
_jdwpTransport_OnLoad@16")

as referenced in
https://docs.microsoft.com/en-ie/cpp/cpp/dllexport-dllimport?view=vs-2017
https://docs.microsoft.com/en-ie/cpp/build/reference/exports?view=vs-2017

The third options still needs to be tested to make sure it does 
not break other platforms builds.
I'm not fond of either solution 1 or 3. I really think the 
signature should be made correctly at the point of definition in 
the source code.


That is why I proposed removing JNICALL (__stdcall) from the 
function declaration as we had done for libzip, libjimage [1] and 
mlib_image [2].


Microsoft recommends using __stdcall for DLLs, at the same time it 
decorates the function name making 

Re: RFR: (S): JDK-8200613: SA: jstack throws UnmappedAddressException with a CDS core file

[Jini George]

I have the revised webrev here:

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

The extra changes here are to:

* Introduce a new option DumpPrivateMappingsInCore to control the 
dumping of the file backed private regions into the corefile.


* Close the modules file before dumping core in os::abort(). Currently, 
there is a small bug (https://bugs.openjdk.java.net/browse/JDK-8215026) 
which prevents the closure of the image file in unmapping the entire file.


I plan to take up the unmapping of NIO MapMode.PRIVATE files as a 
separate task (https://bugs.openjdk.java.net/browse/JDK-8215027) since 
this seems a bit involved.


Thanks a bunch,
Jini.

On 11/12/2018 10:26 AM, Jini George wrote:

Thank you very much, Chris, Kevin and Ioi for your comments!

I will send another webrev with this change enabled under an opt-out 
flag, as you suggest, and would look at unmapping the JDK modules file 
and if possible, the NIO mapped files too in the signal handler.


Thanks a bunch,
Jini.

On 11/9/2018 11:53 PM, Ioi Lam wrote:

Hi Jini,

Thanks for investigating the size expansion issue.

I agree that the size increase is worth it. Even when not using SA, if 
we open the core file inside GDB, we cannot read certain sections in 
the CDS archive (such as the RO section and strings sections). That 
would make debugging difficult. So I am in favor of this change.


For the JDK modules file, maybe we can unmap it in the signal handler, 
before going ahead with the core dump? I think it's hardly needed for 
debugging purposes. (Perhaps we can also do the same for the NIO 
mapped files?)


A opt-flag as suggested by Kevin is a good idea.

Thanks

- Ioi

On 11/9/18 3:29 AM, Kevin Walls wrote:

Hi Jini,

Looks good to me.  It might be a significant increase in size of 
_some_ core files, but so many core files we see are much larger, in 
gigabytes++ of course, so the CDS data size should not be such a 
significant increase on (I think) most files.


The flexibiity of always having the CDS data there is very 
significant.  A core file should ideally be usable, without 
additionally requiring the CDS archive from the machine.  That 
additional human round-trip upload request on every transmitted core 
that needs investigating, seems like a less efficient route...).


Is there an opt-out?  It's conditional on UseSharedSpaces but could 
there be a flag to disable, in case we see crashes with gigabytes of 
private mappings that we really don't want to retain (the user would 
have to know to set a flag, to disable the new coredump filter ahead 
of time).


Thanks!
Kevin


On 29/10/2018 06:02, Jini George wrote:
Thank you very much, Ioi, for looking into this, and the 
clarification offline. My bad, I had missed the earlier mail from 
you. :-( My responses below.


Yes, I had tested this on MacOS. The issue does not exist on MacOS 
since the file backed private mmap()-ed regions get dumped into the 
MacOS corefiles by default.


The corefile sizes on Linux do increase due to this change. And the 
increase would also include any file mapped using NIO with 
MapMode.PRIVATE. The typical corefile size increase with this change 
would include the following components at a high level:


* Any NIO file mapping with MapMode.PRIVATE.
* Any file mmap()-ed by any native library with MAP_PRIVATE.
* The read only CDS regions (ro and od): Of the order of a few MB.
* The shared strings CDS region. (typically less than 1 MB).
* 2 MB per native shared library (regions with ---p permissions 
mapped by the dynamic linker for better alignment and for keeping 
libraries efficiently shareable).

* The JDK 'modules' file. (About 140 MB).

So, without including any NIO mapping, I typically see around 
250-300 MB increase in the corefile sizes. I agree that the size 
increase could be a cause for concern, but for FWIW, these privately 
mapped files get dumped into the corefile for MacOS too. And the 
corefile sizes for the same program on MacOS are way larger (of the 
order of a few GB as against about 300 MB on Linux (without the 
change)).


The advantage of fixing this by modifying the coredump_filter v/s 
doing it in SA (by reading in more sections of the shared archive 
file) is that this would benefit other debuggers like gdb also. (And 
reduces the dependence on having the shared archive file being 
available at the time of debugging). If folks still think this is a 
cause for concern, I could make modifications to fix this by reading 
in the regions from the shared archive file in the SA code. I also 
wonder if it is worth taking a relook at the mapping types of the 
various CDS regions also.


Thank you,
Jini.

On 10/22/2018 10:27 AM, Ioi Lam wrote:

Hi Jini,

Did you see my earlier reply? I might have sent it out during the 
mail server outage days :-(


http://openjdk.5641.n7.nabble.com/RFR-S-JDK-8200613-SA-jstack-throws-UnmappedAddressException-with-a-CDS-core-file-td352681.html#a353026 



Here was my reply again:


Hi 

Re: JDK-8171311 Current state

[Erik Gahlin]

The reason to put this into the JDK is to standardize the protocol.

If you want to build a client today, you must build one for every 
adapter because they have different ways to represent URLs etc.


The JDK is in unique position to set the standard, since the 
implementation comes by default.


Erik


Thanks,

I just found that JEP searching for an simple way to attach to a non 
application server VM avoiding the hassle for setting up Firewall 
Rules for RMI and that JEP was the first in the list followed by the 
Jolokia that seems not jet ready for Java 11...


I will look into the Jolokia library and will try to find out, what 
the exact issues with Java 11 are.


Besides that it would really make sense to see if there would be a 
better way for starting the JMX services as Alan pointed out.


-Patrick




On 2018-12-07 10:59, Raghavan Puranam wrote:

My apologies Patrick...I should have added the comment first before
closing.  I have added it now.

Regards,
Raga

-Original Message-
From: Alan Bateman
Sent: Friday, December 7, 2018 2:52 PM
To: Patrick Reinhart
Cc: serviceability-dev@openjdk.java.net
Subject: Re: JDK-8171311 Current state

On 07/12/2018 08:36, Patrick Reinhart wrote:

It's a bit disturbing that just at the time of my question this JEP
has been closed (without any further comment why)

I suspect your inquiry prompted Raghavan to close it as there isn't (to
my knowledge anyway) anyone actively working on it. I agree a comment is
needed when closing issues.



I think that it still would be worth while looking into supporting
a REST based implementation in favour of the existing RMI based
solution just by the fact of the troubles just one can have with
firewalls.

Right, and I think there is some interest. In addition to the REST
adapters that you found then I think some of the app servers have
support too. The big question for features like this is whether it is
something that the JDK  has to include or not (the batteries included
vs. batteries available discussion).  If you look at Harsha's prototype
(linked from the JEP) then you'll you see it can be mostly developed in
its own project, the only JDK piece is integrating it with the JMX agent
and existing -Dcom.sun.management options for starting the JMX agent. I
think this is an area that could be improved to make it easier to deploy
JMX adapters that aren't in the JDK.

-Alan

Re: RFR: JDK-8210106: sun/tools/jps/TestJps.java timed out

[Daniel D. Daugherty]

On 12/7/18 7:58 AM, Gary Adams wrote:

On 12/6/18, 7:52 PM, David Holmes wrote:

Hi Gary,

On 6/12/2018 11:27 pm, Gary Adams wrote:
On a local linux-x64-debug build this test consistently runs in less 
than 40 seconds.
On the mach5 testing machines there was a large fluctuation in the 
time to complete this test.


Since the test runs jps with different combinations of arguments, 
the total
test time is dependent on the number of processes running java 
programs.

Since the mach5 test infrastructure runs java programs I have seen a 3X
in the amount of output the test produces compared to local test
runs.

I've run the test several hundred times through mach5 on the slower 
platforms
and then examined the test logs using a 3X setting from the default 
120 second
jtreg timeout. The slowest reported elapse time from the test logs 
showed

280 seconds to complete.

To improve the reliability of the test to complete, I'd like to 
increase the

timeout to 360 seconds.

   Issue: https://bugs.openjdk.java.net/browse/JDK-8210106

Proposed fix:

diff --git a/test/jdk/sun/tools/jps/TestJps.java 
b/test/jdk/sun/tools/jps/TestJps.java

--- a/test/jdk/sun/tools/jps/TestJps.java
+++ b/test/jdk/sun/tools/jps/TestJps.java
@@ -27,7 +27,7 @@
   * @modules jdk.jartool/sun.tools.jar
   * @build LingeredAppForJps
   * @build LingeredApp
- * @run main/othervm TestJps
+ * @run main/othervm/timeout=360 TestJps
   */


Doesn't that then get scaled by the timeout factor resulting in a 
much longer timeout than the 360 seconds you intended?


For other timeout adjustments the needed time has been divided by the 
timeout factor to get the actual intended timeout.


This bug was filed fairly recently in Aug 2018.
At that time the timeout and timeout factor were not sufficient
to avoid the test failing.

The mach5 timeout factors were adjusted recently, so this test may
no longer be an issue.

If that is true, then we could simply close this bug as "cannot 
reproduce".

An argument could be made that the change in timeout factor may be
responsible for fixing a lot more of the intermittent bugs and that they
should be closed in a similar manner.

Historically, we could say this particular bug should have had timeouts
reassessed when the infrastructure switched from jprt to mach5 testing
where there were more visible Java processes running.

Using a higher explicit timeout will not make the test run any longer
than it needs. It will simply allow the test to not be terminated sooner
in a hung test scenario.

What is your preference for this particular issue:
   - increase the explicit timeout
   - close as cannot reproduce attributed to the timeout factor 
adjustments


What would you recommend going forward for other similar issues:
   - determine a new explicit timeout
   - close if no timeout failures have been observed since the timeout 
factor

  was raised


General guidance that I've been giving folks is that you bump the
default timeout when you see timeouts with 'release' bits with
a timeout factor of '1'. The default timeout factor in Mach5 is 4
so we run both 'release' and 'fastdebug' bits with that timeout
factor. That means our total timeout value for 'release' bits is
probably a little long and our total timeout value for 'fastdebug'
bits is probably just right for a concurrency factor of 12.

On my personal servers I use the following timeout factors:

    release)
    TIMEOUT_FACTOR="4"
    ;;
    fastdebug)
    TIMEOUT_FACTOR="6"
    ;;
    slowdebug)
    TIMEOUT_FACTOR="12"
    ;;

However, I run with a concurrency factor of 16. I rarely see
timeout failures.

All that is well and good, but a 'jps' test is a different
beast since it is affected by factors outside of most tests.
Since the number of java processes running on the system can
affect the test, I'm okay with using a default timeout of '360'
for this test as a guard against increases in load or...

Gary saw a longest time value of 280 seconds in his testing with
fastdebug bits. The default timeout value is 120 seconds with a
default timeout factor of 4 in Mach5 gives a total timeout of
480 seconds (8 minutes). With Gary's new default timeout of
360 seconds, we'll have a total timeout value of 1440 seconds
(24 minutes).

Before the recent changes to Mach5, the default timeout factor
was 10 so we had a total timeout value of 1200 seconds (20
minutes). Gary's change only bumps the total timeout value
by 4 minutes from what we had back in August... when this bug
was filed... unfortunately, the log for that sighting is gone
so all we know is that the test timed out after 20 minutes

> Timeout refired 1200 times

We know that JTREG timeout handling kicks in at timeout expiration,
but without the log we don't know how much longer than 20 minutes
the test ran before JTREG killed it. 24 minutes might do it, but...

Dan







Cheers,
David

Re: JDK-8171311 Current state

[Patrick Reinhart]

Thanks,

I just found that JEP searching for an simple way to attach to a non 
application server VM avoiding the hassle for setting up Firewall Rules 
for RMI and that JEP was the first in the list followed by the Jolokia 
that seems not jet ready for Java 11...


I will look into the Jolokia library and will try to find out, what the 
exact issues with Java 11 are.


Besides that it would really make sense to see if there would be a 
better way for starting the JMX services as Alan pointed out.


-Patrick




On 2018-12-07 10:59, Raghavan Puranam wrote:

My apologies Patrick...I should have added the comment first before
closing.  I have added it now.

Regards,
Raga

-Original Message-
From: Alan Bateman
Sent: Friday, December 7, 2018 2:52 PM
To: Patrick Reinhart
Cc: serviceability-dev@openjdk.java.net
Subject: Re: JDK-8171311 Current state

On 07/12/2018 08:36, Patrick Reinhart wrote:

It's a bit disturbing that just at the time of my question this JEP
has been closed (without any further comment why)

I suspect your inquiry prompted Raghavan to close it as there isn't (to
my knowledge anyway) anyone actively working on it. I agree a comment 
is

needed when closing issues.



I think that it still would be worth while looking into supporting
a REST based implementation in favour of the existing RMI based
solution just by the fact of the troubles just one can have with
firewalls.

Right, and I think there is some interest. In addition to the REST
adapters that you found then I think some of the app servers have
support too. The big question for features like this is whether it is
something that the JDK  has to include or not (the batteries included
vs. batteries available discussion).  If you look at Harsha's prototype
(linked from the JEP) then you'll you see it can be mostly developed in
its own project, the only JDK piece is integrating it with the JMX 
agent

and existing -Dcom.sun.management options for starting the JMX agent. I
think this is an area that could be improved to make it easier to 
deploy

JMX adapters that aren't in the JDK.

-Alan

RFR (round 6), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector

[Roman Kennke]

Here comes round 6, possibly/hopefully the last round of webrevs for
upstreaming Shenandoah:

http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/06/

It incorporates:
- renames vm_operations_shenandoah* to shenandoahVMOperations*, as
suggested by Coleen
- reshuffles gcCause in shared-gc SA as suggested by Per
- print number of threads in SA, as suggested by Jini
- fixed a problem in webrev generation that did not track move of
CriticalNativeStress.java and CriticalNativeArgs.java

The CSR has been approved, the JEP moved to target jdk12, and I got
positive reviews for all parts. I intend to push this early next week,
unless somebody stops me. If Zhengyu's and Thomas' TaskQueue change goes
in by then, I'll incorporate it.

Thanks everybody for reviewing and reviewing and reviewing again ;-)

Cheers,
Roman

> Round 5 of Shenandoah review includes:
> - A fix for the @requires tag in TestFullGCCountTest.java. It should be
> correct now. We believe the CMS @requires was also not quite right and
> fixed it the same.
> 
> It reads now: Don't run this test if:
>  - Actual GC set by harness is CMS *and* ExplicitGCInvokesConcurrent is
> true, as set by harness
>  - Actual GC set by harness is Shenandoah *and*
> ExplicitGCInvokesConcurrent is not set false by harness (it's true by
> default in Shenandoah, so this needs to be double-inverteed).
> 
> The @requires for CMS was wrong before (we think), because it would also
> filter defaultGC + ExplicitGCInvokesConcurrent.
> 
> - Sorting of macros was fixed, as was pointed out by Per
> - Some stuff was added to SA, as suggested by Jini
> - Rebased on most current jdk/jdk code
> 
> Webrevs:
> http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/05/
> 
> I also need reviews from GC reviewers for the CSR:
> https://bugs.openjdk.java.net/browse/JDK-8214349
> 
> I already got reviews for:
> [x] shared-runtime (coleenp)
> [x] shared-compiler (kvn)
> 
> I got reviews for shared-build, but an earlier version, so maybe makes
> sense to look over this again. Erik J, Magnus?
> 
> I still need approvals for:
> [ ] shared-build  (kvn, erikj, ihse, pliden)
> [ ] shared-gc (pliden, kbarrett)
> [ ] shared-serviceability (jgeorge, pliden)
> [ ] shared-tests  (lmesnik, pliden)
> [ ] shenandoah-gc
> [ ] shenandoah-tests
> 
> 
> Thanks for your patience and ongoing support!
> 
> Cheers,
> Roman
> 
> 
>> Hi all,
>>
>> here comes round 4 of Shenandoah upstreaming review:
>>
>> This includes fixes for the issues that Per brought up:
>> - Verify and gracefully reject dangerous flags combinations that
>> disables required barriers
>> - Revisited @requires filters in tests
>> - Trim unused code from Shenandoah's SA impl
>> - Move ShenandoahGCTracer to gc/shenandoah
>> - Fix ordering of GC names in various files
>> - Rename UINT64_FORMAT_HEX_W to UINT64_FORMAT_X_W
>>
>> http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/04/
>>
>> Thanks everybody for taking time to review this!
>> Roman
>>
>>> Hello all,
>>>
>>> Thanks so far for all the reviews and support!
>>>
>>> I forgot to update the 'round' yesterday. We are in round 3 now :-)
>>> Also, I fixed the numbering of my webrevs to match the review-round. ;-)
>>>
>>> Things we've changed today:
>>> - We moved shenandoah-specific code out of .ad files into our own .ad
>>> files under gc/shenandoah (in shenandoah-gc), how cool is that? This
>>> requires an addition in build machinery though, see
>>> make/hotspot/gensrc/GensrcAdlc.gmk (in shared-build).
>>> - Improved zero-disabling and build-code-simplification as suggested by
>>> Magnus and Per
>>> - Cleaned up some leftovers in C2
>>> - Improved C2 loop opts code by introducing another APIs in
>>> BarrierSetC2. See the new APIs in shared-gc under BarrierSetC2.hpp.
>>> - I don't see where it makes sense to put INCLUDE_SHENANDOAHGC guards now.
>>> - We would all very much prefer to keep ShenandoahXYZNode names, as
>>> noted earlier. This stuff is Shenandoah-specific, so let's just call it
>>> that.
>>> - Rehashed Shenandoah tests (formatting, naming, directory layout, etc)
>>> - Rebased on jdk-12+22
>>>
>>> - Question: let us know if you need separate RFE for the new BSC2 APIs?
>>>
>>> http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/03/
>>>
>>> Thanks,
>>> Roman
>>>
 Alright, we fixed:
 - The minor issues that Kim reported in shared-gc
 - A lot of fixes in shared-tests according to Leonid's review
 - Disabled SA heapdumping similar to ZGC as Per suggested

 Some notes:
 Leonid:  test/hotspot/jtreg/gc/TestFullGCCount.java was actually
 correct. The @requires there means to exclude runs with both CMS and
 ExplicitGCInvokesConcurrent at the same time, because that would be
 (expectedly) failing. It can run CMS, default GC and any other GC just
 fine. Adding the same clause for Shenandoah means the same, and filters
 the combination (+UseShenandoahGC)+(+ExplicitGCInvokesConcurrent). I
 made the condition 

Re: RFR: JDK-8210106: sun/tools/jps/TestJps.java timed out

[Gary Adams]

On 12/6/18, 7:52 PM, David Holmes wrote:

Hi Gary,

On 6/12/2018 11:27 pm, Gary Adams wrote:
On a local linux-x64-debug build this test consistently runs in less 
than 40 seconds.
On the mach5 testing machines there was a large fluctuation in the 
time to complete this test.


Since the test runs jps with different combinations of arguments, the 
total

test time is dependent on the number of processes running java programs.
Since the mach5 test infrastructure runs java programs I have seen a 3X
in the amount of output the test produces compared to local test
runs.

I've run the test several hundred times through mach5 on the slower 
platforms
and then examined the test logs using a 3X setting from the default 
120 second
jtreg timeout. The slowest reported elapse time from the test logs 
showed

280 seconds to complete.

To improve the reliability of the test to complete, I'd like to 
increase the

timeout to 360 seconds.

   Issue: https://bugs.openjdk.java.net/browse/JDK-8210106

Proposed fix:

diff --git a/test/jdk/sun/tools/jps/TestJps.java 
b/test/jdk/sun/tools/jps/TestJps.java

--- a/test/jdk/sun/tools/jps/TestJps.java
+++ b/test/jdk/sun/tools/jps/TestJps.java
@@ -27,7 +27,7 @@
   * @modules jdk.jartool/sun.tools.jar
   * @build LingeredAppForJps
   * @build LingeredApp
- * @run main/othervm TestJps
+ * @run main/othervm/timeout=360 TestJps
   */


Doesn't that then get scaled by the timeout factor resulting in a much 
longer timeout than the 360 seconds you intended?


For other timeout adjustments the needed time has been divided by the 
timeout factor to get the actual intended timeout.


This bug was filed fairly recently in Aug 2018.
At that time the timeout and timeout factor were not sufficient
to avoid the test failing.

The mach5 timeout factors were adjusted recently, so this test may
no longer be an issue.

If that is true, then we could simply close this bug as "cannot reproduce".
An argument could be made that the change in timeout factor may be
responsible for fixing a lot more of the intermittent bugs and that they
should be closed in a similar manner.

Historically, we could say this particular bug should have had timeouts
reassessed when the infrastructure switched from jprt to mach5 testing
where there were more visible Java processes running.

Using a higher explicit timeout will not make the test run any longer
than it needs. It will simply allow the test to not be terminated sooner
in a hung test scenario.

What is your preference for this particular issue:
   - increase the explicit timeout
   - close as cannot reproduce attributed to the timeout factor adjustments

What would you recommend going forward for other similar issues:
   - determine a new explicit timeout
   - close if no timeout failures have been observed since the timeout 
factor

  was raised




Cheers,
David

Re: RFR (round 5), JDK-8214259: Implementation: JEP 189: Shenandoah: A Low-Pause Garbage Collector

[Aleksey Shipilev]

On 12/4/18 8:10 AM, Roman Kennke wrote:
> Webrevs:
> http://cr.openjdk.java.net/~rkennke/shenandoah-upstream/05/
> 
> [ ] shenandoah-gc
> [ ] shenandoah-tests

These two parts look good.

-Aleksey



signature.asc
Description: OpenPGP digital signature

RE: JDK-8171311 Current state

[Raghavan Puranam]

My apologies Patrick...I should have added the comment first before closing.  I 
have added it now.

Regards,
Raga

-Original Message-
From: Alan Bateman 
Sent: Friday, December 7, 2018 2:52 PM
To: Patrick Reinhart
Cc: serviceability-dev@openjdk.java.net
Subject: Re: JDK-8171311 Current state

On 07/12/2018 08:36, Patrick Reinhart wrote:
> It's a bit disturbing that just at the time of my question this JEP
> has been closed (without any further comment why)
I suspect your inquiry prompted Raghavan to close it as there isn't (to 
my knowledge anyway) anyone actively working on it. I agree a comment is 
needed when closing issues.

>
> I think that it still would be worth while looking into supporting
> a REST based implementation in favour of the existing RMI based
> solution just by the fact of the troubles just one can have with
> firewalls.
Right, and I think there is some interest. In addition to the REST 
adapters that you found then I think some of the app servers have 
support too. The big question for features like this is whether it is 
something that the JDK  has to include or not (the batteries included 
vs. batteries available discussion).  If you look at Harsha's prototype 
(linked from the JEP) then you'll you see it can be mostly developed in 
its own project, the only JDK piece is integrating it with the JMX agent 
and existing -Dcom.sun.management options for starting the JMX agent. I 
think this is an area that could be improved to make it easier to deploy 
JMX adapters that aren't in the JDK.

-Alan

Re: JDK-8171311 Current state

[Alan Bateman]

On 07/12/2018 08:36, Patrick Reinhart wrote:

It's a bit disturbing that just at the time of my question this JEP
has been closed (without any further comment why)
I suspect your inquiry prompted Raghavan to close it as there isn't (to 
my knowledge anyway) anyone actively working on it. I agree a comment is 
needed when closing issues.




I think that it still would be worth while looking into supporting
a REST based implementation in favour of the existing RMI based
solution just by the fact of the troubles just one can have with
firewalls.
Right, and I think there is some interest. In addition to the REST 
adapters that you found then I think some of the app servers have 
support too. The big question for features like this is whether it is 
something that the JDK  has to include or not (the batteries included 
vs. batteries available discussion).  If you look at Harsha's prototype 
(linked from the JEP) then you'll you see it can be mostly developed in 
its own project, the only JDK piece is integrating it with the JMX agent 
and existing -Dcom.sun.management options for starting the JMX agent. I 
think this is an area that could be improved to make it easier to deploy 
JMX adapters that aren't in the JDK.


-Alan

RE: RFR (M) 8214892: Delayed starting of debugging via jcmd

[Langer, Christoph]

Hi Ralf,

thanks for doing this change. Overall, it looks very good to me and seems to be 
a nice feature.

I have thought about a more generic name for the option rather than "onjcmd". 
What about "standby=y". That would indicate that the debugging agent is on 
standby and can be triggered by whatever means. What do others think?

Here are some minor comments, mostly about the wording of text messages:

src/hotspot/share/services/diagnosticCommand.cpp, line 1097:
I'd prefer  if you write out either "Debugging has been started." or "Debugging 
is already active.". I think this would make it more clear for the user of the 
feature what actually happened.

src/hotspot/share/services/diagnosticCommand.hpp, line 878:
"Starts up the Java debugging if enabled in the jdwp agentlib via the onjcmd=y 
option."
A better wording would be:
"Starts up the Java debugging if the jdwp agentlib was enabled with the option 
onjcmd=y."

src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c:
line 876:
replace "debug triggered by jcmd?" with " start debug via jcmd" (or "start 
debug on request" if we opt to change the option name from onjcmd to something 
more general)
line 1300:
 suspendOnInit = JNI_FALSE;
Is it really necessary/desired to set suspendOnInit to false? We could still 
honor suspend=y|n. The suspension will of course happen at the time debug 
activation is triggered.

Another question: Do we need a CSR for this?

Best regards
Christoph

> -Original Message-
> From: serviceability-dev  On
> Behalf Of Schmelter, Ralf
> Sent: Donnerstag, 6. Dezember 2018 15:54
> To: Chris Plummer ; serviceability-
> d...@openjdk.java.net
> Subject: [CAUTION] RE: RFR (M) 8214892: Delayed starting of debugging via
> jcmd
> 
> Hi Chris,
> 
> > I think I prefer passing EI_VM_INIT for triggering_ei, but if you prefer
> > to keep it as is, I suggest a comment to clarify why it might be out of
> > range.
> 
> Actually, I used EI_VM_INIT for the longest time and only changed it
> recently, because I thought that code could assume that e.g. no classes have
> been loaded yet when getting the INIT_VM event. But since the JVMTI spec
> does not guarantees this in any way (it allows other events to be send before
> a VM_INIT), I just will change it back to use EI_VM_INIT for the initialize 
> call.
> 
> Regarding the name of the option, I agree that onjcmd, while not technically
> fully accurate, makes most sense for the common user.
> 
> > ... It think you could just return the error right away and remove
> > the error checking code that comes later.
> 
> I've changed the code to just return the error directly.
> 
> > It's not clear to me why you want the name and address of the first
> > transport. Why is the first one necessarily the one you want?
> 
> Since currently the bag of transports must always have a size of 1, getting 
> the
> first or the last transport is the same. But the callback function used to 
> iterate
> the bag has to return a boolean value. I've decided to return JNI_FALSE,
> which would mean the iteration stops at the first entry.
> 
> 
> I've updated the webrev with your and Goetz Lindenmeier's suggestions:
> http://cr.openjdk.java.net/~rrich/webrevs/schmelter/8214892/webrev.02/
> 
> Best regards,
> Ralf

Re: JDK-8171311 Current state

[Patrick Reinhart]

It's a bit disturbing that just at the time of my question this JEP
has been closed (without any further comment why)

I think that it still would be worth while looking into supporting
a REST based implementation in favour of the existing RMI based
solution just by the fact of the troubles just one can have with
firewalls.

When doing a quick search for JMX REST adapters, I did not found
that many:

- Jolokia (seems to be active) [1]
- Apache ESME - JMX REST API (inactive) [2]
- MX4J (inactive) [3]
- OpenDMK (not found anymore)

-Patrick



I'm not aware of any current activity on this in OpenJDK. One thing
about the JEP is that it didn't make the case clear as to why the
adapter needed to be in the JDK. There are also several existing JMX
adapters that support REST and it would have been useful to evaluate
those and maybe explore what the pain points and issues are with
deploying these solutions. It might be that the -Dcom.sun.management
mechanism to start the JMX agent needs to be improved, maybe it should
be integrated with -javaagent, maybe the pluggability of the JMX agent
just needs to be improved.

-Alan



[1] https://jolokia.org/features-nb.html
[2] https://esme.apache.org/docs/apis/jmx-rest-api.html
[3] http://mx4j.sourceforge.net