Re: [PATCHv5 3/3] gitweb: gravatar url cache

Giuseppe Bilotta [off-list ref] writes:

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

Sign-off?

I think the cache is placed at the wrong level (it doesn't have to be a
GRavatar_url_cache, but can be a general avatar_url_cache).

IOW,

	our %avatar_url_cache = ();
        sub git_get_avatar {
        	...
                my $url;
                if (!exists $avatar_url_cache{$email}) {
                	if ($git_gravatar_enabled) {
                        	$url = ... gravatar stuff ...;
			} else if ($git_whatever_enabled) {
                        	$url = ... other stuff ...;
			}
                        $avatar_url_cache{$email} = $url;
		}
                $url = $avatar_url_cache{$email};
                if (defined $url) {
                	return ... prefix then $url then suffix ...;
                } else {
                	return "";
                }
	}

But the basic idea is sound, I think.

On Thu, Jun 25, 2009 at 12:02 AM, Junio C Hamano[off-list ref] wrote:

Giuseppe Bilotta [off-list ref] writes:

quoted

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

Sign-off?

Duh.

I think the cache is placed at the wrong level (it doesn't have to be a
GRavatar_url_cache, but can be a general avatar_url_cache).

I'm not sure about it. The URL depends on email and size (can you use
arrays as hash keys in Perl?) , and the email part might be the same
but the size part might differ across separate calls (in theory; in
practice avatars in a view are presently all the same size; but for
example if we were to autodetect email addresses in commit messages,
we might have both single- and double- sided avatars in the same
page). By hashing on email+size only we would lose the benefit of
cache when using the same avatar at separate sizes.

-- 
Giuseppe "Oblomov" Bilotta

On Wed, Jun 24, 2009 at 15:46, Giuseppe
Bilotta[off-list ref] wrote:

On Thu, Jun 25, 2009 at 12:02 AM, Junio C Hamano[off-list ref] wrote:

quoted

I think the cache is placed at the wrong level (it doesn't have to be a
GRavatar_url_cache, but can be a general avatar_url_cache).

I'm not sure about it. The URL depends on email and size (can you use
arrays as hash keys in Perl?) , and the email part might be the same
but the size part might differ across separate calls (in theory; in
practice avatars in a view are presently all the same size; but for
example if we were to autodetect email addresses in commit messages,
we might have both single- and double- sided avatars in the same
page). By hashing on email+size only we would lose the benefit of
cache when using the same avatar at separate sizes.

You could have a hash key of "$email_$size", or something similar to
fake an array hash key.

On Thu, Jun 25, 2009 at 1:06 AM, Jacob Helwig[off-list ref] wrote:

On Wed, Jun 24, 2009 at 15:46, Giuseppe
Bilotta[off-list ref] wrote:

quoted

On Thu, Jun 25, 2009 at 12:02 AM, Junio C Hamano[off-list ref] wrote:

quoted

I think the cache is placed at the wrong level (it doesn't have to be a
GRavatar_url_cache, but can be a general avatar_url_cache).

I'm not sure about it. The URL depends on email and size (can you use
arrays as hash keys in Perl?) , and the email part might be the same
but the size part might differ across separate calls (in theory; in
practice avatars in a view are presently all the same size; but for
example if we were to autodetect email addresses in commit messages,
we might have both single- and double- sided avatars in the same
page). By hashing on email+size only we would lose the benefit of
cache when using the same avatar at separate sizes.

You could have a hash key of "$email_$size", or something similar to
fake an array hash key.

The point is not so much the form to give to the key but rather the
fact that hashing on both means the URL has to be recomputed when the
same email appears with both sizes. Considering that (at least for
gravatars) the computational intensive part comes from the MD5 of the
email, this means a waste of cycles.

By letting the cache be per-avatar, each avatar kind can choose to
hash on whatever it needs to. But I got an idea on how to improve on
this.

-- 
Giuseppe "Oblomov" Bilotta

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.

Signed-off-by: Giuseppe Bilotta <redacted>
---
 gitweb/gitweb.perl |   24 ++++++++++++++++++++++--
 1 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6e807fe..6771a9d 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -3249,6 +3249,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 = ();
+
+# 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;
+}
+
 # Insert an avatar for the given $email at the given $size if the feature
 # is enabled.
 sub git_get_avatar {

@@ -3258,8 +3279,7 @@ sub git_get_avatar {
 	my $size = $avatar_size{$params{'size'}} || $avatar_size{'default'};
 	my $url = "";
 	if ($git_gravatar_enabled) {
-		$url = "http://www.gravatar.com/avatar.php?gravatar_id=" .
-			Digest::MD5::md5_hex(lc $email) . "&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

-- 
1.6.3.rc1.192.gdbfcb