RFC: JWarmup precompile java hot methods at application startup

[yumin qi]

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.

Thanks
Yumin

On Sun, May 19, 2019 at 10:28 AM yumin qi <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> 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> 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
>>
>