Re: [PATCHv6 5/8] gitweb: gravatar url cache

From: Jakub Narebski <jnareb@gmail.com>
Date: Jun 26, 2009, 23:11 UTC
Message-ID: <200906270111.26640.jnareb@gmail.com>
In-Reply-To: <1245926587-25074-6-git-send-email-giuseppe.bilotta@gmail.com>

On Thu, 25 Jun 2009, Giuseppe Bilotta wrote:

> Views which contain many occurrences of the same email address (e.g.
> shortlog view) benefit from not having to recalculate the MD5 of the
> email address every time.

It would be nice to have some benchmarks comparing performance before and after this patch.

> 
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>

Acked-by: Jakub Narebski <jnareb@gmail.com>

Show 19 quoted lines

> ---
>  gitweb/gitweb.perl |   24 ++++++++++++++++++++++--
>  1 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index f2e0cfe..d3bc849 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3257,6 +3257,27 @@ sub git_print_header_div {
>  	      "\n</div>\n";
>  }
>  
> +# Rather than recomputing the url for an email multiple times, we cache it
> +# after the first hit. This gives a visible benefit in views where the avatar
> +# for the same email is used repeatedly (e.g. shortlog).
> +# The cache is shared by all avatar engines (currently gravatar only), which
> +# are free to use it as preferred. Since only one avatar engine is used for any
> +# given page, there's no risk for cache conflicts.
> +our %avatar_cache = ();

Nice explanation.

Show 13 quoted lines

> +
> +# Compute the gravatar url for a given email, if it's not in the cache already.
> +# Gravatar stores only the part of the URL before the size, since that's the
> +# one computationally more expensive. This also allows reuse of the cache for
> +# different sizes (for this particular engine).
> +sub gravatar_url {
> +	my $email = lc shift;
> +	my $size = shift;
> +	$avatar_cache{$email} ||=
> +		"http://www.gravatar.com/avatar.php?gravatar_id=" .
> +			Digest::MD5::md5_hex($email) . "&amp;size=";
> +	return $avatar_cache{$email} . $size;
> +}

Nice solution. Very good.

I guess it is not worth it to _not_ use cache for few avatars views such as 'commit', 'commitdiff', in the future also 'tag' view, isn't it?

BTW. http://www.gravatar.com/site/implement/url recommends http://www.gravatar.com/avatar/3b3be63a4c2a439b013787725dfce802 rather than http://www.gravatar.com/avatar.php?gravatar_id=3b3be63a4c2a439b013787725dfce802 you use, following http://www.gravatar.com/site/implement/perl Hmmm...

Show 14 quoted lines

> +
>  # Insert an avatar for the given $email at the given $size if the feature
>  # is enabled.
>  sub git_get_avatar {
> @@ -3266,8 +3287,7 @@ sub git_get_avatar {
>  	my $size = $avatar_size{$params{'size'}} || $avatar_size{'default'};
>  	my $url = "";
>  	if ($git_avatar eq 'gravatar') {
> -		$url = "http://www.gravatar.com/avatar.php?gravatar_id=" .
> -			Digest::MD5::md5_hex(lc $email) . "&amp;size=$size";
> +		$url = gravatar_url($email, $size);
>  	}
>  	# Currently only gravatars are supported, but other forms such as
>  	# picons can be added by putting an else up here and defining $url

Very nice.

> -- 
> 1.6.3.rc1.192.gdbfcb
> 
>

-- 
Jakub Narebski
Poland