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.
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.)
--
Jean Delvare
SUSE L3 Support
