[Aptitude-devel] Experimental package and package_pool implementation

[Daniel Burrows]

On Mon, Jul 12, 2010 at 2:03 PM, Piotr Galiszewski <piotr at galiszewski.pl> wrote:
> Today I have been designing and writing new packages class
> implementation. All work is available at 004.1-package branch on
> gitorious.

  Thanks!  Do you think you could rebase these onto master?  (probably
renaming them 001.1 then)

> Firstly I created new package class, This version doesn't contains any
> reference to any Qt classes. As it was suggested in the discussion,
> all members of the class are independently lazy loaded from the
> pkgIterator object. Package name is an only exception, cause it would
> be loaded for all package shortly after creating package_model. In
> order to avoid discarding const qualifiers of getters, all class
> variables are mutable and requires methods are const. I am not using
> PackageInformation class from GTK frontend, but I have moved required
> code to this new class. For a convenience I have added a typedef for
> package's boost::shared_ptr. If the program is compiled with Qt
> support this pointer is exported as metatype for use in packages_model

  A couple comments here:

    - What's a metatype?

    - Many of the values in this class can vary by version; maybe it would
      be a good idea to have a "version" class to go with package?

    - In that case, the list of versions should probably be a list of shared
      version pointers.

    - I would return the members of package by const reference, to avoid
      extra copies.  This is slightly dangerous, of course, but the idiom is
      widely used inside aptitude; return by reference generally indicates
      that you're getting a reference to part of an object and you should
      copy it if you want it to survive longer than the parent object.

    - I would probably do this for the list of versions:

          typedef /* TBD */ version_type;

        public:
          typedef std::list<version_type>::const_iterator version_iterator;

          version_iterator versions_begin();
          version_iterator versions_end();

      That insulates the client code a bit more from the details of how the
      versions are stored.

    - Typically I declare "namespace cw = cwidget" at the top of files that use
      cwidget names, to make names shorter.

    - On namespacing: I've recently taken to pulling individual names in via
       "using" at the top of the file; it's nice documentation of what a module
       uses:

           using cw::util::transcode;

       This avoids the problem with "using namespace", where you pollute
       your namespace with a huge amount of junk.

       I generally don't do this for names in std::, because that's not too long
       a prefix, and the names in std tend to be fairly generic.  I could be
       convinced that it's a good idea, though.

    - Is the list of "tags" the debtags for the package?  I believe that tags.h
      returns those as a std::set<tag>; it would probably make sense to just
      store that value directly.

    - Maybe inherit from boost::noncopyable to make it noncopyable?

    - I would drop the virtual destructor here and make it impossible to
      subclass package (by hiding its constructor and using a static factory).
      That allows you to avoid a virtual method table for each instance (not
      a huge deal even if we create tens of thousands of them, but not bad
      either)

    - I really wish boost::flyweight didn't suck so we could make these strings
      into flyweights easily.

> Second class is package_aware_object. It purpose is to notify relevant
> classes about changing a state of package_pool class, which was added
> in later patch. If some class want to be notified about this event, it
> needs to inherit from this class and reimplement three methods. This
> approach is commonly used instead of Qt signals in Kadu IM, so I'd
> like to ask you about your opinions. package_info_tab and
> status_widget will be another classes using this aware object

  The normal way of doing this in aptitude would be to just connect to
the relevant signals out of src/generic/apt.h and src/generic/aptcache.h.
I don't see a problem with having a convenience base class that sets up
these connections for you in its constructor.  And using sigc++ will give
you automatic features like detaching signals when the target is
destroyed (as long as you inherit from sigc::trackable, which you can do
in the base class).  [ok, I read your comments below; I'll think this over
some more]

  You should store the object pointers in a hash table (e.g.,
boost::unordered_set) so that removing them is efficient.

  Also, the state-changed signal should probably take a set of package
objects instead of accepting them one at a time.  That's a bit problematic
since you'll have to implement operator< or a hash function; a list would
be OK for now.

  I do find it odd that the methods to trigger the signals are public, but
the signals are protected.  If you set up connections to the global signals
in the base class constructor, you shouldn't need public methods to trigger
the notifications, and you can encapsulate all the signal-handling in one
module.  Conversely, I don't see any harm in making these methods
public.  aptitude generally doesn't use protected inheritance, although this
is a reasonably sane use of it.

  I know there are a bunch of examples of this in the aptitude source
base.  e.g., look at how src/gtk/info.cc uses the cache_closed and
cache_reloaded signals.  The package_states_changed signal is used
in entityview.cc.

  (an amusing variant would be to use *private* virtual methods here.
I don't think there's any real point in deploying that particular C++ trick,
though)

  I know I said that I normally prefer fully abstract interfaces :).  This
may be an exception -- for this sort of event handling it could be better
to have default NOP implementations.

> Third patch introduces package_pool. This class is responsible for
> managing a list of packages, which was earlier held by packages_model
> class. Updated packages_model was added in fourth patch. packages_pool
> will be the only class which is directly connected to signals about
> cache updating or reloading. It updates inner package list (which uses
> boost::multi_index_conantainer) and later translates this signal and
> propagates it by package_aware_object. I did it in this way, cause I
> need to be sure that packages_pool is updated earlier than other
> classes using package objects.

  First, please put this behind an abstract interface -- it shouldn't be called
often enough for that to matter performance-wise, and right now you're
forcing everyone to instantiate a multi_index_container if they include it.
(that will also help with unit tests -- see below)
  Similarly, you should be able to forward-declare "package" instead of
including it.

  I would probably do a reserve() on the random access index before
stuffing the whole package cache into it, for efficiency.


  I think a more "aptitude"y way of handling events would be to expose
signals (or better yet, abstract connect_ methods like you can find in
some of my more recent code) from the package_pool corresponding
to the signals that it emits.  It would then emit its own signals at the
appropriate time, allowing you to handle signal ordering without adding
a new abstraction just for handling callbacks.

  I'm not really sure which of these approaches is "better" -- having an
event-handler interface would be a bit more memory-efficient (less
signal connections to store) and would add some uniformity to the
code, but is different from how this is handled elsewhere in aptitude.

  I also wonder how this would work with unit tests.  Ideally we would
eventually be able to fully wrap the low-level apt cache in package_pool
or some similar class, and then provide a fake test implementation
that didn't require bringing up all of apt.  Using a singleton makes this
somewhat tricky; it would also be nice if the connections weren't hidden
inside the package_aware_object class.

  Daniel