On Wed, Mar 6, 2013 at 12:44 PM, Simon Gomizelj <simongmzlj@gmail.com> wrote:
While diving into pacman's output code to try and add colours I've noticed that pacman's pretty printing code is somewhat awkward. Its very clear its grow organically without ever really being cleaned up.
- Translation messages shouldn't have formatting information embedded into them:
_("Name"), not _("Name :") _("Version"), not _("Version :") etc
- Pacman puts a lot of effort into making some parts of its output is nicely aligned, while other parts don't and expects padding to be hardcoded. Pacman should do _all_ the aligning. Agreed.
An example of where pacman's pretty printing code is specifically inefficient... dump_pkg_full:
cols = getcols(fileno(stdout));
string_display(_("Name :"), alpm_pkg_get_name(pkg), cols); string_display(_("Version :"), alpm_pkg_get_version(pkg), cols); string_display(_("Description :"), alpm_pkg_get_desc(pkg), cols); string_display(_("Architecture :"), alpm_pkg_get_arch(pkg), cols); string_display(_("URL :"), alpm_pkg_get_url(pkg), cols); list_display(_("Licenses :"), alpm_pkg_get_licenses(pkg), cols); list_display(_("Groups :"), alpm_pkg_get_groups(pkg), cols); deplist_display(_("Provides :"), alpm_pkg_get_provides(pkg), cols); deplist_display(_("Depends On :"), alpm_pkg_get_depends(pkg), cols);
- cols is recalculated for each package. - string_display,list_display,etc call string_length on $1 each time.
These two numbers could basically be considered constants though through the outputting. string_length on the gettext string == useful optimization.
So this means when I do `pacman -Si` (output for ~6080 packages here):
- cols is recalculated ~6080 times too much. - string_display about (6080 * 20) or 121600 times.
dump_pkg_full is basically outputting a table. There is no reason pacman couldn't pre-calculate the dimensions of the table to significantly reduce the amount of work it does when printing each row.
Furthermore, there seems to be, with my colour patches, two more independent table implementations: table_display and add_table_row/display_transaction_sizes. They all build the same kind of table and format them in similar ways.
I think the right way forward is to add a nice table formatter library to pacman that can be reused for everything. I whipped up a very quick prototype as a proof of concept: http://ix.io/4CP/
I wanted to bring this up so that a) its on the record and b) we can start talking about if/how to implement this. I'm willing to put in the effort to get this through. Haven't looked at the code yet but I like where this is going. Will
Please don't change the getcols logic though. This has been made cheap enough and we do this quite on purpose. See something like the download and install progress code- we call getcols() every 0.1 seconds at a minimum and this is to ensure resizing a console window resizes the output you get on screen. While odd that someone would resize during a full package listing, I don't think it is unusual for us to accomidate it. If you profile and actually find this is a significant slowness, we might consider it, but I would guess the string_length changes are MUCH more effective. try to do that sometime and offer more concrete feedback.
From a history perspective, the table stuff is super new while the Qi/Si is nearly original code, with translation and whatnot added much later.
-Dan