8161605 : The '!UseSharedSpaces' check is not need in JvmtiManageCapabilities::recompute_always_capabilities

[Alex Menkov]

Hi all,

Please take a look at a fix for
JDK-8161605 : The '!UseSharedSpaces' check is not need in 
JvmtiManageCapabilities::recompute_always_capabilities


The fix moves can_generate_all_class_hook_events capability from 
"onload_capabilities" to "always_capabilities" and cleans up 
recompute_always_capabilities method.


jira: https://bugs.openjdk.java.net/browse/JDK-8161605
webrev: http://cr.openjdk.java.net/~amenkov/can_generate_class_hook/webrev/

--alex

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Hohensee, Paul]

Hi Erik & co, thanks for looking at this.

Would you be ok with pushing this fix (it really is a bug!) and then me doing a 
followup RFE? That way, I can backport the fix to 8u and eventually remove the 
patch I’ve already pushed to our OpenJDK8 internal release.

Some background. We used to measure heap occupancy using MemoryPoolMXBean.Usage 
and alarm on it when it got ‘too high’. The problem is that it’s an 
instantaneous measure and therefore includes an unknown amount of garbage, so 
you can’t determine where to set an alarm, because you don’t know how much of 
it will be collected in the near future. We want to detect steadily increasing 
memory use such as you’d see with a leak, so we’re switching to 
MemoryPoolMXBean.CollectionUsage, which is the usage immediately after the last 
GC that affected the memory pool. We have a synthetic metric that’s defined for 
all collectors as (sum of CollectionUsage.used for non-eden pools) / (sum of 
CollectionUsage.max-if-defined for all non-eden pools), and alarm on that. 
‘max-if-defined’ means zero if JMX returns -1, which is the ‘undefined’ value.

The JMX API spec doesn’t specify what the memory pool or garbage collector 
names are, but the current names are de-facto part of the API, so if we change 
the existing ones, imo a CSR should be filed. To avoid that, how about we keep 
the existing memory pool names the same and, in the spirit of compatibility, 
make the new ones similar to the existing ones. I.e.,

“G1 Eden Space”
“G1 Survivor Space”
“G1 Old Gen”
“G1 Humongous Space”
“G1 Archive Space”

Alternatively, we can make the existing names aliases for the new ones, though 
“Regions” seems a bit G1-specific to me and doesn’t convey the relationship to 
the equivalent spaces in the other collectors. Especially if there’s an archive 
space for the other collectors (see below).

There’s no specification requirement that memory pools be disjoint. What do you 
think of defining the archive space as a subset of the old gen? That way 
existing code can stay the same (it typically just iterates over a collector’s 
pools as above), and new code can decide exactly what it wants to report. 
Do/should the non-G1 collectors have an archive space too? If so, we should 
just call it “Archive Space” and not make it G1 specific. I’d guess for non-G1 
collectors it’s just the initial live prefix of the old gen and therefore 
ignored during a collection. In this scheme, the initial value of ‘used’ for 
all collectors’ oldgens would be the size of the archive space. All the archive 
space’s MemoryUsage attributes would have the same value.

Not attaching the archive space to any collector seems correct, because a 
collector’s memory pools are defined to be the ones that it affects.

Currently, Usage.max and CollectionUsage.max for all G1 memory pools other than 
the oldgen is -1, for “undefined”. For the oldgen it’s –Xmx. This makes it easy 
to generate aggregate metrics for all the collectors by just summing used 
values and dividing by the sum of max values as described above. It would be 
nice to keep this characteristic. Otherwise we’d have to write special-case 
code for G1, and change existing code to check for which JDK we’re running on.

I’m uncertain whether your definition of the memory pool usage fields is for 
MemoryPool.Usage or MemoryPool.CollectionUsage. Seems like the former, which is 
fine and matches the current definition, except for max.

All the existing collector names have an implicit “ Collector” suffix, e.g., 
“ConcurrentMarkSweep” means “ConcurrentMarkSweep Collector”. So, I’d use

“G1 Young”
“G1 Mixed”
“G1 Full”
“G1 Concurrent Cycle”

keep the existing “G1 Young Collection” as an alias for both “G1 Young” and “G1 
Mixed”, and keep the existing “G1 Old Collection” as an alias for “G1 Full”. If 
you accept this and the memory pool name suggestions, then strictly speaking 
you don’t need a CSR.

