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

Wed Mar 2 13:37:37 UTC 2016

Hi Pavel, Nils,

thanks for your input. Please find my comments inline.

I've also prepared a new webrev which includes your suggestions (and
the new regression test which was missing from the first webrev):

http://cr.openjdk.java.net/~simonis/webrevs/2016/8150646_hotspot.v2/
http://cr.openjdk.java.net/~simonis/webrevs/2016/8150646_toplevel.v2/

On Tue, Mar 1, 2016 at 9:04 PM, Nils Eliasson <nils.eliasson at oracle.com> wrote:
> 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.
>

Done. That nicely simplifys the code!

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

As Nils already wrote, this already works. I was a little unspecific
in WhiteBox.java where I set the blocking option for both compilers.
In my new version I set it only for the compiler which matches the
actual compilation level. But I'm not sure if this is really relevant
in practice.

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

Yes, you're right. And as you correctly noticed, you could still use
CompileCommand to set an option which will not be shadowed by a new
compiler directive.

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

But you can still do that. I've just added the 'blocking' versions of
the enqueueMethod() for convenience because I thought that doing a
blocking compile without additional compiler directives is quite
common for tests.

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