Re: RFC: JWarmup precompile java hot methods at application startup
Kuai Wei
kuaiwei.kw at alibaba-inc.com
Tue Jul 23 01:46:43 UTC 2019
Hi David,
Thanks for the clarification. We will update wiki and patch for next round review.
Kuai Wei
------------------------------------------------------------------
From:David Holmes <david.holmes at oracle.com>
Send Time:2019年7月22日(星期一) 14:49
To:蒯微(麦庶) <kuaiwei.kw at alibaba-inc.com>; yumin qi <yumin.qi at gmail.com>; hotspot-runtime-dev at openjdk.java.net <hotspot-runtime-dev at openjdk.java.net>
Cc:hotspot-dev <hotspot-dev at openjdk.java.net>
Subject:Re: RFC: JWarmup precompile java hot methods at application startup
Hi Kuai,
On 21/06/2019 5:18 pm, Kuai Wei wrote:
> Hi David,
>
> Sorry for the late reply.
Sorry for my even later one. I was traveling and then had vacation, and
have had other things to look at.
> We plan to create a wiki page on OpenJDK website and put the design documents there. How do you think about it?
That sounds like a good idea.
> Here are the answers to some questions in your last message:
I haven't had time to context switch in everything sorry. So just a
couple of responses below.
> - "source file" in JWarmUp record:
> Application will load same class from multiple places. For example, logging jar will be packaged by different web apps. So we record this property.
>
> - super class resolution
> It's used for diagnostic. Same class loaded by different loaders will cause a warning message in PreloadClassChain::record_loaded_class(). We use the super class resolve mark to reduce warning messages when resolving super class. We are thinking to refine it.
If "refine" means remove then I encourage your thinking :)
>
> - dummy method
> It's hard to know whether JWarmUp compilations are completed or not. The dummy method is used as the last method compiled due to JWarmUp. We are able to check its entry to see whether all compilations are finished.
>
> - native entry in jvm.cpp
> JWarmUp defined some jvm entries which are invoked by java. We assume all jvm entries are put into jvm.cpp. Would you give us some reference we can follow?
jvm.cpp contains the definitions of the JVM entry point methods but it
doesn't (as you can see in existing file) contain code for registering
those methods:
#define CC (char*)
static JNINativeMethod jdk_jwarmup_JWarmUp_methods[] = {
{ CC "notifyApplicationStartUpIsDone0", CC "()V", (void
*)&JVM_NotifyApplicationStartUpIsDone},
{ CC "checkIfCompilationIsComplete0", CC "()Z", (void
*)&JVM_CheckJWarmUpCompilationIsComplete},
{ CC "notifyJVMDeoptWarmUpMethods0", CC "()V", (void
*)&JVM_NotifyJVMDeoptWarmUpMethods}
};
JVM_ENTRY(void, JVM_RegisterJWarmUpMethods(JNIEnv *env, jclass
jwarmupclass))
JVMWrapper("JVM_RegisterJWarmUpMethods");
ThreadToNativeFromVM ttnfv(thread); // can't be in VM when we call JNI
int ok = env->RegisterNatives(jwarmupclass,
jdk_jwarmup_JWarmUp_methods,
sizeof(jdk_jwarmup_JWarmUp_methods)/sizeof(JNINativeMethod));
guarantee(ok == 0, "register jdk.jwarmup.JWarmUp natives");
JVM_END
#undef CC
That is typically done by the C code in the JDK. See for example
src/java.base/share/native/libjava/System.c
> - logging flags
> JWarmUp was initially developed for JDK8. A flag was used to print out trace. When we ported the patch to JDK tip, we changed code to use the new log utility but with the legacy flag kept.
Please remove legacy flag.
> - VM flags
> We will check and remove unnecessary flags.
Thank you.
> - init.cpp and mutex initialization
> We will modify that.
>
> - Deoptimization change
> I'm not clear about that. Would you like to provide more details? We will check the impact on our patch.
I forget the exact context now sorry. If you've rebased to latest code
and everything builds and runs then that should suffice.
I have a lot of general concerns about the impact of this work on
various areas of the JVM. It really needs to be as unobtrusive as
possible and ideally causing no changes to executed code unless enabled.
Potentially/possibly it might even need to be selectable at build-time,
as to whether this feature is included.
And I apologise in advance because I don't have a lot of time to deep
dive into all the details of this proposed feature.
Thanks,
David
-----
> Thanks,
> Kuai Wei
>
>
>
>
> ------------------------------------------------------------------
> From:David Holmes <david.holmes at oracle.com>
> Send Time:2019年6月10日(星期一) 15:18
> To:yumin qi <yumin.qi at gmail.com>; hotspot-runtim. <hotspot-runtime-dev at openjdk.java.net>
> Cc:hotspot-dev <hotspot-dev at openjdk.java.net>
> Subject:Re: RFC: JWarmup precompile java hot methods at application startup
>
> Hi Yumin,
>
> On 8/06/2019 3:25 am, yumin qi wrote:
>> Hi, David and all
>>
>> Can I have one more comment from runtime expert for the JEP?
>> David, can you comment for the changes? Really appreciate your last
>> comment. It is best if you follow the comment.
>> Looking forward to having your comment.
>
> I still have a lot of trouble understanding the overall design here. The
> JEP is very high-level; the webrev is very low-level; and there's
> nothing in between to explain the details of the design - the kind of
> document you would produce for a design review/walkthrough. For example
> I can't see why you need to record the "source file"; I can't see why
> you need to make changes to the superclass resolution. I can't tell when
> changes outside of Jwarmup may need to make changes to the Jwarmup code
> - the dependencies are unclear. I'm unclear on the role of the "dummy
> method" - is it just a sentinel? Why do we need it versus using some
> state in the JitWarmup instance?
>
> Some further code comments, but not a file by file review by any means ...
>
> The code split between the JDK and JVM doesn't seem quite right to me.
> registerNatives is something usually done by the JDK .c files
> corresponding to the classes defining the native method; it's not
> something done in jvm.cpp. Or if this is meant to be a special case like
> JVM_RegisterMethodHandleMethods then probably it should be in the
> jwarmup.cpp file. Also if you pass the necessary objects through the API
> you won't need to jump back to native to call a JNI function.
>
> AliasedLoggingFlags are for converting legacy flags to unified logging.
> You should just be using UL and not introducing the
> PrintCompilationWarmUpDetail psuedo-flag.
>
> This work introduces thirteen new VM flags! That's very excessive.
> Perhaps you should look at defining something more like -Xlog that
> encodes all the options? (And this will need a very lengthy CSR request!).
>
> The init.cpp code should be factored out into jwarmup_init functions in
> jwarmup.cpp.
>
> Mutex initialization should be conditional on jwarmup being enabled.
>
> Deoptimization has been changed lately to avoid use of safepoints so you
> may need to re-examine that aspect.
>
> You have a number of uses of patterns like this (but not everywhere):
>
> + JitWarmUp* jwp = JitWarmUp::instance();
> + assert(jwp != NULL, "sanity check");
> + jwp->preloader()->jvm_booted_is_done();
>
> The assertion should be inside instance() so that these collapse to a
> single line:
>
> JitWarmup::instance()->preloader->whatever();
>
> Your Java files have @version 1.8.
>
> ---
>
> Cheers,
> David
> -----
>
>
>
>> Thanks
>> Yumin
>>
>> On Sun, May 19, 2019 at 10:28 AM yumin qi <yumin.qi at gmail.com
>> <mailto:yumin.qi at gmail.com>> wrote:
>>
>> Hi, Tobias and all
>> Have done changes based on Tobias' comments. New webrev based on
>> most recent base is updated at:
>> http://cr.openjdk.java.net/~minqi/8220692/webrev-03/
>>
>> Tested local for jwarmup and compiler.
>>
>> Thanks
>> Yumin
>>
>> On Tue, May 14, 2019 at 11:26 AM yumin qi <yumin.qi at gmail.com
>> <mailto:yumin.qi at gmail.com>> wrote:
>>
>> HI, Tobias
>>
>> Thanks very much for the comments.
>>
>> On Mon, May 13, 2019 at 2:58 AM Tobias Hartmann
>> <tobias.hartmann at oracle.com <mailto:tobias.hartmann at oracle.com>>
>> wrote:
>>
>> Hi Yumin,
>>
>> > In this version, the profiled method data is not used at
>> > precomilation, it will be addressed in followed bug fix.
>> After the
>> > first version integrated, will file bug for it.
>>
>> Why is that? I think it would be good to have everything in
>> one JEP.
>>
>>
>> We have done some tests on adding profiling data and found the
>> result is not as expected, and the current version is working
>> well for internal online applications. There is no other reason
>> not adding to this patch now, we will like to study further to
>> see if we can improve that for a better performance.
>>
>> I've looked at the compiler related changes. Here are some
>> comments/questions.
>>
>> ciMethod.cpp
>> - So CompilationWarmUp is not using any profile information?
>> Not even the profile obtained in the
>> current execution?
>>
>>
>> Yes. This is also related to previous question.
>>
>> compile.cpp
>> - line 748: Why is that required? Couldn't it happen that a
>> method is never compiled because the
>> code that would resolve a field is never executed?
>>
>>
>> Here a very aggressive decision --- to avoid compilation failure
>> requires that all fields have already been resolved.
>>
>> graphKit.cpp
>> - line 2832: please add a comment
>> - line 2917: checks should be merged into one if and please
>> add a comment
>>
>>
>> Will fix it.
>>
>> jvm.cpp
>> - Could you explain why it's guaranteed that warmup
>> compilation is completed once the dummy method
>> is compiled? And why is it hardcoded to print
>> "com.alibaba.jwarmup.JWarmUp"?
>>
>>
>> This is from practical testing of real applications. Due to the
>> parallelism of compilation works, it should check if
>> compilation queue contains any of those methods --- completed if
>> no any of them on the queue and it is not economic. By using of
>> a dummy method as a simplified version for that, in real case,
>> it is not observed that dummy method is not the last compilation
>> for warmup. Do you have suggestion of a way to do that? The
>> dummy way is not strictly a guaranteed one theoretically.
>> Forgot to change the print to new name after renaming package,
>> thanks for the catching.
>>
>> - What is test_symbol_matcher() used for?
>>
>>
>> This is a leftover(used to test matching patterns), will remove
>> it from the file.
>>
>> jitWarmUp.cpp:
>> - line 146: So what about methods that are only ever
>> compiled at C1 level? Wouldn't it make sense to
>> keep track of the comp_level during CompilationWarmUpRecording?
>>
>>
>> Will consider your suggestion in future work on it.
>>
>> I also found several typos while reading through the code
>> (listed in random order):
>>
>> globals.hpp
>> - "flushing profling" -> "flushing profiling"
>>
>> method.hpp
>> - "when this method first been invoked"
>>
>> templateInterpreterGenerator_x86.cpp
>> - initializition -> initialization
>>
>> dict.cpp
>> - initializated -> initialized
>>
>> jitWarmUp.cpp
>> - uninitilaized -> uninitialized
>> - inited -> should be initialized, right?
>>
>> jitWarmUp.hpp
>> . nofityApplicationStartUpIsDone ->
>> notifyApplicationStartUpIsDone
>>
>> constantPool.cpp
>> - recusive -> recursive
>>
>> JWarmUp.java
>> - appliacation -> application
>>
>> TestThrowInitializaitonException.java ->
>> TestThrowInitializationException.java
>>
>> These tests should be renamed (it's not clear what issue the
>> number refers to):
>> - issue9780156.sh
>> - Issue11272598.java
>>
>>
>> Will fix all above suggestions.
>>
>> Thanks!
>>
>> Yumin
>>
>
More information about the hotspot-runtime-dev
mailing list