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

[Vladimir Kozlov]

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
>>>
>