Re: [PATCH 2/6] d3dx9: Use versioned parameter updates instead of 'dirty' flags.

[Matteo Bruni]

2017-05-12 14:24 GMT+02:00 Paul Gofman :
> Looping through shared parameters in set_dirty() which is removed by this patch
> has a major performance impact on parameter update for applications creating a
> lot of effects and sharing parameters between them, which is a common case.
> Using versioned scheme also allows for some further optimization.
>
> Signed-off-by: Paul Gofman

I don't have serious issues with this patch but I also have a number
of questions so I don't feel like signing it off either. Sorry :/
> @@ -163,6 +163,7 @@ struct d3dx_const_tab
>      unsigned int const_set_size;
>      struct d3dx_const_param_eval_output *const_set;
>      const enum pres_reg_tables *regset2table;
> - ULONG64 update_version;
>
> };

Is it necessary? Could you do without it, using the pass version instead?
> struct d3dx_shared_data;
>
> @@ -213,6 +214,8 @@ struct d3dx_parameter
>      UINT bytes;
>      DWORD runtime_flags;
>      DWORD object_id;
> - ULONG64 update_version;
> - ULONG64 *update_version_counter;

I don't like the "update_version_counter" name that much but I don't
have particularly good suggestions. pass_update_version?
current_update_version? parent_update_version? new_update_version?
> +static inline ULONG64 get_update_version_counter(ULONG64 *update_version_counter)
> +{
> - return ++*update_version_counter;
>
> +}

Maybe it's just me but it's a bit surprising to see this get_x()
function having side effects. Not sure how to call it.
next_update_version()? inc_update_version_counter()?
> +static inline BOOL is_param_dirty(struct d3dx_parameter *param, ULONG64 update_version)
> +{
> - struct d3dx_shared_data *shared_data;
> -
> - if ((shared_data = param->top_level_param->u.shared_data))
> -
>    return update_version < shared_data->update_version;
> - else
> -
>    return update_version < param->top_level_param->update_version;
>
> +}

Did you try if a unified update_version pointer looks any nicer?
Especially considering that you have a more or less matching
*update_version_counter at the moment.
> HRESULT d3dx_evaluate_parameter(struct d3dx_param_eval *peval,
> -
>    const struct d3dx_parameter *param, void *param_value, BOOL update_all) DECLSPEC_HIDDEN;
> -
>    const struct d3dx_parameter *param, void *param_value) DECLSPEC_HIDDEN;

I guess this has to do with the update_version in struct
d3dx_const_tab. I might be missing something, in that case please clue
me.
> +static ULONG64 get_effect_update_version_counter(struct d3dx9_base_effect *base)
> +{
> - return get_update_version_counter(get_update_version_counter_ptr(base));
>
> }

Same naming caveat here.
> -static void clear_dirty_params(struct d3dx9_base_effect *base)
> +static void set_dirty(struct d3dx_parameter *param)
>  {
> - unsigned int i;
> - struct d3dx_shared_data *shared_data;
> - struct d3dx_parameter *top_param = param->top_level_param;
> - ULONG64 new_update_version = get_update_version_counter(top_param->update_version_counter);
> - for (i = 0; i < base->parameter_count; ++i)
> -
>    base->parameters[i].runtime_flags &= ~PARAMETER_FLAG_DIRTY;
> - if ((shared_data = top_param->u.shared_data))
> -
>    shared_data->update_version = new_update_version;
> - else
> -
>    top_param->update_version = new_update_version;
>
> }

Another option would be NOT to increment the effect / pool counter on
each set_dirty() but instead only increment it in BeginPass() and
CommitChanges(), after you're done with the update (or well, it's fine
at almost any point given that we don't have to care about races). It
seems a bit better to me, unless I'm overlooking something and it
won't work.
> static HRESULT set_string(char **param_data, const char *string)
> @@ -3078,6 +3074,7 @@ static HRESULT d3dx9_apply_pass_states(struct ID3DXEffectImpl *effect, struct d3
>      unsigned int i;
>      HRESULT ret;
>      HRESULT hr;
> -
>
> ULONG64 new_update_version = get_effect_update_version_counter(&effect->base_effect);
>
> TRACE("effect %p, pass %p, state_count %u.\n", effect, pass, pass->state_count);
>
> @@ -3109,7 +3106,7 @@ static HRESULT d3dx9_apply_pass_states(struct ID3DXEffectImpl *effect, struct d3
>      }
>      effect->material_updated = FALSE;
> - clear_dirty_params(&effect->base_effect);
> - pass->update_version = new_update_version;
> return ret;
>
> }

Right, here is where I'd update the counter.