RFR(S/M): 8150646: Add support for blocking compiles through whitebox API

Wed Mar 2 12:36:23 UTC 2016

Hi Volker,

I created these webrevs including all the feedback from everyone:

http://cr.openjdk.java.net/~neliasso/8150646/webrev_jdk.02/
* Only add- and removeCompilerDirective

http://cr.openjdk.java.net/~neliasso/8150646/webrev.04/
* whitebox.cpp
-- addCompilerDirective to have correct VM states
* advancedThresholdPolicy.cpp
-- prevent blocking tasks from becoming stale
-- The logic for picking first blocking task broke JVMCI code. Instead 
made the JVMCI code default (select the blocking task with highest score.)
* compilerDirectives.hpp
-- Remove option CompileCommand. Not needed.
* compileBroker.cpp
-- Wrapped compile_method so that directive get and release always are 
matched.

Is anything missing?

Best regards,
Nils Eliasson

On 2016-03-01 19:31, Volker Simonis wrote:
> Hi Pavel, Nils, Vladimir,
>
> sorry, but I was busy the last days so I couldn't answer your mails.
>
> Thanks a lot for your input and your suggestions. I'll look into this
> tomorrow and hopefully I'll be able to address all your concerns.
>
> Regards,
> Volker
>
>
> On Tue, Mar 1, 2016 at 6:24 PM, Vladimir Kozlov
> <vladimir.kozlov at oracle.com> wrote:
>> Nils, please answer Pavel's questions.
>>
>> Thanks,
>> Vladimir
>>
>>
>> On 3/1/16 6:24 AM, Nils Eliasson wrote:
>>> Hi Volker,
>>>
>>> An excellent proposition. This is how it should be used.
>>>
>>> I polished a few rough edges:
>>> * CompilerBroker.cpp - The directives was already access in
>>> compile_method - but hidden incompilation_is_prohibited. I moved it out
>>> so we only have a single directive access. Wrapped compile_method to
>>> make sure the release of the directive doesn't get lost.
>>> * Let WB_AddCompilerDirective return a bool for success. Also fixed the
>>> state - need to be in native to get string, but then need to be in VM
>>> when parsing directive.
>>>
>>> And some comments:
>>> * I am against adding new compile option commands (At least until the
>>> stringly typeness is fixed). Lets add good ways too use compiler
>>> directives instead.
>>>
>>> I need to look at the stale task removal code tomorrow - hopefully we
>>> could save the blocking info in the task so we don't need to access the
>>> directive in the policy.
>>>
>>> All in here:
>>> Webrev: http://cr.openjdk.java.net/~neliasso/8150646/webrev.03/
>>>
>>> The code runs fine with the test I fixed for JDK-8073793:
>>> http://cr.openjdk.java.net/~neliasso/8073793/webrev.02/
>>>
>>> Best regards,
>>> Nils Eliasson
>>>
>>> On 2016-02-26 19:47, Volker Simonis wrote:
>>>> Hi,
>>>>
>>>> so I want to propose the following solution for this problem:
>>>>
>>>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8150646_toplevel
>>>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8150646_hotspot/
>>>>
>>>> I've started from the opposite site and made the BackgroundCompilation
>>>> manageable through the compiler directives framework. Once this works
>>>> (and it's actually trivial due to the nice design of the
>>>> CompilerDirectives framework :), we get the possibility to set the
>>>> BackgroundCompilation option on a per method base on the command line
>>>> via the CompileCommand option for free:
>>>>
>>>>
>>>> -XX:CompileCommand="option,java.lang.String::charAt,bool,BackgroundCompilation,false"
>>>>
>>>>
>>>> And of course we can also use it directly as a compiler directive:
>>>>
>>>> [{ match: "java.lang.String::charAt", BackgroundCompilation: false }]
>>>>
>>>> It also becomes possible to use this directly from the Whitebox API
>>>> through the DiagnosticCommand.compilerDirectivesAdd command.
>>>> Unfortunately, this command takes a file with compiler directives as
>>>> argument. I think this would be overkill in this context. So because
>>>> it was so easy and convenient, I added the following two new Whitebox
>>>> methods:
>>>>
>>>>     public native void addCompilerDirective(String compDirect);
>>>>     public native void removeCompilerDirective();
>>>>
>>>> which can now be used to set arbitrary CompilerDirective command
>>>> directly from within the WhiteBox API. (The implementation of these
>>>> two methods is trivial as you can see in whitebox.cpp).
>>>> v
>>>> The blocking versions of enqueueMethodForCompilation() now become
>>>> simple wrappers around the existing methods without the need of any
>>>> code changes in their native implementation. This is good, because it
>>>> keeps the WhiteBox API stable!
>>>>
>>>> Finally some words about the implementation of the per-method
>>>> BackgroundCompilation functionality. It actually only requires two
>>>> small changes:
>>>>
>>>> 1. extending CompileBroker::is_compile_blocking() to take the method
>>>> and compilation level as arguments and use them to query the
>>>> DirectivesStack for the corresponding BackgroundCompilation value.
>>>>
>>>> 2. changing AdvancedThresholdPolicy::select_task() such that it
>>>> prefers blocking compilations. This is not only necessary, because it
>>>> decreases the time we have to wait for a blocking compilation, but
>>>> also because it prevents blocking compiles from getting stale. This
>>>> could otherwise easily happen in AdvancedThresholdPolicy::is_stale()
>>>> for methods which only get artificially compiled during a test because
>>>> their invocations counters are usually too small.
>>>>
>>>> There's still a small probability that a blocking compilation will be
>>>> not blocking. This can happen if a method for which we request the
>>>> blocking compilation is already in the compilation queue (see the
>>>> check 'compilation_is_in_queue(method)' in
>>>> CompileBroker::compile_method_base()). In testing scenarios this will
>>>> rarely happen because methods which are manually compiled shouldn't
>>>> get called that many times to implicitly place them into the compile
>>>> queue. But we can even completely avoid this problem by using
>>>> WB.isMethodQueuedForCompilation() to make sure that a method is not in
>>>> the queue before we request a blocking compilation.
>>>>
>>>> I've also added a small regression test to demonstrate and verify the
>>>> new functionality.
>>>>
>>>> Regards,
>>>> Volker
>>> On Fri, Feb 26, 2016 at 9:36 AM, Nils Eliasson
>>> <nils.eliasson at oracle.com> wrote:
>>>>> Hi Vladimir,
>>>>>
>>>>> WhiteBox::compilation_locked is a global state that temporarily stops
>>>>> all
>>>>> compilations. I this case I just want to achieve blocking compilation
>>>>> for a
>>>>> single compile without affecting the rest of the system. The tests
>>>>> using it
>>>>> will continue executing as soon as that compile is finished, saving time
>>>>> where wait-loops is used today. It adds nice determinism to tests.
>>>>>
>>>>> Best regards,
>>>>> Nils Eliasson
>>>>>
>>>>>
>>>>> On 2016-02-25 22:14, Vladimir Kozlov wrote:
>>>>>> You are adding parameter which is used only for testing.
>>>>>> Can we have callback(or check field) into WB instead? Similar to
>>>>>> WhiteBox::compilation_locked.
>>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>> On 2/25/16 7:01 AM, Nils Eliasson wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review this change that adds support for blocking compiles
>>>>>>> in the
>>>>>>> whitebox API. This enables simpler less time consuming tests.
>>>>>>>
>>>>>>> Motivation:
>>>>>>> * -XX:-BackgroundCompilation is a global flag and can be time
>>>>>>> consuming
>>>>>>> * Blocking compiles removes the need for waiting on the compile
>>>>>>> queue to
>>>>>>> complete
>>>>>>> * Compiles put in the queue may be evicted if the queue grows to big -
>>>>>>> causing indeterminism in the test
>>>>>>> * Less VM-flags allows for more tests in the same VM
>>>>>>>
>>>>>>> Testing:
>>>>>>> Posting a separate RFR for test fix that uses this change. They
>>>>>>> will be
>>>>>>> pushed at the same time.
>>>>>>>
>>>>>>> RFE: https://bugs.openjdk.java.net/browse/JDK-8150646
>>>>>>> JDK rev: http://cr.openjdk.java.net/~neliasso/8150646/webrev_jdk.01/
>>>>>>> Hotspot rev: http://cr.openjdk.java.net/~neliasso/8150646/webrev.02/
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Nils Eliasson
>>>>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20160302/767607f5/attachment.html>