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/questionsto 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 fileThe 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 PalecekAttachment:
0001-Cache-the-minimum-version-for-a-particular-soname.patch
Description: Binary data