<div dir="ltr"><div><div>Hi Nils,<br><br></div><div>your last webrev (jdk.03 and hotspot.05)) looks pretty good! Ive used is as base for my new webrevs at:<br><br><a href="http://cr.openjdk.java.net/~simonis/webrevs/2016/8150646_hotspot.v3">http://cr.openjdk.java.net/~simonis/webrevs/2016/8150646_hotspot.v3</a><br><a href="http://cr.openjdk.java.net/~simonis/webrevs/2016/8150646_toplevel.v3">http://cr.openjdk.java.net/~simonis/webrevs/2016/8150646_toplevel.v3</a><br><br></div><div>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).<br><br></div><div>Except that, I've only done the following minor fixes/changes:<br></div><div><b><br>compileBroker.{cpp,hpp}</b><br><br>- we don't need CompileBroker::is_compile_blocking() anymore.<br><br><b>compilerDirectives.hpp</b><br><br>- I think we should use<br>cflags(BackgroundCompilation,   bool, BackgroundCompilation, BackgroundCompilation)<br>instead of:<br>cflags(BackgroundCompilation,   bool, BackgroundCompilation, X)<br><br>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" ;-)<br><br></div><b>whitebox.cpp</b><br><br></div>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:<br><br>  {<br>    ThreadInVMfromNative ttvfn(thread); // back to VM<br>    DirectivesParser::parse_string(dir, tty);<br>  }<br><br><div><div><b>advancedThresholdPolicy.cpp<br></b><br></div><div>- 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.<br></div><div><br><b>diagnosticCommand.cpp<br><br></b>- 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?<br><br><b>WhiteBox.java</b><br><br></div><div>- 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.<br><br><b>BlockingCompilation.java</b><br><br></div><div>- 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.<br><br></div><div>I think we're close to a final version now, what do you think :)<br><br></div><div>Regards,<br></div><div>Volker<br><br></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 2, 2016 at 2:37 PM, Nils Eliasson <span dir="ltr"><<a href="mailto:nils.eliasson@oracle.com" target="_blank">nils.eliasson@oracle.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  
    
  
  <div text="#000000" bgcolor="#FFFFFF">
    Yes, I forgot to add the fix for working with multiple directives
    from whitebox.<br>
    <br>
    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).<br>
    <br>
    <a href="http://cr.openjdk.java.net/~neliasso/8150646/webrev_jdk.03/" target="_blank">http://cr.openjdk.java.net/~neliasso/8150646/webrev_jdk.03/</a><br>
    <a href="http://cr.openjdk.java.net/~neliasso/8150646/webrev.05/" target="_blank">http://cr.openjdk.java.net/~neliasso/8150646/webrev.05/</a><br>
    <br>
    Fixed test in JDK-8073793 to work with this:
    <a href="http://cr.openjdk.java.net/~neliasso/8073793/webrev.03/" target="_blank">http://cr.openjdk.java.net/~neliasso/8073793/webrev.03/</a><br>
    <br>
    Best regards,<br>
    Nils Eliasson<div><div class="h5"><br>
    <br>
    <br>
    <div>On 2016-03-02 13:36, Nils Eliasson
      wrote:<br>
    </div>
    <blockquote type="cite">
      
      Hi Volker,<br>
      <br>
      I created these webrevs including all the feedback from everyone:<br>
      <br>
      <a href="http://cr.openjdk.java.net/%7Eneliasso/8150646/webrev_jdk.02/" target="_blank">http://cr.openjdk.java.net/~neliasso/8150646/webrev_jdk.02/</a><br>
      * Only add- and removeCompilerDirective<br>
      <br>
      <a href="http://cr.openjdk.java.net/%7Eneliasso/8150646/webrev.04/" target="_blank">http://cr.openjdk.java.net/~neliasso/8150646/webrev.04/</a><br>
      * whitebox.cpp <br>
      -- addCompilerDirective to have correct VM states<br>
      * advancedThresholdPolicy.cpp <br>
      -- prevent blocking tasks from becoming stale<br>
      -- The logic for picking first blocking task broke JVMCI code.
      Instead made the JVMCI code default (select the blocking task with
      highest score.) <br>
      * compilerDirectives.hpp<br>
      -- Remove option CompileCommand. Not needed.<br>
      
      * compileBroker.cpp<br>
      -- Wrapped compile_method so that directive get and release always
      are matched.<br>
      <br>
      
      Is anything missing?<br>
      <br>
      Best regards,<br>
      Nils Eliasson<br>
      <br>
      <br>
      <div>On 2016-03-01 19:31, Volker Simonis
        wrote:<br>
      </div>
      <blockquote type="cite">
        <pre>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
<a href="mailto:vladimir.kozlov@oracle.com" target="_blank"><vladimir.kozlov@oracle.com></a> wrote:
</pre>
        <blockquote type="cite">
          <pre>Nils, please answer Pavel's questions.

Thanks,
Vladimir


On 3/1/16 6:24 AM, Nils Eliasson wrote:
</pre>
          <blockquote type="cite">
            <pre>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: <a href="http://cr.openjdk.java.net/%7Eneliasso/8150646/webrev.03/" target="_blank">http://cr.openjdk.java.net/~neliasso/8150646/webrev.03/</a>

The code runs fine with the test I fixed for JDK-8073793:
<a href="http://cr.openjdk.java.net/%7Eneliasso/8073793/webrev.02/" target="_blank">http://cr.openjdk.java.net/~neliasso/8073793/webrev.02/</a>

Best regards,
Nils Eliasson

On 2016-02-26 19:47, Volker Simonis wrote:
</pre>
            <blockquote type="cite">
              <pre>Hi,

so I want to propose the following solution for this problem:

<a href="http://cr.openjdk.java.net/%7Esimonis/webrevs/2016/8150646_toplevel" target="_blank">http://cr.openjdk.java.net/~simonis/webrevs/2016/8150646_toplevel</a>
<a href="http://cr.openjdk.java.net/%7Esimonis/webrevs/2016/8150646_hotspot/" target="_blank">http://cr.openjdk.java.net/~simonis/webrevs/2016/8150646_hotspot/</a>

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
</pre>
            </blockquote>
            <pre>On Fri, Feb 26, 2016 at 9:36 AM, Nils Eliasson
<a href="mailto:nils.eliasson@oracle.com" target="_blank"><nils.eliasson@oracle.com></a> wrote:
</pre>
            <blockquote type="cite">
              <blockquote type="cite">
                <pre>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:
</pre>
                <blockquote type="cite">
                  <pre>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:
</pre>
                  <blockquote type="cite">
                    <pre>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: <a href="https://bugs.openjdk.java.net/browse/JDK-8150646" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8150646</a>
JDK rev: <a href="http://cr.openjdk.java.net/%7Eneliasso/8150646/webrev_jdk.01/" target="_blank">http://cr.openjdk.java.net/~neliasso/8150646/webrev_jdk.01/</a>
Hotspot rev: <a href="http://cr.openjdk.java.net/%7Eneliasso/8150646/webrev.02/" target="_blank">http://cr.openjdk.java.net/~neliasso/8150646/webrev.02/</a>

Best regards,
Nils Eliasson
</pre>
                  </blockquote>
                </blockquote>
                <pre></pre>
              </blockquote>
            </blockquote>
          </blockquote>
        </blockquote>
      </blockquote>
      <br>
    </blockquote>
    <br>
  </div></div></div>

</blockquote></div><br></div>