The definition of the “G1 Concurrent Cycle” elapsed time makes sense to me. The 
young collection would still be reported separately, right?

Paul

On 1/25/18, 5:27 AM, "Erik Helin"  wrote:

Hi Paul,

thanks for your interest in this area and for your patch! The 
GarbageCollectorMXBean and MemoryPoolMXBean support for G1 is in need of 
some updates, so thanks for working on this.

Looking at your patch, I'm not sure that this is the direction we want 
to go in. I discussed this a bit with Thomas and Stefan J, and our 
current line of thinking is the following:

- Memory pools (MemoryMXBean):
   - "G1 Eden Regions"
   - "G1 Survivor Regions"
   - "G1 Old Regions"
   - "G1 Humongous Regions"
   - "G1 Archive Regions" (if CDS and/or AppCDS is used)

`init` for these pools would be 0, `used` would be total size of the 
"live" objects in the used regions of that type, `committed` the total 
size of the used regions of that that type and `max` would be 

Re: RFR: JDK-8080990: libdt_socket/socket_md.c(202) : warning C4996: 'gethostbyname': Use getaddrinfo() or GetAddrInfoW()

[Erik Joelsson]

Build change looks fine.

/Erik


On 2018-01-25 10:46, Gary Adams wrote:
Here's a first pass attempt to remove the 
-D_WINSOCK_DEPRECATED_NO_WARNINGS

build flag and update the winsock deprecated functions.

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

These are the initial deprecated functions updated:
   gethostbyname -> getaddrinfo
   inet_addr -> inet_pton
   inet_ntoa -> inet_ntop

I'm not sure how to replace the existing WSASendDisconnect calls.
It's not clear that it actually provides a graceful disconnect.

RFR: JDK-8080990: libdt_socket/socket_md.c(202) : warning C4996: 'gethostbyname': Use getaddrinfo() or GetAddrInfoW()

[Gary Adams]

Here's a first pass attempt to remove the -D_WINSOCK_DEPRECATED_NO_WARNINGS
build flag and update the winsock deprecated functions.

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

These are the initial deprecated functions updated:
   gethostbyname -> getaddrinfo
   inet_addr -> inet_pton
   inet_ntoa -> inet_ntop

I'm not sure how to replace the existing WSASendDisconnect calls.
It's not clear that it actually provides a graceful disconnect.

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

[mandy chung]

Hi Harsha,

JEP 293 [1] describes the guidelines for JDK command-line options.  As 
Alan points out, new options should move away from -X prefix but use 
`--` GNU-style long form option.  The guideline says:


The use of |-X| as a prefix to indicate "non-standard" options will be 
discontinued for new options, although command-line help may continue to 
draw a distinction between more commonly used options and those for 
advanced use.


You can consider `--management` as an alternative.  Should this be a 
launcher option that converts it to the corresponding 
`-Dcom.sun.management.jmxremote.` rather than a VM option?


Mandy
[1] http://openjdk.java.net/jeps/293

On 1/24/18 11:21 PM, Harsha Wardhana B wrote:


Hi Erik,

The minimal command line would be,

"-Xmanagement", that will start only the local management server.

"-Xmanagement:local=true,port=" will start the remote management 
server without SSL or authentication.



On Wednesday 24 January 2018 06:13 PM, Erik Gahlin wrote:

Hi Harsha,

Very nice to see progress on this!

Before reviewing, the minimal command line to start up the default 
management server now becomes


-Xmanagement:ssl=false,authenticate=false

No. Please refer above for minimal options.


and if you use a property that doesn't exist, or that is mandatory, 
you will get an error message stating what is wrong?
If we use property, that doesn't exist, we get invalid option error. 
As said before, no options are mandatory.

///
//./java -Xmanagement:ssl=true,authenticate=false,rmiregistry_ssl=true 
HelloWorld//

//Error: Invalid option specified: rmiregistry_ssl//
///


Could we reduce the command line further, so only a single property 
is needed:


-Xmanagement:secure=false

or perhaps:

-Xmanagement:unsecure

which would set ssl=false,authenticate=false, because that is what 
you want 99% of the time.


Thanks
Erik


Thanks
Harsha

Hi,

Please review the changes for above enhancement having webrev at,

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

Thanks
Harsha

Re: RFR (S): 8195115: G1 Old Gen MemoryPool CollectionUsage.used values don't reflect mixed GC results

