[PATCH] Cache the minimum version for a particular soname

Hello,

when building some package, I noticed the dh_shlibdeps step takes a
noticeable amount of time. I profiled it and discovered it is because of
the function get_smallest_version, which is called for each (binary,
soname), and takes time proportional to the number of symbols.

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+).

What do you think about it?

Regards
Jiri Palecek

---
scripts/Dpkg/Shlibs/SymbolFile.pm | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/scripts/Dpkg/Shlibs/SymbolFile.pm
b/scripts/Dpkg/Shlibs/SymbolFile.pm
index 3412f5f..c71c390 100644
--- 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
@@ -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}));
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;
return $minver;
}

--
1.6.3.3


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST RemoveThis @lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster RemoveThis @lists.debian.org

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.

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.

Cheers,
--
Raphaël Hertzog


--
To UNSUBSCRIBE, email to debian-dpkg-REQUEST.DeleteThis@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmaster.DeleteThis@lists.debian.org