[15] RFR: 8193210: [JVMCI/Graal] add JFR compiler phase/inlining events
Jamsheed C M
jamsheed.c.m at oracle.com
Wed Mar 25 22:24:54 UTC 2020
Hi Vladimir,
Thank you for the review and feedback
reworked webrev (incremental diff) =>
http://cr.openjdk.java.net/~jcm/8193210/webrev.2/webrev.incremental/
full diff => http://cr.openjdk.java.net/~jcm/8193210/webrev.2/webrev.full/
Best regards,
Jamsheed
On 25/03/2020 03:35, Vladimir Kozlov wrote:
> Compiler changes review.
>
> You don't need to put all #if INCLUDE_JFR into compilerEvent.cpp.
> First you can exclude this file from compilation when JFR is not enabled:
>
> make/hotspot/lib/JvmFeatures.gmk Tue Mar 24 14:42:11 2020 -0700
> @@ -173,6 +173,7 @@
> ifneq ($(call check-jvm-feature, jfr), true)
> JVM_CFLAGS_FEATURES += -DINCLUDE_JFR=0
> JVM_EXCLUDE_PATTERNS += jfr
> + JVM_EXCLUDE_FILES += compilerEvent.cpp
> endif
>
> You can use NOT_JFR_RETURN_(x) macro in compilerEvent.hpp (you used it
> already in ticksNow()):
>
> static int register_phases(GrowableArray<const char*>*
> new_phases) NOT_JFR_RETURN_(-1);
>
> You can also add new macro NOT_JFR_RETURN for cases when methods don't
> return.
>
> Other changes in compiler changes look fine to me.
>
> Thanks,
> Vladimir
>
> On 3/23/20 11:06 AM, Jamsheed C M wrote:
>> Hi all,
>>
>> There were some changes to the requirement. JVMCI phase name
>> registration logic likes it to be completely dynamic(or lazily added)
>> making old assumptions and implementation invalid(i.e. The serializer
>> re-registration logic was very wasteful at startup for totally lazily
>> available java phase names). So, the phase names registration logic
>> is changed for better incremental registration support (see
>> CompilerEvent::PhaseEvent::register_phases).
>>
>> JVMCI CompilerPhaseEvent uses JFR C++ event definition
>> EventCompilerPhase(defined in src/share/jfr/metadata/metadata.xml).
>> This required CompilerPhaseType to be extended and used for JVMCI
>> Compiler phase names that are lazily added to the VM, more on it below.
>>
>> Changes to CompilerPhaseType.
>>
>> JFR type CompilerPhaseType is extended for JVMCI use, new mapping is
>> maintained as growable array phase_names (in
>> compiler/compilerEvent.cpp).
>> c2 mapping for the type is located in opto/phasetype.hpp. It is
>> explicit mapping, so if it is used, it should be appended to
>> phase_names before any other mapping.
>> JVMCI phase name are appended to phase_names lazily(when phases are
>> first used by the jvmci compiler). Its phaseToId version is located
>> at jdk.vm.ci.hotspot.JFR.phaseToId.
>>
>> Changes to its serializer[1].
>>
>> CompilerPhaseTypeConstant serializer requires changes for handling
>> dynamic entries.
>>
>> The types managed by the JfrSerializer framework are usually static
>> by nature, but there were exceptions like JfrNetworkUtilization(where
>> the type values are added lazily/dynamic).
>> Although adding new entries to the type lazily and serializing it for
>> every new chunk created later was easy to do with current setup,
>> there was no external support available for serializing the new
>> entries for the current chunk (i.e. JVMCI use case was similar to
>> JfrNetworkUtilization use case, but JfrCheckpointWriter was internal
>> feature and was not available outside JFR module)
>>
>> So the serializer re-registration feature was used for serializing it
>> for the current chunk in the webrev[2], but it was wasteful at startup.
>>
>> JFR team provided some improvement over this, and made
>> JfrCheckpointWriter available outside JFR module. This was possible
>> by the improved flush logic provided by Markus.
>> The new improved phase name registration logic is available at
>> webrev.1 http://cr.openjdk.java.net/~jcm/8193210/webrev.1/
>>
>> Files changed in webrev.1
>> src/hotspot/share/compiler/compilerEvent.hpp
>> src/hotspot/share/compiler/compilerEvent.cpp
>> src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
>> src/hotspot/share/compiler/compileBroker.cpp
>>
>> JFR changes (Flush logic improvement provided my Markus)
>> src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.cpp
>> src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.hpp
>> src/hotspot/share/jfr/recorder/checkpoint/types/jfrTypeManager.cpp
>> src/hotspot/share/jfr/recorder/checkpoint/types/jfrTypeManager.hpp
>> src/hotspot/share/jfr/recorder/service/jfrRecorderService.cpp
>>
>>
>> Other changes (Function name changes)
>> http://cr.openjdk.java.net/~jcm/8193210/webrev.1/src/hotspot/share/jvmci/jvmciCompilerToVM.cpp.udiff.html
>>
>> http://cr.openjdk.java.net/~jcm/8193210/webrev.1/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/CompilerToVM.java.udiff.html
>>
>> tracingTimeNow => ticksNow
>> writeEventCompilerPhase => notifyCompilerPhaseEvent
>> writeEventCompilerInlining => notifyCompilerInliningEvent
>>
>> http://cr.openjdk.java.net/~jcm/8193210/webrev.1/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/JFR.java.html
>>
>> registerCompilerPhases => registerPhases
>> Also, added logic for duplicate removal for non regular bulk
>> registration use case(not properly synchronized, so there can be
>> still duplicates in registrationl).
>>
>> Thanks a lot Markus for the continuous support provided for this work.
>>
>> Please review.
>>
>> Best regards,
>> Jamsheed
>>
>>
>> [1] Serializers for types(subclasses of JfrSerializer) serializes
>> type on its registration, once registered, type is serliazed for
>> every new chunks that is created later.
>> [2] http://cr.openjdk.java.net/~jcm/8193210/webrev/
>> [3] http://cr.openjdk.java.net/~jcm/8193210/webrev.1/
>>
>> On 06/03/2020 22:34, Jamsheed C M wrote:
>>
>>> Hi Markus,
>>>
>>> On 06/03/2020 21:32, Markus Gronlund wrote:
>>>>
>>>> Hi Jamsheed,
>>>>
>>>> Looks good.
>>>>
>>> Thank you for the review.
>>>>
>>>> Good work in getting this integration in place.
>>>>
>>> Sure, it wouldn't have been easy without the quick jfr serializer
>>> (Re)registration support provided by you. Thanks for it again.
>>>
>>> Best regards,
>>>
>>> Jamsheed
>>>
>>>> Thanks
>>>>
>>>> Markus
>>>>
>>>> *From:* Jamsheed C M
>>>> *Sent:* den 29 februari 2020 08:28
>>>> *To:* hotspot-jfr-dev at openjdk.java.net;
>>>> hotspot-compiler-dev at openjdk.java.net; Markus Gronlund
>>>> <markus.gronlund at oracle.com>; Doug Simon <doug.simon at oracle.com>;
>>>> Vladimir Kozlov <vladimir.kozlov at oracle.com>
>>>> *Subject:* Re: [15] RFR: 8193210: [JVMCI/Graal] add JFR compiler
>>>> phase/inlining events
>>>>
>>>> Hi all,
>>>>
>>>> i did some more updates to webrev.
>>>>
>>>> 1) used short names for CompilerEvent::PhaseEvent::post.. shortened
>>>> to CompilerEvent::PhaseEvent::post
>>>>
>>>> 2) added detailed comments for
>>>> CompilerEvent::PhaseEvent::register_phases. There is some implicit
>>>> assumption at usage site, like c2 is always registered first. added
>>>> comment to make it clear, and made registration final if jvmci is
>>>> not included.
>>>>
>>>> Best regards,
>>>>
>>>> Jamsheed
>>>>
>>>> On 28/02/2020 19:52, Jamsheed C M wrote:
>>>>
>>>> also removed compilerEvent.inline.hpp.. and made most of guarded
>>>> profiling code out-of-line.
>>>>
>>>> updated webrev in place.
>>>>
>>>> Best regards,
>>>>
>>>> Jamsheed
>>>>
>>>> On 28/02/2020 10:38, Jamsheed C M wrote:
>>>>
>>>> Hi,
>>>>
>>>> missed newly added Deoptimization event test. also
>>>> TestJfrJavaBase.java test**
>>>>
>>>> modified deopt test for jvmci(graal) use case, TestJfrJavaBase
>>>> is not relevant as compiler is not available for the test.
>>>>
>>>> updated webrev in place.
>>>>
>>>> Best regards,
>>>>
>>>> Jamsheed
>>>>
>>>> On 26/02/2020 14:19, Jamsheed C M wrote:
>>>>
>>>> Hi,
>>>>
>>>> i messed up with order of includes, got undetected due to
>>>> PCH. corrected it, updated webrev in place.
>>>>
>>>> corrected the title.
>>>>
>>>> Best regards,
>>>>
>>>> Jamsheed
>>>>
>>>> On 26/02/2020 07:35, Jamsheed C M wrote:
>>>>
>>>> Hi,
>>>>
>>>> This enhancement adds jvmci supports that is required
>>>> for extending existing phase/inlining events support
>>>> in graal compilers.
>>>>
>>>> During this enhancement all jfr events related codes
>>>> are moved to compiler/compilerEvent.[.hpp, inline.hpp,
>>>> .cpp]
>>>>
>>>> Graal use these events via jvmci JFR helper class,
>>>> which uses c2v interface to post and get information
>>>> from hotspot
>>>>
>>>> Overview of functionalities exposed by JFR helper
>>>> class
>>>> <http://cr.openjdk.java.net/~jcm/8193210/webrev/src/jdk.internal.vm.ci/share/classes/jdk.vm.ci.hotspot/src/jdk/vm/ci/hotspot/JFR.java.html>
>>>>
>>>>
>>>> Ticks, for getting current jfr counter/ time stamp.
>>>> used for getting start time of events by Graal
>>>> compiler.
>>>>
>>>> CompilerPhaseEvent for posting phase event to
>>>> hotspot JFR.
>>>>
>>>> CompilerInlininingEvent for posting inlininig events.
>>>>
>>>> CompilerPhaseEvent extends and use
>>>> TYPE_COMPILERPHASETYPE content type for phase events,
>>>> this is achieved using reregistration support added by
>>>> Markus [1]
>>>>
>>>> CompilerEvent::PhaseType::register_phases is used for
>>>> registering phases. This function is usually called on
>>>> compiler singleton object creation. It is also called
>>>> lazily if phases are not known beforehand.
>>>>
>>>> The event generation codes in c1/c1_GraphBuilder.cpp,
>>>> opto/bytecodeInfo.cpp, opto/compile.hpp uses common
>>>> functions exposed by compiler/compilerEvent.hpp
>>>> <http://cr.openjdk.java.net/~jcm/8193210/webrev/src/hotspot/share/compiler/compilerEvent.hpp.html>/
>>>>
>>>> inline.hpp
>>>> <http://cr.openjdk.java.net/~jcm/8193210/webrev/src/hotspot/share/compiler/compilerEvent.inline.hpp.html>/ ..cpp
>>>>
>>>> <http://cr.openjdk.java.net/~jcm/8193210/webrev/src/hotspot/share/compiler/compilerEvent.cpp.html>
>>>>
>>>>
>>>> webrev:
>>>> http://cr.openjdk.java.net/~jcm/8193210/webrev/
>>>>
>>>> jbs: https://bugs.openjdk.java.net/browse/JDK-8193210
>>>>
>>>> Testing: ran jdk/jfr in my local machine. mach5 run
>>>> created, links in jbs.
>>>>
>>>> Please review,
>>>>
>>>> Best regards,
>>>>
>>>> Jamsheed
>>>>
>>>> [1]
>>>> http://cr.openjdk.java.net/~jcm/8193210/webrev/src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointManager.cpp.udiff.html
>>>>
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~jcm/8193210/webrev/src/hotspot/share/jfr/recorder/checkpoint/jfrCheckpointWriter.hpp.udiff.html
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~jcm/8193210/webrev/src/hotspot/share/jfr/recorder/checkpoint/types/jfrTypeManager.cpp.udiff.html
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~jcm/8193210/webrev/src/hotspot/share/jfr/recorder/checkpoint/types/jfrTypeManager.hpp.udiff.html
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~jcm/8193210/webrev/src/hotspot/share/jfr/utilities/jfrTypes.hpp.udiff.html
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~jcm/8193210/webrev/src/jdk.jfr/share/classes/jdk/jfr/internal/consumer/ChunkParser.java.udiff.html
>>>>
>>>>
More information about the hotspot-compiler-dev
mailing list