GNU bug report logs - #40355
[PATCH] Implement caching for libravatar lookup

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 40355 in the body.
You can then email your comments to 40355 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Philip K <philip <at> warpmail.net>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Implement caching for libravatar lookup
Date: Tue, 31 Mar 2020 20:03:36 +0200

Checking if a domain has an avatar server associated with it requires
at most two (synchronous) DNS queries, at least to ultimately default
to "libravatar.org". Caching the results of domain lookups reduced the
evaluation time (on my machine) from ~0.3s to an instantaneous
evaluation.

* lisp/image/gravatar.el (gravatar--service-libravatar): Check if
  domain has already been resolved before staring DNS queries
(gravatar-libravatar-cache): New variable.
---
 lisp/image/gravatar.el | 40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/lisp/image/gravatar.el b/lisp/image/gravatar.el
index a9cd540aa4..2572f9136f 100644
--- a/lisp/image/gravatar.el
+++ b/lisp/image/gravatar.el
@@ -138,14 +138,24 @@ gravatar-service
   :link '(url-link "https://gravatar.com/")
   :group 'gravatar)
 
-(defun gravatar--service-libravatar (addr)
-  "Find domain that hosts avatars for email address ADDR."
+(defvar gravatar-libravatar-cache (make-hash-table :test 'equal)
+  "Cache for `gravatar--service-libravatar'.")
+
+(defun gravatar--service-libravatar (addr &optional cache)
+  "Find domain that hosts avatars for email address ADDR.
+The optional argument CACHE must either be a hash table to
+memorise avatar server resolutions in, or nil, in which case it
+will default to `gravatar-libravatar-cache'."
   ;; implements https://wiki.libravatar.org/api/
   (save-match-data
     (if (not (string-match ".+@\\(.+\\)" addr))
         "https://seccdn.libravatar.org/avatar"
-      (let ((domain (match-string 1 addr)))
+      (let ((domain (downcase (match-string 1 addr))))
         (catch 'found
+          (setq cache (or cache gravatar-libravatar-cache))
+          (let ((cache (gethash domain cache)))
+            (when (and cache (time-less-p (current-time) (cdr cache)))
+              (throw 'found (car cache))))
           (dolist (record '(("_avatars-sec" . "https")
                             ("_avatars" . "http")))
             (let* ((query (concat (car record) "._tcp." domain))
@@ -173,12 +183,24 @@ gravatar--service-libravatar
                                (<= 1 (dns-get 'port rec) 65535)
                                (string-match-p "\\`[-.0-9A-Za-z]+\\'"
                                                (dns-get 'target rec)))
-                      (throw 'found (format "%s://%s:%s/avatar"
-                                            (cdr record)
-                                            (dns-get 'target rec)
-                                            (dns-get 'port rec))))
-                    (setq sum (- sum (dns-get 'weight rec)))))))
-            "https://seccdn.libravatar.org/avatar"))))))
+                      (let ((url (format "%s://%s:%s/avatar"
+                                         (cdr record)
+                                         (dns-get 'target rec)
+                                         (dns-get 'port rec)))
+                            (timeout (if (time-less-p
+                                          (seconds-to-time (dns-get 'ttl rec))
+                                          (days-to-time 1))
+                                         (days-to-time 1)
+                                       (seconds-to-time (dns-get 'ttl rec)))))
+                        (puthash domain
+                                 (cons url (time-add (current-time) timeout))
+                                 cache)
+                        (throw 'found url)))
+                    (setq sum (- sum (dns-get 'weight rec))))))))
+          (car (puthash domain
+                        (cons "https://seccdn.libravatar.org/avatar"
+                              (time-add (current-time) (days-to-time 30)))
+                        cache)))))))
 
 (defun gravatar-hash (mail-address)
   "Return the Gravatar hash for MAIL-ADDRESS."
-- 
2.20.1

Message #8 received at 40355 <at> debbugs.gnu.org (full text, mbox):

From: Robert Pluim <rpluim <at> gmail.com>
To: Philip K <philip <at> warpmail.net>
Cc: 40355 <at> debbugs.gnu.org
Subject: Re: bug#40355: [PATCH] Implement caching for libravatar lookup
Date: Tue, 31 Mar 2020 21:58:49 +0200

>>>>> On Tue, 31 Mar 2020 20:03:36 +0200, Philip K <philip <at> warpmail.net> said:

    Philip> Checking if a domain has an avatar server associated with it requires
    Philip> at most two (synchronous) DNS queries, at least to ultimately default
    Philip> to "libravatar.org". Caching the results of domain lookups reduced the
    Philip> evaluation time (on my machine) from ~0.3s to an instantaneous
    Philip> evaluation.

I doubt you need the 'cache' arg, you can keep the cache purely
internal.

How big is this cache likely to get? Perhaps use a weak hash table?

Robert

Message #11 received at 40355 <at> debbugs.gnu.org (full text, mbox):

From: philip <at> warpmail.net (Philip K.)
To: Robert Pluim <rpluim <at> gmail.com>
Cc: 40355 <at> debbugs.gnu.org
Subject: Re: bug#40355: [PATCH] Implement caching for libravatar lookup
Date: Tue, 31 Mar 2020 23:30:23 +0200

Robert Pluim <rpluim <at> gmail.com> writes:

>>>>>> On Tue, 31 Mar 2020 20:03:36 +0200, Philip K <philip <at> warpmail.net> said:
>
> I doubt you need the 'cache' arg, you can keep the cache purely
> internal.

I added it because I was considering defining gravatar-libravatar-cache
as

    (let ((cache (make-hash-table :test 'equal)))
      (dolist (domain '("aol.com" "att.net" "comcast.net" "facebook.com"
                        "gmail.com" "gmx.com" "googlemail.com" "google.com"
                        "hotmail.com" "hotmail.co.uk" "mac.com" "me.com"
                        "mail.com" "msn.com" "live.com" "sbcglobal.net"
                        ;; ...
                        ))
        (gravatar--service-libravatar (concat "ignored@" domain) cache))
      cache)

with a few popular domains. The only issue is that evaluating it takes
forever.

> How big is this cache likely to get? Perhaps use a weak hash table?

I haven't managed to gather good real-world data yet, but I would assume
something along the lines of 50-200 domains?

-- 
	Philip K.

Message #14 received at 40355 <at> debbugs.gnu.org (full text, mbox):

From: Robert Pluim <rpluim <at> gmail.com>
To: philip <at> warpmail.net (Philip K.)
Cc: 40355 <at> debbugs.gnu.org
Subject: Re: bug#40355: [PATCH] Implement caching for libravatar lookup
Date: Wed, 01 Apr 2020 09:27:00 +0200

>>>>> On Tue, 31 Mar 2020 23:30:23 +0200, philip <at> warpmail.net (Philip K.) said:

    Philip> Robert Pluim <rpluim <at> gmail.com> writes:
    >>>>>>> On Tue, 31 Mar 2020 20:03:36 +0200, Philip K <philip <at> warpmail.net> said:
    >> 
    >> I doubt you need the 'cache' arg, you can keep the cache purely
    >> internal.

    Philip> I added it because I was considering defining gravatar-libravatar-cache
    Philip> as

    Philip>     (let ((cache (make-hash-table :test 'equal)))
    Philip>       (dolist (domain '("aol.com" "att.net" "comcast.net" "facebook.com"
    Philip>                         "gmail.com" "gmx.com" "googlemail.com" "google.com"
    Philip>                         "hotmail.com" "hotmail.co.uk" "mac.com" "me.com"
    Philip>                         "mail.com" "msn.com" "live.com" "sbcglobal.net"
    Philip>                         ;; ...
    Philip>                         ))
    Philip>         (gravatar--service-libravatar (concat "ignored@" domain) cache))
    Philip>       cache)

    Philip> with a few popular domains. The only issue is that evaluating it takes
    Philip> forever.

Perhaps thatʼs a sign that pre-population is not needed :-)

    >> How big is this cache likely to get? Perhaps use a weak hash table?

    Philip> I haven't managed to gather good real-world data yet, but I would assume
    Philip> something along the lines of 50-200 domains?

OK, I donʼt think thatʼs onerous.

Robert

Message #17 received at 40355 <at> debbugs.gnu.org (full text, mbox):

From: philip <at> warpmail.net (Philip K.)
To: Robert Pluim <rpluim <at> gmail.com>
Cc: 40355 <at> debbugs.gnu.org
Subject: Re: bug#40355: [PATCH] Implement caching for libravatar lookup
Date: Wed, 01 Apr 2020 12:34:34 +0200

Robert Pluim <rpluim <at> gmail.com> writes:

>     Philip> I added it because I was considering defining gravatar-libravatar-cache
>     Philip> as
>
>     Philip>     (let ((cache (make-hash-table :test 'equal)))
>     Philip>       (dolist (domain '("aol.com" "att.net" "comcast.net" "facebook.com"
>     Philip>                         "gmail.com" "gmx.com" "googlemail.com" "google.com"
>     Philip>                         "hotmail.com" "hotmail.co.uk" "mac.com" "me.com"
>     Philip>                         "mail.com" "msn.com" "live.com" "sbcglobal.net"
>     Philip>                         ;; ...
>     Philip>                         ))
>     Philip>         (gravatar--service-libravatar (concat "ignored@" domain) cache))
>     Philip>       cache)
>
>     Philip> with a few popular domains. The only issue is that evaluating it takes
>     Philip> forever.
>
> Perhaps thatʼs a sign that pre-population is not needed :-)

It's not needed, but as soon as it is pre-populated the user experience
becomes a lot better. But unless there's a sensible way to fix it, I'll
just resubmit the patch without the optional argument :/

-- 
	Philip K.

Message #20 received at 40355 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Philip K <philip <at> warpmail.net>
Cc: 40355 <at> debbugs.gnu.org
Subject: Re: bug#40355: [PATCH] Implement caching for libravatar lookup
Date: Sat, 08 Aug 2020 15:03:58 +0200

Philip K <philip <at> warpmail.net> writes:

> Checking if a domain has an avatar server associated with it requires
> at most two (synchronous) DNS queries, at least to ultimately default
> to "libravatar.org". Caching the results of domain lookups reduced the
> evaluation time (on my machine) from ~0.3s to an instantaneous
> evaluation.
>
> * lisp/image/gravatar.el (gravatar--service-libravatar): Check if
>   domain has already been resolved before staring DNS queries
> (gravatar-libravatar-cache): New variable.

The code has changed a bit (and become asynchronous), but adding caching
would be nice.

Can you re-spin the patch on top of the current implementation?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #23 received at 40355 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Philip K <philip <at> warpmail.net>
Cc: 40355 <at> debbugs.gnu.org
Subject: Re: bug#40355: [PATCH] Implement caching for libravatar lookup
Date: Tue, 18 Aug 2020 16:46:09 +0200

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> The code has changed a bit (and become asynchronous), but adding caching
> would be nice.
>
> Can you re-spin the patch on top of the current implementation?

I've now implemented this in a very different way -- with a general
in-memory cache for all types of gravatars.  They're pretty small
images, anyway.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #30 received at 40355 <at> debbugs.gnu.org (full text, mbox):

From: Robert Pluim <rpluim <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 40355 <at> debbugs.gnu.org, Philip K <philip <at> warpmail.net>
Subject: Re: bug#40355: [PATCH] Implement caching for libravatar lookup
Date: Mon, 24 Aug 2020 16:09:34 +0200

>>>>> On Tue, 18 Aug 2020 16:46:09 +0200, Lars Ingebrigtsen <larsi <at> gnus.org> said:

    Lars> Lars Ingebrigtsen <larsi <at> gnus.org> writes:
    >> The code has changed a bit (and become asynchronous), but adding caching
    >> would be nice.
    >> 
    >> Can you re-spin the patch on top of the current implementation?

    Lars> I've now implemented this in a very different way -- with a general
    Lars> in-memory cache for all types of gravatars.  They're pretty small
    Lars> images, anyway.

Doesnʼt url-retrieve cache on-disk anyway?

Robert

Message #33 received at 40355 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: 40355 <at> debbugs.gnu.org, Philip K <philip <at> warpmail.net>
Subject: Re: bug#40355: [PATCH] Implement caching for libravatar lookup
Date: Mon, 24 Aug 2020 16:13:22 +0200

Robert Pluim <rpluim <at> gmail.com> writes:

> Doesnʼt url-retrieve cache on-disk anyway?

It does if you ask it to, but I thought it was simpler to just do a
general cache -- that way we didn't need two levels of caches.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.