[15] RFR: 8193210: [JVMCI/Graal] add JFR compiler phase/inlining events
Vladimir Kozlov
vladimir.kozlov at oracle.com
Thu Mar 26 00:32:41 UTC 2020
On 3/25/20 3:24 PM, Jamsheed C M wrote:
> 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/
Yes, good.
Thanks,
Vladimir
>
> 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-jfr-dev
mailing list