RFR(S/M): 8150646: Add support for blocking compiles through whitebox API
Nils Eliasson
nils.eliasson at oracle.com
Fri Mar 4 10:07:30 UTC 2016
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/%7Esimonis/webrevs/2016/8150646_hotspot.v4>
> http://cr.openjdk.java.net/~simonis/webrevs/2016/8150646_toplevel.v4
> <http://cr.openjdk.java.net/%7Esimonis/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 <mailto: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/%7Esimonis/webrevs/2016/8150646_hotspot.v3>
>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8150646_toplevel.v3
>> <http://cr.openjdk.java.net/%7Esimonis/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 <mailto: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/%7Eneliasso/8150646/webrev_jdk.03/>
>> http://cr.openjdk.java.net/~neliasso/8150646/webrev.05/
>> <http://cr.openjdk.java.net/%7Eneliasso/8150646/webrev.05/>
>>
>> Fixed test in JDK-8073793 to work with this:
>> http://cr.openjdk.java.net/~neliasso/8073793/webrev.03/
>> <http://cr.openjdk.java.net/%7Eneliasso/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/
>>> <http://cr.openjdk.java.net/%7Eneliasso/8150646/webrev_jdk.02/>
>>> * Only add- and removeCompilerDirective
>>>
>>> http://cr.openjdk.java.net/~neliasso/8150646/webrev.04/
>>> <http://cr.openjdk.java.net/%7Eneliasso/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>
>>>> <mailto: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/
>>>>>> <http://cr.openjdk.java.net/%7Eneliasso/8150646/webrev.03/>
>>>>>>
>>>>>> The code runs fine with the test I fixed for JDK-8073793:
>>>>>> http://cr.openjdk.java.net/~neliasso/8073793/webrev.02/
>>>>>> <http://cr.openjdk.java.net/%7Eneliasso/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/%7Esimonis/webrevs/2016/8150646_toplevel>
>>>>>>> http://cr.openjdk.java.net/~simonis/webrevs/2016/8150646_hotspot/
>>>>>>> <http://cr.openjdk.java.net/%7Esimonis/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> <mailto: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/
>>>>>>>>>> <http://cr.openjdk.java.net/%7Eneliasso/8150646/webrev_jdk.01/>
>>>>>>>>>> Hotspot rev:http://cr.openjdk.java.net/~neliasso/8150646/webrev.02/
>>>>>>>>>> <http://cr.openjdk.java.net/%7Eneliasso/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/20160304/e8a7920d/attachment-0001.html>
More information about the hotspot-compiler-dev
mailing list