On 09.08.2021 15:33, Jean Delvare wrote:
> Hi Heiner,
>
> On Fri, 6 Aug 2021 22:49:00 +0200, Heiner Kallweit wrote:
>> On 05.08.2021 16:23, Jean Delvare wrote:
>>> On Sun, 01 Aug 2021 16:23:34 +0200, Heiner Kallweit wrote:
>>>> + if (dmi_match(DMI_PRODUCT_NAME, dell_lis3lv02d_devices[i].dmi_product_name)) {
>>>
>>> This causes dmi_get_system_info(DMI_PRODUCT_NAME) to be called for
>>> every iteration of the loop, slowing down the lookup. It's an exported
>>> function so it can't be inlined by the compiler. I know this happens
>>> only once, but we try to keep boot times as short as possible.
>>>
>> I'm aware of this. However we just talk about a small in-memory operation and
>> the performance impact should be neglectable. dmi_get_system_info() is just
>> the following:
>>
>> const char *dmi_get_system_info(int field)
>> {
>> return dmi_ident[field];
>> }
>> EXPORT_SYMBOL(dmi_get_system_info);
>>
>> I'd rate the simpler and better maintainable code higher.
>> But that's just a personal opinion and mileage may vary.
>
> I'm not worried about multiple calls to dmi_get_system_info(), which is
> indeed simple and inlined anyway, but about multiple calls to dmi_match
> (which can't be inlined). Function calls have a high cost (which is the
> very reason why the compiler will try to inline functions whenever
> possible).
>
> I wouldn't mind if you were replacing several lines of code,
> but here you are only removing one local variable and one simple line
> of code, or 15 bytes of binary code total. But you add up to 8 function
> calls, and that number could grow in the future as we add support for
> more devices. That's why I say the benefit of the change is
> questionable.
>
This code is called only if is_dell_system_with_lis3lv02d() returns true,
what further reduces the impact of what you mention. But sure, the preference
is a question of personal taste. Let me know whether you have any other
review comments regarding v2 of the series, then I'll drop this change in v3.
> If it was new code, I probably wouldn't mind. But when changing
> existing code, I need to be convinced that the new code is
> unquestionably better than what we had before. That's not the case here.
>
> (And don't get me wrong, I would love to live in a world where you don't
> have do choose between best performance and and systematic use of
> existing APIs. Alas, we often have to make choices in either direction.)
>
