On 7/27/22 17:29, Tao Zhou wrote:
>> +/*
>> + * Handle event for implicit monitor: da_get_monitor_##name() will figure out
>> + * the monitor.
>> + */
>> +#define DECLARE_DA_MON_MONITOR_HANDLER_IMPLICIT(name, type) \
>> + \
>> +static inline void __da_handle_event_##name(struct da_monitor *da_mon, \
>> + enum events_##name event) \
>> +{ \
>> + int retval; \
>> + \
>> + retval = da_monitor_handling_event_##name(da_mon); \
>> + if (!retval) \
>> + return; \
> I checked the callers of __da_handle_event_##name():
> da_handle_event_##name() for all cases need the above check.
> da_handle_start_event_##name() for all cases may not need this check.
> (this function checked the enable first and the da_monitoring later and if
> it is not monitoring it will start monitoring and return, the later event
> handler will not be called. Otherwise enable is enabled, da_monitoring is
> monitoring)
> da_handle_start_run_event_##name() for implicit case may not need this check.
> (almost the same with the above, the difference is if da-monitor is not
> monitoring, it will start monitoring and not return and do the event handler,
> here enable is enabled and da_monitoring is monitoring, if I am not wrong)
> So after another(v7) looking at this patch, I realized that this check can
> be omited in two cases(all three cases). Just in fuction da_handle_event_##name()
> we need to do da_monitor_handling_event_##name().
> So I'd write like this:
> static inline void __da_handle_event_##name(struct da_monitor *da_mon, \
> enum events_##name event) \
> { \
> int retval; \
> \
> retval = da_event_##name(da_mon, event); \
> if (!retval) \
> da_monitor_reset_##name(da_mon); \
> } \
>
> static inline void da_handle_event_##name(enum events_##name event) \
> { \
> struct da_monitor *da_mon = da_get_monitor_##name(); \
> int retval; \
> \
> retval = da_monitor_handling_event_##name(da_mon); \
> if (!retval) \
> return; \
> \
> __da_handle_event_##name(da_mon, event); \
>
> } \
>
IOW,
The code is checking twice if the monitor is enabled in these two cases:
- da_handle_start_run_event_##name()
- da_handle_start_event_##name()
Because it is checking in these functions first and then again in __da_handle_event_##name().
The function da_handle_event_##name() is not checking if the monitors are enabled because
__da_handle_event_##name() does it.
By adding the check on da_handle_event_##name(), we can remove it in
__da_handle_event_##name(). Making the check run only once for all cases.
This is an optimization, and it makes sense.
(changed return value to bool)
-- Daniel
|