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

Fri Mar 4 11:29:57 UTC 2016

Great! I'm happy we finally came to an agreement :)

Best regards,
Volker

On Fri, Mar 4, 2016 at 11:07 AM, Nils Eliasson <nils.eliasson at oracle.com>
wrote:

> Hi Volker,
>
> On 2016-03-03 16:25, Volker Simonis wrote:
>
> Hi Nils,
>
> thanks for your comments. Please find my new webrev here:
>
> http://cr.openjdk.java.net/~simonis/webrevs/2016/8150646_hotspot.v4
> http://cr.openjdk.java.net/~simonis/webrevs/2016/8150646_toplevel.v4
>
>
> Looks very good now.
>
>
> Comments as always inline:
>
> On Thu, Mar 3, 2016 at 1:47 PM, Nils Eliasson < <nils.eliasson at oracle.com>
> nils.eliasson at oracle.com> wrote:
>
>> Hi Volker,
>>
>> On 2016-03-02 17:36, Volker Simonis wrote:
>>
>> Hi Nils,
>>
>> your last webrev (jdk.03 and hotspot.05)) looks pretty good! Ive used is
>> as base for my new webrevs at:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8150646_hotspot.v3
>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8150646_toplevel.v3
>>
>> I've updated the copyrights, added the current reviewers and also added
>> us both in the Contributed-by line (hope that's fine for you).
>>
>>
>> Absolutely
>>
>>
>> Except that, I've only done the following minor fixes/changes:
>>
>> * compileBroker.{cpp,hpp}*
>>
>> - we don't need CompileBroker::is_compile_blocking() anymore.
>>
>> Good
>>
>>
>> *compilerDirectives.hpp*
>>
>> - I think we should use
>> cflags(BackgroundCompilation,   bool, BackgroundCompilation,
>> BackgroundCompilation)
>> instead of:
>> cflags(BackgroundCompilation,   bool, BackgroundCompilation, X)
>>
>> so we can also trigger blocking compiles from the command line with a
>> CompileCommand (e.g.
>> -XX:CompileCommand="option,java.lang.String::charAt,bool,BackgroundCompilation,false")
>> That's very handy during development or and also for simple tests where we
>> don't want to mess with compiler directives. (And the overhead to keep this
>> feature is quite small, just "BackgroundCompilation" instead of "X" ;-)
>>
>>
>> Without a very strong use case for this I don't want it as a
>> CompileCommand.
>>
>> CompileCommand options do have a cost - they force a temporary unique
>> copy of the directive if any option command matches negating some of the
>> positive effects of directives. Also the CompileCommands are stringly
>> typed, no compile time name or type check is done. This can be fixed in
>> various ways, but until then I prefer not to add it.
>>
>>
> Well, daily working is a strong use case for me:) Until there's no
> possibility to provide compiler directives directly on the command line
> instead of using an extra file, I think there's a justification for the
> CompileCommand version. Also I think the cost argument is not so relevent,
> because the feature will be mainly used during developemnt or in small
> tests which don't need compiler directives. It will actually make it
> possible to write simple tests which require blocking compilations without
> the need to use compiler directives or WB (just by specifying a
> -XX:CompileCommand option).
>
>
> ok, you convinced me.
>
>
>> *whitebox.cpp*
>>
>> I think it is good that you fixed the state but I think it is too
>> complicated now. We don't need to strdup the string and can easily forget
>> to free 'tmpstr' :) So maybe it is simpler to just do another transition
>> for parsing the directive:
>>
>>   {
>>     ThreadInVMfromNative ttvfn(thread); // back to VM
>>     DirectivesParser::parse_string(dir, tty);
>>   }
>>
>>
>> Transitions are not free, but on the other hand the string may be long.
>> This is not a hot path in anyway so lets go with simple.
>>
>>
>>
>> *advancedThresholdPolicy.cpp *
>> - the JVMCI code looks reasonable (although I haven't tested JVMCI) and
>> is actually even an improvement over my code which just picked the first
>> blocking compilation.
>>
>>
>> Feels good to remove the special cases.
>>
>>
>>
>>
>> *diagnosticCommand.cpp *- Shouldn't you also fix
>> CompilerDirectivesAddDCmd to return the number of added directives and
>> CompilerDirectivesRemoveDCmd to take the number of directives you want to
>> pop? Or do you want to do this in a later, follow-up change?
>>
>>
>> Yes, lets do that in a follow up change. They affect a number of tests.
>>
>>
>> *WhiteBox.java*
>>
>> - I still think it would make sense to keep the two 'blocking' versions
>> of  enqueueMethodForCompilation() for convenience. For example your test
>> fix for JDK-8073793 would be much simpler if you used them. I've added two
>> comments to the 'blocking' convenience methods to mention the fact that
>> calling them may shadow previously added compiler directives.
>>
>>
>> I am ok with having then, but think Whitebox.java will get too bloated. I
>> would rather have the convenience-methods in some test utility class, like
>> CompilerWhiteBoxTest.java.
>>
>>
> OK, I can live with that. I removed the blocking enqueue methods and the
> corresponding tests.
>
>>
>> *BlockingCompilation.java*
>>
>> - I've extended my regression test to test both methods of doing blocking
>> compilation - with the new, 'blocking' enqueueMethodForCompilation()
>> methods as well as by manually setting the corresponding compiler
>> directives. If we should finally get consensus on removing the blocking
>> convenience methods, please just remove the corresponding tests.
>>
>>
>> Line 85:        for (level = 1; level <= 4; level++) {
>>
>> You can not be sure all compilation levels are available. Use
>>
>>  * @library /testlibrary /test/lib /
>>  * @build sun.hotspot.WhiteBox
>>  *        compiler.testlibrary.CompilerUtils
>>
>> import compiler.testlibrary.CompilerUtils;
>>
>> int[] levels = CompilerUtils.getAvailableCompilationLevels();
>>         for (int level : levels) {
>> ...
>>
>
> Good catch. I've slightly revorked the test. I do bail out early if there
> are no compilers at all and I've also fixed the break condition of the loop
> which is calling foo() to compare against the highest available compilation
> level instead of just using '4'.
>
>>
>> I think we're close to a final version now, what do you think :)
>>
>>
>> Yes! I'll take a look as soon as you post an updated webrev.
>>
>
> Would be good if you could run it trough JPRT once so we can be sure we
> didn't break anything.
>
>
> I am running all jtreg tests on some select platforms right now.
>
> Best regards,
> Nils
>
>
> Regards,
> Volker
>
>
>>
>> Regards,
>> Nils
>>
>>
>>
>> Regards,
>> Volker
>>
>>
>> On Wed, Mar 2, 2016 at 2:37 PM, Nils Eliasson <
>> <nils.eliasson at oracle.com>nils.eliasson at oracle.com> wrote:
>>
>>> 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> <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_toplevelhttp://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> <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/20160304/18b72120/attachment-0001.html>