RFR(M): 8031752: Failed speculative optimization should be reattempted when root of compilation is different
Vladimir Kozlov
vladimir.kozlov at oracle.com
Tue Jan 21 15:25:31 PST 2014
On 1/20/14 2:28 AM, Roland Westrelin wrote:
> Here is a new webrev with Chris’ suggestions:
>
> http://cr.openjdk.java.net/~roland/8031752/webrev.02/
>
> Vladimir, I didn’t include your suggestion because as you mention trap_recompiled_at() needs the method argument.
Okay. Looks fine.
Vladimir
>
> Roland.
>
> On Jan 17, 2014, at 6:09 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>
>> Sorry, I was not clear. I suggested to move next reason check
>>
>> + ciMethod* m = Deoptimization::reason_is_speculate(reason) ? this->method() : NULL;
>> + if (md->has_trap_at(bci, m, reason) != 0) {
>>
>> inside has_trap_at():
>>
>> int has_trap_at(int bci, ciMethod* m, int reason) {
>> if (!Deoptimization::reason_is_speculate(reason)) {
>> m = NULL;
>> }
>> return has_trap_at(bci_to_data(bci, m), reason);
>> }
>>
>> and path method always:
>>
>> + if (md->has_trap_at(bci, this->method(), reason) != 0) {
>>
>> The are 2 places where you do that. But I see that in the second case 'm' is also passed to trap_recompiled_at(). So my suggestion may be not good. IT is up to you.
>>
>> Thanks,
>> Vladimir
>>
>> On 1/17/14 5:03 AM, Roland Westrelin wrote:
>>> Hi Vladimir,
>>>
>>> Thanks for taking a loot at this.
>>>
>>>> Can you pass method to has_trap_at() and check the reason inside instead of current assert?
>>>
>>> Sorry, but I don’t understand what change you’d like to see.
>>> Is this the assert you’re talking about:
>>>
>>> int has_trap_at(int bci, ciMethod* m, int reason) {
>>> assert((m != NULL) == Deoptimization::reason_is_speculate(reason), "inconsistent method/reason");
>>> return has_trap_at(bci_to_data(bci, m), reason);
>>> }
>>>
>>> Roland.
>>>
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 1/16/14 8:32 AM, Roland Westrelin wrote:
>>>>> Thanks for reviewing this, Chris.
>>>>>
>>>>> Here is a new webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~roland/8031752/webrev.01/
>>>>>
>>>>> And comments below:
>>>>>
>>>>>> 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?
>>>>>
>>>>> I want the speculative traps to be listed together with the byte code they affect. Diagnosing a missed optimization is much easier that way. Here is an example output:
>>>>>
>>>>> 6 ldc_w TestTrapSpeculate$B
>>>>> 9 if_acmpne 19
>>>>> 64 bci: 9 BranchData trap(class_check recompiled) trap/ TestTrapSpeculate::m2(class_check recompiled) trap/ TestTrapSpeculate::m1(class_check recompiled)
>>>>> not taken(10)
>>>>> 12 fast_aload_0
>>>>>
>>>>> But the speculative traps are not stored with the ProfileData for each byte code. So I need to pass something to each ProfileData::print_data_on() that can be used to print the speculative traps. I decided to pass a formatted string with the trap stuff. Which is then passed to ProfileData::print_shared() where the per byte code line is printed.
>>>>>
>>>>>> 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).
>>>>>
>>>>> Ok.
>>>>>
>>>>>>
>>>>>> 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.
>>>>>
>>>>> Would needs_speculative_traps be better?
>>>>>
>>>>>>
>>>>>> + 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.
>>>>>
>>>>> Ok. So I tried to use switches when iterating over the extra data everywhere. The problem is that break, breaks out of the switch but not out of the loop anymore. The new MethodData::next_extra() doesn’t know how to move past a arg_info_data_tag because it’s a static method. It’s a static method because it’s used by ciMethodData as well. So I need to break out of the loop before calling the last MethodData::next_extra(). Anyway, I refactored the code so that the loops are in their own methods and then I can return from them to jump out of the switch and loop.
>>>>>
>>>>>>
>>>>>> 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)
>>>>>>
>>>>>> ?
>>>>>>
>>>>>
>>>>> Ok.
>>>>>
>>>>>> 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?
>>>>>
>>>>> At this point, it’s unclear whether those values are going to be good enough so I think we need to be able to adjust the parameters in benchmark runs.
>>>>>
>>>>>> 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 :-)
>>>>>
>>>>> Indeed.
>>>>>
>>>>> Roland.
>>>>>
>>>>>>
>>>>>> 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.
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>
>
More information about the hotspot-compiler-dev
mailing list