RFR(M): 8031752: Failed speculative optimization should be reattempted when root of compilation is different

[Christian Thalinger]

All these print_data_on changes are a pain:

!   virtual void print_data_on(outputStream* st, const char* extra) const {

I see you actually pass something in only in one place.  How does the output look like?  Can’t you print the “extra” output in ProfileData::print_data_on directly after the other output?

src/share/vm/oops/methodData.cpp:

+   case Bytecodes::_invokestatic:
+     return UseTypeSpeculation;
+   }
+   return false;

Could you move the return false into a default case of the switch?  At some point I would like to enable the compiler warnings about switches not having all cases (-Wswitch).

    int empty_bc_count = 0;  // number of bytecodes lacking data
+   bool has_spec_bc = false;

That name is hard to understand for people new to the code.  At least the first ugly name has a comment.

+ DataLayout* MethodData::next_extra(DataLayout* dp) {
+   int nb_cells = 0;
+   if (dp->tag() == DataLayout::bit_data_tag || dp->tag() == DataLayout::no_tag) {
+     nb_cells = BitData::static_cell_count();
+   } else if (dp->tag() == DataLayout::speculative_trap_data_tag) {
+     nb_cells = SpeculativeTrapData::static_cell_count();
+   } else {
+     fatal(err_msg("unexpected tag %d", dp->tag()));
+   }

It would be easier to read with a switch instead of if-else.  Maybe also in MethodData::print_data_on.  The assert:

        assert(dp->tag() == DataLayout::arg_info_data_tag, "must be BitData or ArgInfo”);

is now out-of-date and the code could have a fatal instead.  Same in MethodData::clean_method_data.

src/share/vm/opto/compile.cpp:

!   if (md->has_trap_at(bci, Deoptimization::reason_is_speculate(reason) ? this->method() : NULL, reason) != 0) {

Can you factor this to a local variable as you do later:

+   ciMethod* m = Deoptimization::reason_is_speculate(reason) ? this->method() : NULL;
    if ((per_bc_reason == Deoptimization::Reason_none
!        || md->has_trap_at(bci, m, reason) != 0)

?

src/share/vm/runtime/globals.hpp:

+   product(intx, PerMethodSpecTrapLimit,  5000,                              \
+           "Limit on speculative traps (of one kind) in a method (includes inlines)") \

+   product(intx, SpecTrapLimitExtraEntries,  3,                              \
+           "Extra method data trap entries for speculation")                 \

Do we need these new flags to be product flags?

src/share/vm/ci/ciMethodData.cpp:

! void ciParametersTypeData::print_data_on(outputStream* st, const char* extra) const {
!   st->print_cr("Parametertypes");
!   st->print_cr("ciParameterTypesData”);

Typo replaced with a typo :-)

On Jan 15, 2014, at 3:19 AM, Roland Westrelin <roland.westrelin at oracle.com> wrote:

> http://cr.openjdk.java.net/~roland/8031752/webrev.00/
> 
> When C2 optimizes the code based on type profiling, it emits a guard. If the guard fails, the code "traps" and the location of the trap (method, bci) and cause (class check) are recorded. When C2 generates code for that method again, it doesn't perform any optimization of that type (class check) at that location (bci). 
> 
> If, say, the trap occurs at (m, bci) when m is inlined in m1. When m2 that inlines m is compiled, no class check optimization is performed at (m, bci) because of the trap triggered in m1. With type speculation, type information is flowing from many profiling points in the application code. So in the example above, the profile data used at (m, bci) may come from m1 when the compiled method is m1 and it may be come from m2 when the compiled method is m2. And so the trap at (m,bci) in compiled method m1 doesn't say much about the validity of a speculative optimization at (m,bci) when the compiled method is m2. 
> 
> When a trap occurs from a speculative optimization, the trap should record: (method, bci, compiled method) so that in the example above: erroneous optimization at (m, bci) is not reattempted if m1 is re-compiled but is attempted when m2 is compiled.
> 
> With nashorn, peak performance varies a lot from one run to another for some benchmarks. This is one of the causes. Depending on the order of compilations, what’s compiled or not, what’s inlined or not, some optimizations may or may not be performed.
> 
> This change adds a new type of traps (SpeculativeTrapData). They record the nmethod’s method where the trap occurs. They are allocated in the extra data space of the MDO. If no more space is available in the extra data space, the VM fallbacks to standard traps.
> 
> I also added a PerMethodSpecTrapLimit similar to PerMethodTrapLimit but for speculative traps. Its value is much higher. That appears to be required as well to have reproducible peak performance results from one run to another.
> 
> I have a couple more changes that help reduce performance variation with nashorn and that I will send for review later.
> 
> Roland.
> 
>