[15] RFR: 8193210: [JVMCI/Graal] add JFR compiler phase/inlining events
Jamsheed C M
jamsheed.c.m at oracle.com
Thu Mar 26 10:14:48 UTC 2020
Thank you, Vladimir
Best regards,
Jamsheed
On 26/03/2020 06:02, Vladimir Kozlov wrote:
> 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