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

Tue Mar 1 20:04:15 UTC 2016

Hi,

On 2016-02-29 20:01, Pavel Punegov wrote:
> 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.
Yes, CompileTask::is_blocking() should be used instead of looking up the 
directive again.
>
> 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.
The correct directive set is retrieved in the beginning of the 
compilation when getMatchingDirective(target_method, target_compiler) is 
called. So this will work perfectly even with different flags for 
dfferent 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().

I agree that the enqueueMethod with the block-argument might cause 
undesirable surprises and that it is better to just have the plain 
methods addCompilerDirective, enqueueMethod and removeCompilerDirective 
in Whitebox.java. If we find some often used pattern we can add that to 
CompilerWhitebox or similar.

There is one bug here though - addCompilerDirective adds any number of 
directives, but removeCompilerDirective just removes one. I can do a 
quick fix that limits the WB-api to just add one at a time.

Thanks for the feedback,
Nils Eliasson

>
> — Thanks,
> Pavel Punegov
>
>> On 26 Feb 2016, at 21:47, Volker Simonis <volker.simonis at gmail.com 
>> <mailto: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/%7Esimonis/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/20160301/516be732/attachment.html>