Denis Zaitsev writes:
> Ok, I'm sorry for such a delay...
>
> On Tue, Feb 19, 2002 at 11:40:20AM -0700, Richard Gooch wrote:
> > Denis Zaitsev writes:
> > > This little patch does nothing with the functionality of devfsd, but
> > > with the C code. There are a number of constructions like:
> > >
> > > [PFXLEN = strlen(prefix);]
> > > if (strncmp(str, prefix, PFXLEN) == 0)
> > > do_something_with(str + PFXLEN);
> > >
> > > It is not the best way to do such a things. The idea is to implement
> > > the special function, which will test the string for some prefix and
> > > return the address of a place of the string after that prefix or NULL
> > > in case of an absence of the success. The construction above becomes
> > > better:
> > >
> > > if (ptr= strtry(str, prefix))
> > > do_something_with(ptr);
> > >
> > > And the new function itself is more lightweight than alone strncmp,
> > > and just much more effective than <strlen + strncmp> in a couple. It
> > > is good again. So, all the idea seems to be healthy. I call this
> > > function "strtry", as it tries its arg for the given prefix.
> >
> > Apart from not really liking this approach, you've made the strtry()
> > function inlined. Any saving you might make with removing code from
> > the callers is probably more than lost due to all the extra inlined
> > code.
> >
> > Did you compare the sizes of the stripped binary to see what the
> > effect of your patch is?
> >
>
> Yes, of course, I did. The new binary is 672 bytes smaller...) But
> it's not the answer for your question, because I compile devfsd with
> all the string functions inlined.
Inlining is generally a bad idea. It bloats the object code and
increases the icache footprint. Inlining should only ever be done in a
critical path, where you're trying to save cycles. Nothing in devfsd
is that critical.
> And the difference includes all the throwned out strrchr's calls.
> And I haven't done any "pure" measurements. But I think, they
> aren't really needed.
I prefer hard numbers to speculation, particularly when justifying a
change.
> strtry should be inlined, because it is very small - something like
> 14 inline bytes on x86, plus, say 5 bytes for loading the prefix's
> addr into the reg.
> At the same time, the outlined call for strncmp takes:
> call itself - 5 bytes,
> stack cleanup - 3 bytes,
> stack loading - something like 2 + 1 + 6 bytes
> - i.e. it's generally just equal for the inlined strtry. So, inlined
> strtry is better than outlined strncmp even without the cons of a
> inline size... And strtry's cycle is more effective, than strncmp's
> one, but I've already written about. And when strncmp is used in the
> couple with strlen, than strtry is more than twice faster, as it scans
> the prefix only once vs. twice. And strtry's stuff looks good in C
> (as for me, of course). So, that are the reasons, why strtry is "a
> good choice"...
>
> > > Richard, please, apply this patch, if you find it useful. It is
> > > against devfsd-1.3.22. By the way, I've arranged the
> > > strrchr (devname, '/') + 1
> > > stuff, so this thing to be done once instead of multiply times in the
> > > original.
> >
> > But you've inserted calls to strrchr in cases where it's not really
> > needed.
>
> Yes, I agree, I was thinking about this... Probably, just another
> special function will be healthy here, as that fragment of code is
> repeated often. If you don't reject the whole idea about strtry, I
> will remove these changes from the patch.
Well, like I said earlier, I'm not enthusiastic about strtry(). It
looks like there is little if no benefit, and it adds another
non-standard function that requires the reader to check the API.
Everyone understands strncmp(). I don't plan on using strtry() unless
I see some real benefit.
Regards,
Richard....
Permanent: rgooch@xxxxxxxxxxxxx
Current: rgooch@xxxxxxxxxxxxxxx
|