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

Nils Eliasson nils.eliasson at oracle.com
Wed Mar 9 08:14:04 UTC 2016 

Hi Volker,

On 2016-03-09 09:02, Volker Simonis wrote:
> 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?

I have run all hotspot jtreg tests on linux x64 and x86 without problem. 
I have also run the test with -Xcomp without problem. -Xbatch is not a 
part of flag rotation. Now I am just waiting for Pavels ok.

Regards,
Nils

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

More information about the hotspot-compiler-dev mailing list