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

Thu Mar 3 15:25:17 UTC 2016

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

Comments as always inline:

On Thu, Mar 3, 2016 at 1:47 PM, Nils Eliasson <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).

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

Regards,
Volker

>
> Regards,
> Nils
>
>
>
> Regards,
> Volker
>
>
> On Wed, Mar 2, 2016 at 2:37 PM, Nils Eliasson <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/%7Eneliasso/8073793/webrev.03/>
>> 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/20160303/3c6bacf4/attachment-0001.html>