[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: [PATCH] Cache the minimum version for a particular soname



On Mon, 24 Aug 2009 23:10:10 +0200, Raphael Hertzog <hertzog@debian.org> wrote:

On Wed, 15 Jul 2009, Jiri Palecek wrote:
In this patch, I amended the issue by caching the minimal dependency
for a particular soname. It helped to reduce the time needed to
build the package (without compiling, ie. I didn't clean the package
before measurement) from about 10 minutes to about 5 minutes (on an
Athlon XP 1800+).

That's a significant difference so it's certainly worth it.

What do you think about it?

I would like to see this improved too. So here are some comments/questions
to go forward.

Did you run the test suite and does it work? If not, please do it and fix
either your code or the test suite accordingly.

I'm not sure. I did build the package, which by itself ran some tests, with this outcome:

PATH="../src:../scripts:/usr/sbin:/usr/bin:/sbin:/bin:/usr/bin/X11" srcdir=../../scripts PERL5LIB=../../scripts DPKG_DATADIR=../../scripts/.. PERL_DL_NONLAZY=1 /usr/bin/perl -I../../scripts "-MExtUtils::Command::MM" "-e" "test_harness(0, '.')" ../../scripts/t/*.t
../../scripts/t/000_pod....................skipped
        all skipped: Test::Pod 1.00 required for testing POD
../../scripts/t/100_Dpkg_Version...........ok
../../scripts/t/200_Dpkg_Shlibs............ok
../../scripts/t/300_Dpkg_BuildOptions......ok
../../scripts/t/400_Dpkg_Deps..............ok
../../scripts/t/500_Dpkg_Path..............ok
../../scripts/t/600_Dpkg_Changelog.........ok
        2/70 skipped: various reasons
../../scripts/t/700_Dpkg_Control...........ok
../../scripts/t/750_Dpkg_Substvars.........ok
../../scripts/t/800_Dpkg_IPC...............ok
../../scripts/t/900_update_alternatives....ok
All tests successful, 1 test and 2 subtests skipped.
Files=11, Tests=1589, 37 wallclock secs (16.03 cusr + 3.40 csys = 19.43 CPU)

I haven't done anything beyond that.

Some other remarks:
--- a/scripts/Dpkg/Shlibs/SymbolFile.pm
+++ b/scripts/Dpkg/Shlibs/SymbolFile.pm
@@ -275,8 +275,9 @@ sub merge_symbols {
 	$self->create_object($soname, '');
     }
     # Scan all symbols provided by the objects
+    my $obj = $self->{objects}{$soname};
+    delete $obj->{minver};
     foreach my $name (keys %dynsyms) {
-	my $obj = $self->{objects}{$soname};
         my $sym;
 	if (exists $obj->{syms}{$name}) {
 	    # If the symbol is already listed in the file

The cache should be invalidated in other methods too: mainly add_symbol()
(load() is covered for the relevant cases since it calls add_symbol() to
update symbol information).

Put a small comment explaining that this is only invalidating a cache.

     my $minver;
     foreach my $sym (values %{$self->{objects}{$soname}{syms}}) {
         next if $dep_id != $sym->{dep_id};
@@ -379,6 +382,8 @@ sub get_smallest_version {
             $minver = $sym->{minver};
         }
     }
+    $self->{objects}{$soname}{minver}={ } unless
defined($self->{objects}{$soname}{minver});
+    $self->{objects}{$soname}{minver}{$dep_id}=$minver;

Since $dep_id is a number starting at 0, you should better use an array
instead of a hash. That would be consistent with the "deps" array too.

That array should be always existing and thus created in create_object().
Instead of removing it, you should empty it when you want to invalidate
the cache.

You also miss spaces around "=".

@@ -371,6 +372,8 @@ sub get_dependency {
 sub get_smallest_version {
     my ($self, $soname, $dep_id) = @_;
     $dep_id = 0 unless defined($dep_id);
+    return $self->{objects}{$soname}{minver}{$dep_id}
if(defined($self->{objects}{$soname}{minver}) &&
+
defined($self->{objects}{$soname}{minver}{$dep_id}));

Please simplify this check based on the above remark (array always exists,
you only need to test if ...[$dep_id] is defined). You might also want to
avoid writing $self->{objects}{$soname} too many times by using a
temporary variable for it.

OK. I've reflected the remarks in an updated version, see the attachment. I'm only unsure about two things

* does the ->{wildcards} stuff have anything to do with it? The get_smallest_version computation doesn't use it, so I guess no.

* wouldn't invalidating in add_symbol make it slightly slower?

Regards
    Jiri Palecek

Attachment: 0001-Cache-the-minimum-version-for-a-particular-soname.patch
Description: Binary data


Reply to: