Re: [PATCH] Cache the minimum version for a particular soname
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
Reply to: