RFC: JWarmup precompile java hot methods at application startup

[David Holmes]

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
>