FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > ArchLinux > ArchLinux Pacman Development

 
 
LinkBack Thread Tools
 
Old 05-14-2008, 10:21 PM
Xavier
 
Default The official pacman repository branch, master, updated. v3.1.4-163-gfb09d35

Nagy Gabor wrote:
>> commit f43805d875ad5c672afbbfff48bded2087204773
>> Author: Chantry Xavier<shiningxc@gmail.com>
>> Date: Sat May 10 18:47:42 2008 +0200
>>
>> Cleanup usages of alpm_list_find and alpm_list_remove.
>>
>> * remove obsolete and unused *_cmp helper functions like deppkg_cmp and
>> _alpm_grp_cmp
>>
>> * new alpm_list_remove_str function, used 6 times in handle.c
>>
>> * remove _alpm_prov_cmp / _alpm_db_whatprovides and replace them by
>> a more general alpm_find_pkg_satisfiers with a cleaner implementation.
>> before: alpm_db_whatprovides(db, targ)
>> after: alpm_find_pkg_satisfiers(alpm_db_getpkgcache(db), targ)
>
> Warning: pkg literal also satisfies pkg. But in most cases we called
> what_provides if we didn't find a literal.
>

I know, it's not exactly equivalent but I think it makes more sense that
way and as you noticed, it works the same for our use case.

>> * remove satisfycmp and replace alpm_list_find + satisfycmp usage by
>> _alpm_find_dep_satisfiers.
>> before : alpm_list_find(_alpm_db_get_pkgcache(db), dep, satisfycmp)
>> after : _alpm_find_dep_satisfiers(_alpm_db_get_pkgcache(db ), dep)
>
> Warning: possible slowdown, the old way just stopped after a satisfier (which
> is ideal in checkdeps), now we scan the whole db.
>

Right, I knew about that too, I just wanted to keep the code as clean as
possible and didn't find another way. Though it might be worth to do
some benchmarking / profiling. If it's really too bad, it will have to
change.

>>
>> * remove _alpm_pkgname_pkg_cmp, which was used with alpm_list_remove,
>> and
>> use _alpm_pkg_find + alpm_list_remove with _alpm_pkg_cmp instead.
>>
>
> Imho this is ugly. First we find it, then we again find it via list_remove.
>
>

Yeah, it's not ideal either. But neither are dummy pkg or fake
asymmetric cmp functions. I just preferred it like that.
Imo the real problem here is that our data structures suck and are
inefficient. Linear search ftw.


Anyway, other better suggestions for these 3 points are always welcome.

_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev
 
Old 05-18-2008, 12:08 PM
Xavier
 
Default The official pacman repository branch, master, updated. v3.1.4-163-gfb09d35

Nagy Gabor wrote:
>> Nagy Gabor wrote:
>>>> commit f43805d875ad5c672afbbfff48bded2087204773
>>>> Author: Chantry Xavier<shiningxc@gmail.com>
>>>> Date: Sat May 10 18:47:42 2008 +0200
>>>>
>>>> Cleanup usages of alpm_list_find and alpm_list_remove.
>>>>
>>>> * remove obsolete and unused *_cmp helper functions like deppkg_cmp
>> and
>>>> _alpm_grp_cmp
>>>>
>>>> * new alpm_list_remove_str function, used 6 times in handle.c
>>>>
>>>> * remove _alpm_prov_cmp / _alpm_db_whatprovides and replace them by
>>>> a more general alpm_find_pkg_satisfiers with a cleaner
>> implementation.
>>>> before: alpm_db_whatprovides(db, targ)
>>>> after: alpm_find_pkg_satisfiers(alpm_db_getpkgcache(db), targ)
>>> Warning: pkg literal also satisfies pkg. But in most cases we called
>>> what_provides if we didn't find a literal.
>>>
>
> Yes, there is no problem with this (I just emphasized it). I like the new
> function better because I think find_satisfiers is quite a common task. Btw, as
> I see, after this patch pacman -S 'kernel26>=2.6.24' automagically works, which
> is cool imho.
>

I am glad you liked at least one thing about my patch


>> I know, it's not exactly equivalent but I think it makes more sense that
>> way and as you noticed, it works the same for our use case.
>>
>>>> * remove satisfycmp and replace alpm_list_find + satisfycmp usage by
>>>> _alpm_find_dep_satisfiers.
>>>> before : alpm_list_find(_alpm_db_get_pkgcache(db), dep, satisfycmp)
>>>> after : _alpm_find_dep_satisfiers(_alpm_db_get_pkgcache(db ), dep)
>>> Warning: possible slowdown, the old way just stopped after a satisfier
>> (which
>>> is ideal in checkdeps), now we scan the whole db.
>>>
>> Right, I knew about that too, I just wanted to keep the code as clean as
>> possible and didn't find another way. Though it might be worth to do
>> some benchmarking / profiling. If it's really too bad, it will have to
>> change.
>>
>
> One more thing, the result of alpm_find_dep_satisfiers should be freed, which is
> forgotten (memleak). Remember, that the old list_find was a boolean function, it
> was modified in order to handle causingpkg for -Ru. No doubt, this will be
> slower. The average slowdown of (successful) dependency check will be about 2x.
> (considering only the real calculations, not disk read etc.) The question is
> that overall this will be notable or not, this needs testing, indeed. Basically,
> I don't prefer clean code over "performance", so I think this should be changed (*).
>

I think these are valid concerns, I will send a patch which should
address them, please review it

>>>> * remove _alpm_pkgname_pkg_cmp, which was used with
>> alpm_list_remove,
>>>> and
>>>> use _alpm_pkg_find + alpm_list_remove with _alpm_pkg_cmp instead.
>>>>
>>> Imho this is ugly. First we find it, then we again find it via
>> list_remove.
>>>
>> Yeah, it's not ideal either. But neither are dummy pkg or fake
>> asymmetric cmp functions. I just preferred it like that.
>> Imo the real problem here is that our data structures suck and are
>> inefficient. Linear search ftw.
>>
>
> (*) is also holds here. However, performance is not an issue here, just simply
> we do an "unneeded" task here, which I find ugly. alpm_list_find compare
> functions could be easily killed, but list_remove is quite a complicated
> function, thus "reimplementing" won't work. I think in this case keeping
> pkgpkgnamecmp was better, even if it was ugly.
>
> An alternative idea: alpm_list_find_node, which returns with an alpm_list_t
> node, not the ->data; and reimplement alpm_list_remove_node, but with two param
> list (head) and node. This information is enough to cleanly remove that node,
> and can be cut from the current list_remove (by splitting that function).
>

Since performance doesn't apply here (this code is just used by -Ru, and
should not be run an insane amount of times), then I prefer clean code
here. Anyway, I think we have many other more important things to worry
about.

_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev
 

Thread Tools




All times are GMT. The time now is 11:49 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org