Re: [PATCH v3 4/8] pwm: lpss: Include headers we are direct user of

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Tue, Sep 27, 2022 at 05:55:21PM +0200, Uwe Kleine-König wrote:
> On Tue, Sep 27, 2022 at 06:26:28PM +0300, Andy Shevchenko wrote:
> > On Tue, Sep 27, 2022 at 05:10:53PM +0200, Uwe Kleine-König wrote:
> > > On Tue, Sep 27, 2022 at 05:47:19PM +0300, Andy Shevchenko wrote:
> > > > For the sake of integrity, include headers we are direct user of.
> > > > 
> > > > While at it, add missed struct pwm_lpss_boardinfo one and replace
> > > > device.h with a forward declaration. The latter improves compile
> > > > time due to reducing overhead of device.h parsing with entire train
> > > > of dependencies.
> > > 
> > > Hm, I copied the cmdline for the compiler from a V=1 build and only run
> > > the compiler on drivers/pwm/pwm-lpss-pci.c.
> > > 
> > > With #include <device.h> I got:
> > > 
> > > 	real	0m0.421s
> > > 	user	0m0.354s
> > > 	sys	0m0.066s
> > > 
> > > With struct device; I got:
> > > 
> > > 	real	0m0.431s
> > > 	user	0m0.378s
> > > 	sys	0m0.052s
> > > 
> > > Are the numbers for you considerably different?
> > 
> > Why Ingo created thousands of patches to do something similar? Because for
> > a single user you won't see a big difference, but when amount of small pieces
> > are gathered together, you definitely will.
> 
> My doubt is that for me the effect of using struct device over #include
> <device.h> is even negative (looking at real and user). Is it sys which
> counts in the end?

The main time required for the header inclusion is mmap():ing it (that's what
I believe preprocessor does) and parsing. In any case, testing on a high-speed
machine one file case is not correct (scientific) approach. Of course with
your measurements will be in the error range.

As I have learnt at university the very first question for the experiment
we should ask ourselves is "what exactly have we measured?"

I'm not sure any continuation of this makes sense if we haven't answered
to this. When I measured preprocessor speed up (not in this case, though),
I used ccache tool, because it makes more clear the time spend for C
preprocessor (by caching the compiler results), so 10 runs of that maybe
closer to the real world (hot cache which should have no effect on the
iteration-to-iteration time).

That said, if you want to NAK this, please do it explicitly. I'm not going
to waste my time on this simple change anymore.

> > > > +struct device;

...

> > > > +struct pwm_lpss_boardinfo;
> > > 
> > > Hmm, I wonder why there is no compiler warning without that declaration.
> > > At least in my builds. Do you see a warning? IMHO it's better to fix
> > > that be swapping the order of struct pwm_lpss_chip and struct
> > > pwm_lpss_boardinfo.
> > 
> > Have I told about warning?
> 
> No, it's just me who expected there would be a warning if a pointer to
> struct pwm_lpss_boardinfo is used before struct pwm_lpss_boardinfo is
> defined (or declared).
> 
> Anyhow, I stand by my opinion that swapping the order of struct
> pwm_lpss_chip and struct pwm_lpss_boardinfo is a saner fix.

I agree on this. But I expect a NAK, so simplest to me is to drop this patch
from the series.

> > It's a proper C programming style.
> > You don't have a warning because all pointers are considered to be the same,
> > but it is better style to explicitly point that out.

-- 
With Best Regards,
Andy Shevchenko