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

Wed Mar 2 13:37:22 UTC 2016

Yes, I forgot to add the fix for working with multiple directives from 
whitebox.

WB.addCompilerDirectives now returns the number of directives that where 
added, and removeCompilerDirectives takes a parameter for the number of 
directives that should be popped (atomically).

http://cr.openjdk.java.net/~neliasso/8150646/webrev_jdk.03/
http://cr.openjdk.java.net/~neliasso/8150646/webrev.05/

Fixed test in JDK-8073793 to work with this: 
http://cr.openjdk.java.net/~neliasso/8073793/webrev.03/

Best regards,
Nils Eliasson

On 2016-03-02 13:36, Nils Eliasson wrote:
> 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/dd9834e0/attachment.html>