RFR(S/M): 8150646: Add support for blocking compiles through whitebox API
Pavel Punegov
pavel.punegov at oracle.com
Wed Mar 9 12:19:33 UTC 2016
Thanks for your explanation, Volker. Changes are good.
— Pavel.
> On 09 Mar 2016, at 11:14, Nils Eliasson <nils.eliasson at oracle.com> wrote:
>
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20160309/d1559f16/attachment-0001.html>
More information about the hotspot-compiler-dev
mailing list