Re: [PATCH v13 04/10] accel/tcg: add jit stats and time to TBStatistics

[Alex Bennée]

"Wu, Fei" <fei2.wu@intel.com> writes:

> On 5/30/2023 1:01 PM, Wu, Fei wrote:
>> On 5/30/2023 12:07 PM, Richard Henderson wrote:
>>> On 5/29/23 04:49, Fei Wu wrote:
>>>> +/*
>>>> + * The TCGProfile structure holds data for analysing the quality of
>>>> + * the code generation. The data is split between stuff that is valid
>>>> + * for the lifetime of a single translation and things that are valid
>>>> + * for the lifetime of the translator. As the former is reset for each
>>>> + * new translation so it should be copied elsewhere if you want to
>>>> + * keep it.
>>>> + *
>>>> + * The structure is safe to access within the context of translation
>>>> + * but accessing the data from elsewhere should be done with safe
>>>> + * work.
>>>> + */
<snip>
>>>> +    int64_t cpu_exec_time;
>>>> +    int64_t op_count; /* total insn count */
>>>> +    int64_t code_in_len;
>>>> +    int64_t code_out_len;
>>>> +    int64_t search_out_len;
>>>> +
>>>> +    /* Timestamps during translation  */
>>>> +    uint64_t gen_start_time;
>>>> +    uint64_t gen_ir_done_time;
>>>> +    uint64_t gen_opt_done_time;
>>>> +    uint64_t gen_la_done_time;
>>>> +    uint64_t gen_code_done_time;
>>>> +
>>>> +    /* Lifetime count of TCGOps per TCGContext */
>>>> +    uint64_t table_op_count[NB_OPS];
>>>> +} TCGProfile;
>>>> +
>>>
>>> Why have you added this back?
>>>
>>> The whole point of removing CONFIG_PROFILE to begin was to have all new
>>> code.  Not to remove it then reintroduce it unchanged.
>>>
>>> In tcg_gen_code, you have access to the TranslationBlock as s->gen_tb. 
>>> There is zero point to accumulating into TCGProfile, when you could be
>>> accumulating into TCGStatistics directly.
>>>
>> TCGProfile contains global wide (per TCGContext) stats, but TBStatistics
>> is TB specific, some info in TCGProfile such as table_op_count is not
>> able to be summed up from TBStatistics. The per-translation stats in
>> TCGProfile may be removed indeed.
>> 
> After some cleanup locally, these are the remains in TCGProfile:
> * cpu_exec_time - which is not guarded by tb_stats_enabled, it could be
> moved out as an individual variable?
> * gen_xxx_time - which in kinda global variables across functions to
> calc ts->gen_times

Given the work on JIT profiling I think there is an argument to drop the
time profile bits and pieces. I think you can get the same information
from a perf run although it does amortise the cost of generation over
all translations. Do we see any particular TBs that are particularly
expensive to translate by more than a standard deviation?

> * table_op_count - it's indeed guarded by tb_stats_enabled, but it's a
> large array, it might be too large to put into TBStaticstics.

Probably. This is probably more interesting information as an aggregate
than per TB.

>
> Thanks,
> Fei.
>
>> Thanks,
>> Fei.
>> 
>>>
>>> r~
>> 


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro