[pacman-dev] Holy inefficient batman!

[Nagy Gabor]

> On Sat, Nov 17, 2007 at 07:25:03PM +0100, Xavier wrote:
> > And I put a debug back because Aaron asked for it, and I also think it can
> be
> > very handy sometimes, to debug quicker.
> > 
> > > So I did some speed test, and the results was really suprising:
> > > First of all, I kept only deps.c.diff from our patches (my typo doesn't
> matter 
> > > here). And I did "pactest.py -v" on smoke001.py (empty disk cache,
> repeated 
> > > many times times etc.)
> > > The result: your patch is _significantly_ slower (I got 5,4,4,4, ... sec)
> than 
> > > mine (I got 4,3,3,3, ... sec). I repeated the test 4 times (patch pacman,
> 
> > > compile, 5 pactest); the result is always the same!
> > > Could you test this, too?
> > > 
> > 
> > Hm yes, I'll do more tests.
> 
> Well, I get 9.9s with your patch, and 10.8s with mine.
> I removed the logging (the 3 lines) in depcmp and I get 10.0s.
> 
> But what is one second saved in the runtime of smoke001 compared to the time
> this debug log could save you when debugging? :)
> 
> Maybe things can still be improved though. What is costly is apparently the
> alpm_dep_get_string call. If I remove it, but leave malloc / free and put a
> fake depstring, I still get nearly 10.0s.
> So I looked at that function for things I could change, it seems it's
> sprintf
> which is not very efficient. I replaced it by manual strncpy (so the code
> gets uglier), and I get ~10.0s again.
> 
> I'm not sure this is all worth the troubles though.
> 
> 
> diff --git a/lib/libalpm/deps.c b/lib/libalpm/deps.c
> index 9232984..31493f7 100644
> --- a/lib/libalpm/deps.c
> +++ b/lib/libalpm/deps.c
> @@ -830,8 +830,8 @@ const char SYMEXPORT *alpm_dep_get_version(const
> pmdepend_t *dep)
>   */
>  char SYMEXPORT *alpm_dep_get_string(const pmdepend_t *dep)
>  {
> -	char *opr, *str = NULL;
> -	size_t len;
> +	char *opr, *str = NULL, *ptr;
> +	size_t namelen, oprlen, verlen;
>  
>  	ALPM_LOG_FUNC;
>  
> @@ -858,9 +858,16 @@ char SYMEXPORT *alpm_dep_get_string(const pmdepend_t
> *dep)
>  
>  	/* we can always compute len and print the string like this because opr
>  	 * and ver will be empty when PM_DEP_MOD_ANY is the depend type */
> -	len = strlen(dep->name) + strlen(opr) + strlen(dep->version) + 1;
> -	MALLOC(str, len, RET_ERR(PM_ERR_MEMORY, NULL));
> -	snprintf(str, len, "%s%s%s", dep->name, opr, dep->version);
> +	namelen = strlen(dep->name);
> +	oprlen = strlen(opr);
> +	verlen =  strlen(dep->version);
> +	MALLOC(str, namelen + oprlen + verlen + 1, RET_ERR(PM_ERR_MEMORY, NULL));
> +	ptr = str;
> +	strncpy(ptr, dep->name, namelen);
> +	ptr += namelen;
> +	strncpy(ptr, opr, oprlen);
> +	ptr += oprlen;
> +	strncpy(ptr, dep->version, verlen + 1);
>  
>  	return(str);
>  }
1. I was curious and I tested smoke001.py with "requiredby-pacman" and as it was
expected by me (no compute-requiredby is needed here), the current version is
much faster (3 or 4 sec vs. 7 sec)
2. There is something in my mind about alpm_dep_getstring:
It shouldn't be used for debug preformat (or in libalpm at all); but that is
good for front-ends.
I would prefer the following more elegant(?) solution instead of switch():
there is a global const char *deptypestr[] = {">=", "<=", "="} in the correct
order somewhere; and from now on, you can pass deptypestr[dep->mod] parameter
easily to alpm_log for example (see alpm_depcmp). This is O(1) (<- switch is
also O(1), but that floods the code imho)
I will create a patch for this, after I can sync my git repo ;-)
Bye


----------------------------------------------------
SZTE Egyetemi Könyvtár - http://www.bibl.u-szeged.hu
This mail sent through IMP: http://horde.org/imp/