RFR(L): 8198691: CodeHeap State Analytics

Tue Mar 6 17:03:35 UTC 2018

Hi Tobias, 

thanks for the extensive testing. Here are my comments/actions: 

ISSUES
======
 - frameLine/textLine warnings: These were leftover declarations from when I consolidated all that box printing into printBox(). Removed. 
 - "+=" warning: Resolved by using "unsigned int" and explicit cast "(unsigned int)strlen(text)"
 - "memset" error: Only ran into this problem when moving the new code to a separate file. Resolved by casting to (void*) as suggested.
 - failing AOT tests: I will restrict analytics to FOR_ALL_ALLOCABLE_HEAPS(), as suggested by Vladimir. Not sure if that will heal the tests.
 - SIGFPE in CodeHeap::aggregate(): could that be related to the "memset" error? Never had that here at SAP, neither in OpenJDK test nor in SAP JVM. If that issue persists with the new webrev (coming up soon), I may need ad'l information. If not activated by command line argument or explicit call, the new code has _ZERO_ effect. 

COMMENTS
==========
 - Documentation format: which format would you need? The PDF I uploaded is generated from our internal Wiki. It was the least effort for now. I could also provide plain text (losing all the formatting) or MS Word .docx (hopefully preserving most of the formatting).
 - Documentation location: I'm not familiar with the policies that direct documentation to a certain place. Please put it where you think it's appropriate. Of course I will help wherever I can.
 - Documentation contents/writing style: I will ask a SAP documentation specialist to have a look at it once it's final content-wise. That might eliminate some German-sounding English text.
 - Printing on CodeCache full condition: We have that in our SAP JVM product. Must be activated by a command line argument. Already proved helpful.
 - Expect "unified logging" instead of "PrintCodeHeapState" with the new webrev.
 - Code style: will move the '*' and ',' as requested.

So please stay tuned. I'm working hard to get all the modifications ready. It's a lot to do and, unfortunately, there is that day-to-day business demanding some attention as well. 

Regards, 
Lutz

On 06.03.18, 14:32, "Tobias Hartmann" <tobias.hartmann at oracle.com> wrote:

    Hi Lutz,

    I found more build/test problems and added a summary comment to the bug. Please let me know if you
    need more information.

    Best regards,
    Tobias

    On 06.03.2018 11:31, Tobias Hartmann wrote:
    > I've quickly ran this through our testing and there are some build failures:
    > 
    > jib > t:/workspace/open/src/hotspot/share/memory/heap.cpp(1710) : warning C4101: 'frameLine' :
    > unreferenced local variable
    > jib > t:/workspace/open/src/hotspot/share/memory/heap.cpp(1711) : warning C4101: 'textLine' :
    > unreferenced local variable
    > jib > t:/workspace/open/src/hotspot/share/memory/heap.cpp(1922) : warning C4101: 'frameLine' :
    > unreferenced local variable
    > jib > t:/workspace/open/src/hotspot/share/memory/heap.cpp(1923) : warning C4101: 'textLine' :
    > unreferenced local variable
    > jib > t:/workspace/open/src/hotspot/share/memory/heap.cpp(2094) : warning C4101: 'frameLine' :
    > unreferenced local variable
    > jib > t:/workspace/open/src/hotspot/share/memory/heap.cpp(2095) : warning C4101: 'textLine' :
    > unreferenced local variable
    > jib > t:/workspace/open/src/hotspot/share/memory/heap.cpp(2282) : warning C4101: 'frameLine' :
    > unreferenced local variable
    > jib > t:/workspace/open/src/hotspot/share/memory/heap.cpp(2283) : warning C4101: 'textLine' :
    > unreferenced local variable
    > jib > t:/workspace/open/src/hotspot/share/memory/heap.cpp(2479) : warning C4101: 'frameLine' :
    > unreferenced local variable
    > jib > t:/workspace/open/src/hotspot/share/memory/heap.cpp(2480) : warning C4101: 'textLine' :
    > unreferenced local variable
    > jib > t:/workspace/open/src/hotspot/share/memory/heap.cpp(2699) : warning C4267: '+=' : conversion
    > from 'size_t' to 'int', possible loss of data
    > jib > t:/workspace/open/src/hotspot/share/memory/heap.cpp(2700) : warning C4267: '+=' : conversion
    > from 'size_t' to 'int', possible loss of data
    > 
    > Best regards,
    > Tobias
    > 
    > On 06.03.2018 11:25, Tobias Hartmann wrote:
    >> Hi Lutz,
    >>
    >> first of all, this is a very nice enhancement!
    >>
    >> I haven't looked at the code in detail yet (will wait for the next webrev). Here are some comments:
    >> - The documentation is great and should at least go into the release notes (I've added the
    >> release-not=yes label). I think we should also add something to the Tools Reference Guide [1] or
    >> even the JVM Guide [2].
    >> - As Robbin already pointed out, please use unified logging instead of PrintCodeHeapState. I think
    >> that would also allow to specify <function> and <granularity> on the command line.
    >> - I think it would be nice to print some/all of this new information if the code cache is full at
    >> CodeCache::report_codemem_full() if PrintCodeHeapState is enabled. We had several customer reported
    >> issues in the past where the compilers were disabled although there still was some free space in the
    >> code cache. We were never able to reproduce/analyze but always expected this to be due to high
    >> fragmentation.
    >> - JFR integration would be nice but that should be done in a separate RFE
    >> - It's a style question but I would prefer the pointer asterisk at the type not the argument. For
    >> example, "outputStream* out" instead of "outputStream *out"
    >> - heap.hpp: please put the commas after the definitions in the enum
    >>
    >> Thanks,
    >> Tobias
    >>
    >> [1] https://docs.oracle.com/javase/9/tools/java.htm#JSWOR624
    >> [2]
    >> https://docs.oracle.com/javase/9/vm/java-virtual-machine-technology-overview.htm#JSJVM-GUID-982B244A-9B01-479A-8651-CB6475019281
    >>
    >>
    >> On 01.03.2018 18:17, Schmidt, Lutz wrote:
    >>> Dear all,
    >>>
    >>>  
    >>>
    >>> may I please request reviews for this quite voluminous enhancement:
    >>>
    >>>  
    >>>
    >>> Bug:     https://bugs.openjdk.java.net/browse/JDK-8198691
    >>>
    >>> Webrev:  http://cr.openjdk.java.net/~lucy/webrevs/8198691.00/index.html  
    >>>
    >>>  
    >>>
    >>> Don’t get afraid! Most of the logic is new and isolated in a big, separate block in heap.cpp. The
    >>> changes to other files are not difficult to understand.
    >>>
    >>>  
    >>>
    >>> If you need information about what this enhancement does and how it can be used, please refer to the
    >>> bug description. There I have attached some documentation which will greatly help with understanding
    >>> the code.
    >>>
    >>>  
    >>>
    >>> Thank you!
    >>>
    >>> Lutz
    >>>
    >>>  
    >>>
    >>>  
    >>>
    >>>  
    >>>