bug#40704: 28.0.50; Improve and speed up (Gnus) registry saving

bug#40704: 28.0.50; Improve and speed up (Gnus) registry saving

[Michael Heerdegen]

[-- Attachment #1: Type: text/plain, Size: 1333 bytes --]


Hello,

Saving the Gnus registry is quite slow currently.  I profiled a bit and
for now suggest to do something like in the attached patch.  In detail:

(1) We need to bind inhibit-modification-hooks -> t, this offers a good
speedup (~ 4 or so).

(2) Printing the registry which basically consists of huge hash tables,
causes a lot of garbage.  Most of that garbage seems to be unavoidable
(is it created by the printing primitives?).  Anyway, seems we should
temporarily increase `gc-cons-threshold' drastically, this offers
another speedup of 25% or so.  The patch attached uses the value that
works well for me and the size of my registry, and I bind it in
`gnus-registry-save', because I assume other registries outside of Gnus
can be smaller.  What would be a good value of `gc-cons-threshold', or
should it even scale with `gnus-registry-max-entries' instead of being
constant?

(3) I also decided to change `eieio-override-prin1' to print hash tables
"by hand" from Lisp.  The eieio-persistent requires to modify how
elements in the hash tables are printed, and the current way of doing
this (make a copy of the complete table, change the elements, prin1 and
re-read the result) is not only hackish but also inefficient (it does
this recursively for nested tables).

Any comments on the suggested changes?

TIA,

Michael.




[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-WIP-Try-to-improve-gnus-registry-saving.patch --]
[-- Type: text/x-diff, Size: 3406 bytes --]

From 618362e3496df18d0f60143b71185ad57501fdcb Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen@web.de>
Date: Mon, 13 Apr 2020 23:33:39 +0200
Subject: [PATCH] WIP: Try to improve gnus registry saving

---
 lisp/emacs-lisp/eieio-base.el |  3 ++-
 lisp/emacs-lisp/eieio.el      | 33 +++++++++++++++++++++++++--------
 lisp/gnus/gnus-registry.el    |  1 +
 3 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/lisp/emacs-lisp/eieio-base.el b/lisp/emacs-lisp/eieio-base.el
index 2cb1f614ce..010a2b673e 100644
--- a/lisp/emacs-lisp/eieio-base.el
+++ b/lisp/emacs-lisp/eieio-base.el
@@ -473,7 +473,8 @@ eieio-persistent-save
     (let* ((cfn (or file (oref this file)))
            (default-directory (file-name-directory cfn)))
       (cl-letf ((standard-output (current-buffer))
-                ((oref this file)       ;FIXME: Why change it?
+                (inhibit-modification-hooks t)
+                ((oref this file) ;FIXME: Why change it?
                  (if file
                      ;; FIXME: Makes a name relative to (oref this file),
                      ;; whereas I think it should be relative to cfn.
diff --git a/lisp/emacs-lisp/eieio.el b/lisp/emacs-lisp/eieio.el
index 9f8b639e52..43d515e1fa 100644
--- a/lisp/emacs-lisp/eieio.el
+++ b/lisp/emacs-lisp/eieio.el
@@ -934,15 +934,32 @@ eieio-override-prin1
 	((consp thing)
 	 (eieio-list-prin1 thing))
 	((hash-table-p thing)
-         (let ((copy (copy-hash-table thing)))
-	   (maphash
+         (princ "#s(hash-table size ")
+         (prin1 (hash-table-size thing))
+         (princ " test ")
+         (prin1 (hash-table-test thing))
+         (princ " weakness ")
+         (prin1 (hash-table-weakness thing))
+         (princ " rehash-size ")
+         (prin1 (hash-table-rehash-size thing))
+         (princ " rehash-threshold ")
+         (prin1 (hash-table-rehash-threshold thing))
+         (princ " data (")
+         (let* ((eieio-print-depth (if eieio-print-indentation
+                                       (1+ eieio-print-depth)
+                                     eieio-print-depth))
+                (do-indent (if eieio-print-indentation
+                               (lambda () (princ (make-string (* eieio-print-depth 2) ? )))
+                             #'ignore)))
+           (maphash
 	    (lambda (key val)
-	      (setf (gethash key copy)
-		    (read
-		     (with-output-to-string
-		       (eieio-override-prin1 val)))))
-	    copy)
-	   (prin1 copy)))
+              (princ "\n")
+              (funcall do-indent)
+              (prin1 key)
+              (princ " ")
+              (eieio-override-prin1 val))
+            thing))
+         (princ "))"))
 	((vectorp thing)
          (let ((copy (copy-sequence thing)))
 	  (dotimes (i (length copy))
diff --git a/lisp/gnus/gnus-registry.el b/lisp/gnus/gnus-registry.el
index 480ed80ef8..4ac3c84a80 100644
--- a/lisp/gnus/gnus-registry.el
+++ b/lisp/gnus/gnus-registry.el
@@ -398,6 +398,7 @@ gnus-registry-save
   (interactive)
   (let* ((file (or file gnus-registry-cache-file))
          (db (or db gnus-registry-db))
+         (gc-cons-threshold (max gc-cons-threshold (* 800000 500)))
 	 (clone (clone db)))
     (gnus-message 5 "Saving Gnus registry (%d entries) to %s..."
                   (registry-size db) file)
--
2.25.1

bug#40704: 28.0.50; Improve and speed up (Gnus) registry saving

[Eric Abrahamsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Hello,
>
> Saving the Gnus registry is quite slow currently.  I profiled a bit and
> for now suggest to do something like in the attached patch.  In detail:
>
> (1) We need to bind inhibit-modification-hooks -> t, this offers a good
> speedup (~ 4 or so).
>
> (2) Printing the registry which basically consists of huge hash tables,
> causes a lot of garbage.  Most of that garbage seems to be unavoidable
> (is it created by the printing primitives?).  Anyway, seems we should
> temporarily increase `gc-cons-threshold' drastically, this offers
> another speedup of 25% or so.  The patch attached uses the value that
> works well for me and the size of my registry, and I bind it in
> `gnus-registry-save', because I assume other registries outside of Gnus
> can be smaller.  What would be a good value of `gc-cons-threshold', or
> should it even scale with `gnus-registry-max-entries' instead of being
> constant?
>
> (3) I also decided to change `eieio-override-prin1' to print hash tables
> "by hand" from Lisp.  The eieio-persistent requires to modify how
> elements in the hash tables are printed, and the current way of doing
> this (make a copy of the complete table, change the elements, prin1 and
> re-read the result) is not only hackish but also inefficient (it does
> this recursively for nested tables).
>
> Any comments on the suggested changes?

Not that it's up to me, but I'm all for putting in #1 and #3 as-is, and
adjusting #2 to scale with the number of `gnus-registry-max-entries',
with the addition of a hard ceiling.

Eric

bug#40704: 28.0.50; Improve and speed up (Gnus) registry saving

[Michael Heerdegen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> > Any comments on the suggested changes?
>
> Not that it's up to me,

I wonder who it's up to...

> but I'm all for putting in #1 and #3 as-is, and adjusting #2 to scale
> with the number of `gnus-registry-max-entries', with the addition of a
> hard ceiling.

Too large values of `gnus-registry-max-entries' can be harmful
(fragmentation of memory?), I wonder if this can potentially be the case
here.

I'm also not sure if scaling is a good idea at all.  The value I chose
will still make gc happen much less frequently even if your registry is
huge.  So it's still a win.  If your value is too large, the final
cleanup may take longer as necessary -- who has experience with this
problem -- anybody?


Thanks,

Michael.

bug#40704: 28.0.50; Improve and speed up (Gnus) registry saving

[Eric Abrahamsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> > Any comments on the suggested changes?
>>
>> Not that it's up to me,
>
> I wonder who it's up to...

There's sort of a `with-gnus-bug-report' macro that we're working with
here, where if Lars (or in this case, Ted Zlatanov) doesn't respond
within a certain period of time, the commit automatically goes in.

>> but I'm all for putting in #1 and #3 as-is, and adjusting #2 to scale
>> with the number of `gnus-registry-max-entries', with the addition of a
>> hard ceiling.
>
> Too large values of `gnus-registry-max-entries' can be harmful
> (fragmentation of memory?), I wonder if this can potentially be the case
> here.

Well that's where the hard ceiling comes in, right?

> I'm also not sure if scaling is a good idea at all.  The value I chose
> will still make gc happen much less frequently even if your registry is
> huge.  So it's still a win.  If your value is too large, the final
> cleanup may take longer as necessary -- who has experience with this
> problem -- anybody?

Not me, let's hope for more respondents...

bug#40704: 28.0.50; Improve and speed up (Gnus) registry saving

[Michael Heerdegen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> > -- who has experience with this problem -- anybody?

Maybe the scaling idea is not a good idea at all.  Even when it works as
we hope - the ideal factor could still depend on what kind of data is
saved.

> Not me, let's hope for more respondents...

Did you try how the increased gc-limit behaves for you btw?  Then we
would at least have two data points.

Michael.

bug#40704: 28.0.50; Improve and speed up (Gnus) registry saving

[Eric Abrahamsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>
>> > -- who has experience with this problem -- anybody?
>
> Maybe the scaling idea is not a good idea at all.  Even when it works as
> we hope - the ideal factor could still depend on what kind of data is
> saved.
>
>> Not me, let's hope for more respondents...
>
> Did you try how the increased gc-limit behaves for you btw?  Then we
> would at least have two data points.

I applied the patch a couple of days ago and have been very pleased with
the speedup! Nothing "bad" has happened, but I haven't tried
experimenting with different gc limits, either.

bug#40704: 28.0.50; Improve and speed up (Gnus) registry saving

[Eric Abrahamsen]

Eric Abrahamsen <eric@ericabrahamsen.net> writes:

> Michael Heerdegen <michael_heerdegen@web.de> writes:
>
>> Eric Abrahamsen <eric@ericabrahamsen.net> writes:
>>
>>> > -- who has experience with this problem -- anybody?
>>
>> Maybe the scaling idea is not a good idea at all.  Even when it works as
>> we hope - the ideal factor could still depend on what kind of data is
>> saved.
>>
>>> Not me, let's hope for more respondents...
>>
>> Did you try how the increased gc-limit behaves for you btw?  Then we
>> would at least have two data points.
>
> I applied the patch a couple of days ago and have been very pleased with
> the speedup! Nothing "bad" has happened, but I haven't tried
> experimenting with different gc limits, either.

Hey let's at least stick in the modification-hook and hash-table
improvements now, there are Gnus users out there still waiting for their
registry to save! It helps noticeably with EBDB, too.

bug#40704: 28.0.50; Improve and speed up (Gnus) registry saving

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> (3) I also decided to change `eieio-override-prin1' to print hash tables
> "by hand" from Lisp.  The eieio-persistent requires to modify how
> elements in the hash tables are printed, and the current way of doing
> this (make a copy of the complete table, change the elements, prin1 and
> re-read the result) is not only hackish but also inefficient (it does
> this recursively for nested tables).

Yeah, the current implementation seems maximally inefficient...  But I
guess this copies functionality found elsewhere?

> +         (princ "#s(hash-table size ")
> +         (prin1 (hash-table-size thing))
> +         (princ " test ")
> +         (prin1 (hash-table-test thing))
> +         (princ " weakness ")
> +         (prin1 (hash-table-weakness thing))
> +         (princ " rehash-size ")

etc

So if the other printer changes, then this has to change, too?  That
seems kinda brittle -- there should at least be references between the
two printers with a note to keep them updated if one of them changes.

> diff --git a/lisp/gnus/gnus-registry.el b/lisp/gnus/gnus-registry.el
> index 480ed80ef8..4ac3c84a80 100644
> --- a/lisp/gnus/gnus-registry.el
> +++ b/lisp/gnus/gnus-registry.el
> @@ -398,6 +398,7 @@ gnus-registry-save
>    (interactive)
>    (let* ((file (or file gnus-registry-cache-file))
>           (db (or db gnus-registry-db))
> +         (gc-cons-threshold (max gc-cons-threshold (* 800000 500)))
>  	 (clone (clone db)))
>      (gnus-message 5 "Saving Gnus registry (%d entries) to %s..."
>                    (registry-size db) file)

Could this have adverse consequences for people with low memory?

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

bug#40704: 28.0.50; Improve and speed up (Gnus) registry saving

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:


> > +         (princ "#s(hash-table size ")
> > +         (prin1 (hash-table-size thing))
> > +         (princ " test ")
> > +         (prin1 (hash-table-test thing))
> > +         (princ " weakness ")
> > +         (prin1 (hash-table-weakness thing))
> > +         (princ " rehash-size ")
>
> etc
>
> So if the other printer changes, then this has to change, too?  That
> seems kinda brittle -- there should at least be references between the
> two printers with a note to keep them updated if one of them changes.

What do you mean, "other printer"?  The Lisp printer?

This read syntax is officially described in the Elisp manual:

  (info "(elisp) Creating Hash")

(near the end of the page), so I would expect that the syntax will be
supported in the future.

> > diff --git a/lisp/gnus/gnus-registry.el b/lisp/gnus/gnus-registry.el
> > index 480ed80ef8..4ac3c84a80 100644
> > --- a/lisp/gnus/gnus-registry.el
> > +++ b/lisp/gnus/gnus-registry.el
> > @@ -398,6 +398,7 @@ gnus-registry-save
> >    (interactive)
> >    (let* ((file (or file gnus-registry-cache-file))
> >           (db (or db gnus-registry-db))
> > +         (gc-cons-threshold (max gc-cons-threshold (* 800000 500)))
> >  	 (clone (clone db)))
> >      (gnus-message 5 "Saving Gnus registry (%d entries) to %s..."
> >                    (registry-size db) file)
>
> Could this have adverse consequences for people with low memory?

These are 400 MB... Could be?  Dunno.  I wonder, though, if when you
would hit that limit when that code runs, your computer can hold that
huge hash-table at all.  I don't know the relation between hash-table
size and corresponding amount of garbage.  But I guess if you are low on
memory using the registry is problematic per se.

Michael.

bug#40704: 28.0.50; Improve and speed up (Gnus) registry saving

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

>> So if the other printer changes, then this has to change, too?  That
>> seems kinda brittle -- there should at least be references between the
>> two printers with a note to keep them updated if one of them changes.
>
> What do you mean, "other printer"?  The Lisp printer?
>
> This read syntax is officially described in the Elisp manual:
>
>   (info "(elisp) Creating Hash")
>
> (near the end of the page), so I would expect that the syntax will be
> supported in the future.

Presumably there's already code to print a hash table somewhere in
Emacs?  That would be the other printer, since your patch adds a new one.

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

bug#40704: 28.0.50; Improve and speed up (Gnus) registry saving

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Presumably there's already code to print a hash table somewhere in
> Emacs?  That would be the other printer, since your patch adds a new
> one.

That's what the code currently already does.  But: See what I wrote
about the purpose of the patch: our saving code needs to modify how
elements in the hash table are printed.  The Lisp printer doesn't allow
that per se.  My suggested patch substitutes a very inefficient hack
with a more efficient hack.  The current code uses printing + re-reading
AFAIR - and that on nested hash tables.

I thought long about removing the need for this hack altogehter, but
this is very complicated, if not impossible (yes, you would need to
change the printer).

Michael.

bug#40704: 28.0.50; Improve and speed up (Gnus) registry saving

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> Presumably there's already code to print a hash table somewhere in
>> Emacs?  That would be the other printer, since your patch adds a new
>> one.
>
> That's what the code currently already does.  But: See what I wrote
> about the purpose of the patch: our saving code needs to modify how
> elements in the hash table are printed.  The Lisp printer doesn't allow
> that per se.  My suggested patch substitutes a very inefficient hack
> with a more efficient hack.  The current code uses printing + re-reading
> AFAIR - and that on nested hash tables.
>
> I thought long about removing the need for this hack altogehter, but
> this is very complicated, if not impossible (yes, you would need to
> change the printer).

I didn't object to this new, more efficient printer -- I just pointed
out that you should add comments to both of the hash table printers that
there are (now) two of them.

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

bug#40704: 28.0.50; Improve and speed up (Gnus) registry saving

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:


> > > +         (gc-cons-threshold (max gc-cons-threshold (* 800000 500)))
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Maybe it would be wise to factor this out into a defvar so that it's in
reach of users.  The value could be a function whose return value could
also depend on the actual size of the registry.


Michael.

bug#40704: 28.0.50; Improve and speed up (Gnus) registry saving

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Michael Heerdegen <michael_heerdegen@web.de> writes:
>
>> > > +         (gc-cons-threshold (max gc-cons-threshold (* 800000 500)))
>                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Maybe it would be wise to factor this out into a defvar so that it's in
> reach of users.  The value could be a function whose return value could
> also depend on the actual size of the registry.

Sure; that sounds good.

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

bug#40704: 28.0.50; Improve and speed up (Gnus) registry saving

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> (3) I also decided to change `eieio-override-prin1' to print hash tables
> "by hand" from Lisp.  The eieio-persistent requires to modify how
> elements in the hash tables are printed, and the current way of doing
> this (make a copy of the complete table, change the elements, prin1 and
> re-read the result) is not only hackish but also inefficient (it does
> this recursively for nested tables).

[...]

> +         (princ "#s(hash-table size ")
> +         (prin1 (hash-table-size thing))
> +         (princ " test ")
> +         (prin1 (hash-table-test thing))
> +         (princ " weakness ")
> +         (prin1 (hash-table-weakness thing))
> +         (princ " rehash-size ")
> +         (prin1 (hash-table-rehash-size thing))
> +         (princ " rehash-threshold ")
> +         (prin1 (hash-table-rehash-threshold thing))
> +         (princ " data (")

I'm still not enthusiastic about duplicating the hash printing here.
Whenever print_vectorlike changes, this has to be changed, too.

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

bug#40704: 28.0.50; Improve and speed up (Gnus) registry saving

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I'm still not enthusiastic about duplicating the hash printing here.
> Whenever print_vectorlike changes, this has to be changed, too.

Nothing existing fits the needs here, since printing with prin1 can't be
controlled as much as we need.  I'm ok with leaving the code as is for
now, since both alternatives seem equally bad to me.

Michael.

bug#40704: 28.0.50; Improve and speed up (Gnus) registry saving

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> I'm still not enthusiastic about duplicating the hash printing here.
>> Whenever print_vectorlike changes, this has to be changed, too.
>
> Nothing existing fits the needs here, since printing with prin1 can't be
> controlled as much as we need.  I'm ok with leaving the code as is for
> now, since both alternatives seem equally bad to me.

I was wondering whether we could factor out the hash table bits from
print_vectorlike and reuse them here, somehow...

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

bug#40704: 28.0.50; Improve and speed up (Gnus) registry saving

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Any comments on the suggested changes?

This was almost two years ago, and I had some reservations, but the
speedups are significant, so I think you should just go ahead and push.

> +         (gc-cons-threshold (max gc-cons-threshold (* 800000 500)))

Except this bit -- I don't think we should rebind gc-cons-threshold --
it should be up to the user (and may have adverse effects in
memory-constrained setups).

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