[Erik Helin]

Hi Paul,

thanks for your interest in this area and for your patch! The 
GarbageCollectorMXBean and MemoryPoolMXBean support for G1 is in need of 
some updates, so thanks for working on this.


Looking at your patch, I'm not sure that this is the direction we want 
to go in. I discussed this a bit with Thomas and Stefan J, and our 
current line of thinking is the following:


- Memory pools (MemoryMXBean):
  - "G1 Eden Regions"
  - "G1 Survivor Regions"
  - "G1 Old Regions"
  - "G1 Humongous Regions"
  - "G1 Archive Regions" (if CDS and/or AppCDS is used)

`init` for these pools would be 0, `used` would be total size of the 
"live" objects in the used regions of that type, `committed` the total 
size of the used regions of that that type and `max` would be 
MaxHeapSize. Note that "live" here means live from the GCs point of 
view, i.e. an object might be dead in an old region but the GC will 
consider that object live until a concurrent cycle has marked through 
the heap and deemed it dead.


- Collectors (GarbageCollectorMXBean):
  - "G1 Young Collector" with the pools
- "G1 Eden Regions"
- "G1 Survivor Regions"
- "G1 Humongous Regions" (due to early reclamation)
  - "G1 Mixed Collector" with the pools
- "G1 Eden Regions"
- "G1 Survivor Regions"
- "G1 Old Regions"
- "G1 Humongous Regions" (due to early reclamation)
  - "G1 Full Collector" with the pools
- "G1 Eden Regions"
- "G1 Survivor Regions"
- "G1 Old Regions"
- "G1 Humongous Regions" (can collect empty humongous regions)
  - "G1 Concurrent Cycle" with the pools
- "G1 Old Regions" (can collect empty old regions)
- "G1 Humongous Regions" (can collect empty humongous regions)

Note that with this design, the 
GarbageCollectorMXBean::getCollectionTime() method for "G1 Concurrent 
Cycle" would be the wall clock time from start of the first initial mark 
to end of the last cleanup (also including the time of any eventual 
young collection during the concurrent cycle). So 
GarbageCollectorMXBean::getCollectionTime() would be a mix of concurrent 
and STW time for the GarbageCollectorMXBean with name "G1 Concurrent Cycle".


Also note that the MemoryPoolMXBean with name "G1 Archive Regions" will 
not be attached to any GarbageCollectorMXBean, since those regions will 
never be collected.


What do you think about this design, would it work for your use case?

If we want to go ahead with this design, then I think we might have to 
file a CSR. David (who is the HotSpot CSR representative), do we have to 
file a CSR for changing the names of MemoryPoolMXBeans and 
GarbageCollectorMXBeans?


Thanks,
Erik

On 01/20/2018 12:40 AM, Hohensee, Paul wrote:

I’d appreciate a review please.

Bug: https://bugs.openjdk.java.net/browse/JDK-8195115

Webrev: http://cr.openjdk.java.net/~phh/8195115/webrev.00/

The bug is that from the JMX point of view, G1’s incremental collector 
(misnamed as the “G1 Young Generation” collector) only affects G1’s 
survivor and eden spaces. In fact, mixed collections run by this 
collector also affect the G1 old generation.


This proposed fix is to record, for each of a JMX garbage collector's 
memory pools, whether that memory pool is affected by all collections 
using that collector. And, for each collection, record whether or not 
all the collector's memory pools are affected. After each collection, 
for each memory pool, if either all the collector's memory pools were 
affected or the memory pool is affected for all collections, record 
CollectionUsage for that pool.


For collectors other than G1 Young Generation, all pools are recorded as 
affected by all collections and every collection is recorded as 
affecting all the collector’s memory pools. For the G1 Young Generation 
collector, the G1 Old Gen pool is recorded as not being affected by all 
collections, and non-mixed collections are recorded as not affecting all 
memory pools. The result is that for non-mixed collections, 
CollectionUsage is recorded after a collection only the G1 Eden Space 
and G1 Survivor Space pools, while for mixed collections CollectionUsage 
is recorded for G1 Old Gen as well.


Other than the effect of the fix on G1 Old Gen MemoryPool. 
CollectionUsage, the only external behavior change is that 
GarbageCollectorMXBean.getMemoryPoolNames will now return 3 pool names 
rather than 2.


