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

[Pavel Punegov]

Hi Volker,

I have some comments and questions about your patch:

- src/share/vm/runtime/advancedThresholdPolicy.cpp

You check for background compilation  (blocking) by the searching for an appropriate directive. 
But there is a CompileTask::is_blocking() method, that returns a value set in CompilerBroker::compile_method_base when a compile task was created. It seems that CompileBroker::is_compile_blocking() finds the right directive and checks for BackgroundCompilation for being set.

I think that checking it twice could lead to an issue with different directives being set on the stack. With diagnostic commands I can clear the directives stack, or remove directives. If I do this in between the task was submitted and being checked  in AdvancedThresholdPolicy::select_task, this task could became non blocking.

- src/share/vm/compiler/compileBroker.cpp

1317   backgroundCompilation = directive->BackgroundCompilationOption;

Does it check the BackgroundCompilation for being set for both c1 and c2 at the same time? What will happen if I set BackgroundCompilation to c1 only?
AFAIK, there are different queues for c1 and c2, and hence we could have BackgroundCompilation to be set separately for both compilers.

- test/lib/sun/hotspot/WhiteBox.java
 <>
318       addCompilerDirective("[{ match: … 

I’m not quite sure that this is a right way to set a method to be blocking. Adding a directive on top of the stack makes already set directives for that method not used.
For example, if I would like to set method to be logged (LogCompilation) and disable some inlining, but then enqueue it with WB, I will get it to be only compiled without LogCompilation.
But, AFAIK, setting CompileCommand option will work for already set directive through a compatibility in CompilerDirectives.

So, I would prefer to have a directive (file or WB) or an option set by myself, and then invoke standard WB.enqueueMethodForCompilation().

— Thanks,
Pavel Punegov

> On 26 Feb 2016, at 21:47, Volker Simonis <volker.simonis at gmail.com> 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).
> 
> 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/20160229/ffc8b675/attachment-0001.html>