RewriteFrequentPairs flag was disabled in debug model(and discussion for fix)

[Daniel D. Daugherty]

Resending to include the Serviceability team since they own JVM/TI...

Dan


On 1/26/15 11:53 PM, San Hong Li wrote:
> HI All:
> When the JVM is running in debug model,   the RewriteFrequentPairs
> optimization will be disabled by following code  in
>   JvmtiManageCapabilities::update():
>
> *  if (avail.can_generate_breakpoint_events) {*
> *    RewriteFrequentPairs = false;*
> *  }*
>
> *[ pls. check the above code  @Line 328 in jvmtiManageCapabilities.cpp  in
> jdk7 codebase]*
>
> I understand the reason why the  RewriteFrequentPairs doesn't work in debug
> model is that the TOS cache will be disturbed after the execution of
> TemplateTable::aload_0  in such this case,
>
> Let us discuss it in more details about what will happen if we enable
> RewriteFrequentPairs  flag in debugging model...
>
>
> The following code was  excerpted from  TemplateTable::aload_0
> ( @L789 in templateTable_x86_64.cpp).
> For better discussion, I just skipped irrelevant parts:
>
> *void TemplateTable::aload_0() {*
> *  transition(vtos, atos);*
>
>
> *  if (RewriteFrequentPairs) {*
> *      ......  *
> *      // do actual aload_0*
> *      aload(0);                     *
> *      ......*
> *      // rewrite*
> *      // bc: fast bytecode*
> *      __ bind(rewrite);*
> *      patch_bytecode(Bytecodes::_aload_0, bc, rbx, false);*
> *      __ bind(done);*
> *  } else {*
> *    aload(0);*
> *  }*
> *}*
>
> In above code:
> The *aload(0)  *happened before * patch_bytecode. *
> The TOS cache will be set correctly after *aload(0) *, that's, the  state
> should be atos as we have expected.
>
> But the cache will be corrupted by the following *patch_bytecode
> *which actually
>   calls out to InterpreterRuntime::set_original_bytecode_at:
>
> Pls. check the implementation around at L256 in  templateTable_x86_64.cpp:
>
> *    // Let breakpoint table handling rewrite to quicker bytecode*
> *    __ call_VM(noreg, CAST_FROM_FN_PTR(address,
> InterpreterRuntime::set_original_bytecode_at), temp_reg, r13, bc_reg);*
>
> The problem  here is:
> The call out into InterpreterRuntime::set_original_bytecode_at will
> eventually change the values cached in RAX.
>
> I would like to discuss and  propose a fix for this issue:  just do
> the * aload(0)
> *after *patch_bytecode!*
> (actually similar  thing we did in TemplateTable::iload) ,
> So the updated code with fix likes this:
>
> *void TemplateTable::aload_0() {*
> *  transition(vtos, atos);*
>
>
> *  if (RewriteFrequentPairs) {*
> *      ......  *
> *      // do actual aload_0*
> *      //aload(0);                     *
> *      ......*
> *      // rewrite*
> *      // bc: fast bytecode*
> *      __ bind(rewrite);*
> *      patch_bytecode(Bytecodes::_aload_0, bc, rbx, false);*
> *      __ bind(done);*
> *  }  *
> *   // do actual aload_0*
> *    __ movptr(rax, aaddress(0));    *
> *}*
>
> So that the TOS has been cached correctly without disruption by
> *patch_bytecode,
> *safe use for next bytecode.
>
> Finally, just a summary for my questions:
>
> Whether or not *this is the only case t*hat explained why the current
> implementation of HotSpot  has to disable RewriteFrequentPairs flag in
> debug mode?
>      --- If so,  how do u think about the fix?  I did some tests in my
> environment, it works fine.
>      --- if not,  would you pls. point out in which case we also have to
> disable RewriteFrequentPairs  during debugging?
>
> Thanks!
> San Hong