With this fix, a collector’s memory pools can be divided into two 
disjoint subsets, one that participates in all collections and one that 
doesn’t. This is a bit more general than the minimum necessary to fix 
G1, but not by much. Because I expect it to apply to other incremental 
region-based collectors, I went with the more general solution. I 
minimized the amount of code I had to touch by using default parameters 
for GCMemoryManager::add_pool and the TraceMemoryManagerStats constructors.


Tested by running the new jtreg test included in the webrev. I tried to 
use the submit repo, but it was out of order earlier 

Re: Low-Overhead Heap Profiling

[Robbin Ehn]

Hi JC, great to see another revision!


heapMonitoring.cpp

StackTraceData should not contain the oop for 'safety' reasons.
When StackTraceData is moved from _allocated_traces:
L452 store_garbage_trace(trace);
it contains a dead oop.
_allocated_traces could instead be a tupel of oop and StackTraceData thus dead 
oops are not kept.


You should use the new Access API for loading the oop, something like this:
RootAccess::load(...)
I don't think you need to use Access API for clearing the oop, but it would look 
nicer. And you shouldn't probably be using: Universe::heap()->is_in_reserved(value)


The lock:
L424   MutexLocker mu(HeapMonitorStorage_lock);
Is not needed as far as I can see.
weak_oops_do is called in a safepoint, no TLAB allocation can happen and JVMTI 
thread can't access these data-structures. Is there something more to this lock 
that I'm missing?



You have 6 files without any changes in them (any more):
g1CollectedHeap.cpp
psMarkSweep.cpp
psParallelCompact.cpp
genCollectedHeap.cpp
referenceProcessor.cpp
thread.hpp


I have not looked closely, but is it possible to hide heap sampling in 
AllocTracer ? (with some minor changes to the AllocTracer API)



Minor nit, when declaring pointer there is a little mix of having the pointer 
adjacent by type name and data name. (Most hotspot code is by type name)

E.g.
heapMonitoring.cpp:711 jvmtiStackTrace *trace = 
heapMonitoring.cpp:733 Method* m = vfst.method();
(not just this file)


HeapMonitorThreadOnOffTest.java:77
I would make g_tmp volatile, otherwise the assignment in loop may theoretical be 
skipped.


Thanks!

/Robbin

On 01/24/2018 01:40 AM, JC Beyler wrote:

And it has been exactly two months since I posted an update here :)

Thanksgiving, Christmas, and handling
https://bugs.openjdk.java.net/browse/JDK-8190862 will do that to you
:)

I have gotten back to this item now that JDK-8190862 is done and I
have the following webrev ready:
http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.02/

With the incremental here:
http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.01_02/

The updates are:

a) GetCachedTraces

- I added a GC collection if the GetLiveTraces is called, this is
because you really want to know what is currently live when you call
that method
- If you are worried about performance and only care about what was
live at the last GC, you can now call GetCachedTraces, which does not
provoke a GC

Let me know if there are any questions here or concerns. I'm happy to
explain and defend the choices :).
Note: I added a new test for this and updated other tests to double
check the live call. (see for example
http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.01_02/test/hotspot/jtreg/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorCachedTest.java.html)

b) Disabling samples wipes out the data

I had originally implemented for OpenJdk a version that keeps the data
in memory after disabling the sampler, allowing a user to get traces
post-sampling. Because of this, we would always do the weak_oops_do
method, whether enabled or disabled. This led to a slight regression
in performance for GC reference processing time. I had initially fixed
this with a small "was this ever enabled" flag. This would have
allowed a program that never uses this to not have a regression but a
program that enables the disabled the code for the rest of the
execution would still pay the price after disabling the sampler.

Because of this, I have moved things back to where they probably
should be: if you disable the sampler, you lose the data. But this
allows a simpler code path: if the sampler is disabled, skip over the
GC weak_oops_do method.

Let me know what you think and happy 2018 to all!
Jc

On Thu, Nov 23, 2017 at 7:20 AM, Thomas Schatzl
 wrote:

On Tue, 2017-11-21 at 13:50 -0800, JC Beyler wrote:

Hi all,

I have a new webrev here:
http://cr.openjdk.java.net/~rasbold/8171119/webrev.15/

With the incremental one here:
http://cr.openjdk.java.net/~rasbold/8171119/webrev.14a_15/

I think I did most items from Thomans and Robbin. I've especially
added a new test:


Thanks. Looks good.

Thanks,
   Thomas