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

Wed Mar 9 08:02:33 UTC 2016

On Fri, Mar 4, 2016 at 7:30 PM, Volker Simonis <volker.simonis at gmail.com> wrote:
> Hi Pavel,
>
> thanks for your feedabck. Please find my comments inline:
>
> On Fri, Mar 4, 2016 at 6:01 PM, Pavel Punegov <pavel.punegov at oracle.com>
> wrote:
>>
>> Hi Volker,
>>
>> overall looks good to me (not Reviewer).
>>
>> Just some questions about the test:
>> 1. Do you need to PrintCompilation and Inlining in the test?
>>
>>   39  *        -XX:+PrintCompilation
>>   40  *
>> -XX:CompileCommand=option,BlockingCompilation::foo,PrintInlining
>
> It's not necessary for the test, but if the test will fail it will be good
> to have this information in the .jtr file
>
>>
>> 2. 500_000 for a loop seems to be a way to much.
>> There is a test/compiler/whitebox/CompilerWhiteBoxTest.java that has a
>> constants enough for a compilation.
>>
>
> That's just an upper bound which won't be reached. We break out of the loop
> once we reached the maximum compilation level. Notice that the loop count is
> not the count until the method get's enqueued for compilation, but until it
> actually gets compiled because this loop tests the non-blocking
> compilations. So if the machine is loaded and/or the compile  queue is busy,
> it can take quite some iterations until the method will be finally compiled
> (in my manual tests it usually took not more than 8000 iterations until the
> test left the loop).
>
>>
>> 3. Running Client compiler only could pass even if compilation were
>> blocking here:
>>
>>  128     if (level == 4 && iteration == i) {
>>
>> I think it should check that any level changing have happened with some
>> amount of iterations, not only the 4th level.
>
>
> The problem is that C1 compiles are so bleeding fast that I got to many
> false positives, i.e. the method got compiled in the same iteration just
> before I queried the compilation level.
>
> I actually begin to think that this test may always fail if you run the
> JTreg tests with "-Xbatch". Do you do that by default (e.g. in JPRT)? In
> that case we would probably have to additionally set
> "-XX:+BackgroundCompilation" in the test options?
>

I've forgot to say that I've tried the regression test by running
jtreg with -Xbatch and it still works because -vmoption/-javaoption
are not passed to a test which is run with '@run main/othervm'.

@Nils: what's the result of your testing? Is there anything left we
have to do until this can be pushed?

Regards,
Volker

> Regards,
> Volker
>
>
>>
>>
>> — Thanks,
>> Pavel Punegov
>>
>> On 04 Mar 2016, at 14:29, Volker Simonis <volker.simonis at gmail.com> wrote:
>>
>> 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>
>>> 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>
>>>> 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> 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
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>