bug#21028: Performance regression in revision af1a69f4d17a482c359d98c00ef86fac835b5fac (Apr 2014).

["Clément Pit--Claudel" <clement.pitclaudel@live.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2994 bytes --]

Hi all,

I've spent a bit more time looking into this. The problem is due to the following three lines being conditioned on `!NILP(val)`: (that's around line 2780 of src/font.c)

    Lisp_Object copy = copy_font_spec (scratch_font_spec);
    ASET (copy, FONT_TYPE_INDEX, driver_list->driver->type);
    XSETCDR (cache, Fcons (Fcons (copy, val), XCDR (cache)));

Before af1a69f, these lines were always run. In af1a69f, they only run if `val` is non nil, causing the regression: on master, moving them back out of the if statement fixes the problem. For clarity I have attached the corresponding patch.

Clément.

On 07/18/2015 03:24 AM, Clément Pit--Claudel wrote:
> On 07/15/2015 05:32 AM, Dmitry Antipov wrote:
>> On 07/10/2015 07:55 PM, Clément Pit--Claudel wrote:
>>
>>> The figures are very similar to the tests above: with that patch inserting 50 lines takes 3 seconds,
>>> and without it it's instantaneous. Thus I think it's safe to say that this particular patch is indeed
>>> responsible for the performance regression. But maybe I'm missing something?
>>
>> As of c40ea1328bb33abaec14f1fc92ac2349b5ee2715, I can't reproduce this issue, with your fontset setup
>> from http://debbugs.gnu.org/cgi/bugreport.cgi?bug=21028#17. Cursor motion and keyboard/mouse selection
>> are smooth, and CPU/memory usage looks normal.
> 
> Thanks for trying this out. Thanks for your suggestions, too.
> 
>> My suggestions are:
>>
>> 1) Re-run your timed tests from http://debbugs.gnu.org/cgi/bugreport.cgi?bug=21028#20 under /usr/bin/time,
>> not time (the latter is a simple shell builtin). /usr/bin/time also shows memory usage; if "bad" (current)
>> instance consumes more memory than "good" (with reverted change) one, there may be nasty GC issue.
> 
> The timings are similar:
> 
> $ /usr/bin/time emacs-bad/src/emacs -Q --eval "(progn (dotimes (_ 3) (set-fontset-font t 'unicode (font-spec :name \"Arial\") nil 'append)) (dotimes (_ 500) (insert \"日本国\n\")) (dotimes (_ 500) (previous-line) (redisplay t)) (kill-emacs))"
> 13.31user 8.57system 0:34.54elapsed 63%CPU (0avgtext+0avgdata 50592maxresident)k
> 0inputs+0outputs (0major+4958minor)pagefaults 0swaps
> 
> $ /usr/bin/time emacs-good/src/emacs -Q --eval "(progn (dotimes (_ 3) (set-fontset-font t 'unicode (font-spec :name \"Arial\") nil 'append)) (dotimes (_ 500) (insert \"日本国\n\")) (dotimes (_ 500) (previous-line) (redisplay t)) (kill-emacs))"
> 0.58user 0.03system 0:01.05elapsed 58%CPU (0avgtext+0avgdata 50392maxresident)k
> 29840inputs+0outputs (105major+4911minor)pagefaults 0swaps
> 
>> 2) 3 seconds is large enough to leave the traces in profiled runs. On GNU/Linux, it may be worth trying
>> to run under perf, both "good" and "bad" cases, and comparing reports.
> 
> I have attached the reports in the good and bad cases (200c532 and af1a69f respectively). I'm not sure what to conclude from them; I can provide the full trace data if needed.
> 
> Clément.
> 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Draft-of-a-fix-for-21028.patch --]
[-- Type: text/x-diff; name="0001-Draft-of-a-fix-for-21028.patch", Size: 1128 bytes --]

From c0c58994b1aa78289884525e4e3ae8b8e64d0c08 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Cl=C3=A9ment=20Pit--Claudel?= <clement.pitclaudel@live.com>
Date: Sat, 18 Jul 2015 04:13:07 -0700
Subject: [PATCH] Draft of a fix for #21028

---
 src/font.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/font.c b/src/font.c
index 50b966e..5cdde71 100644
--- a/src/font.c
+++ b/src/font.c
@@ -2779,12 +2779,12 @@ font_list_entities (struct frame *f, Lisp_Object spec)
 	    val = driver_list->driver->list (f, scratch_font_spec);
 	    if (!NILP (val))
 	      {
-		Lisp_Object copy = copy_font_spec (scratch_font_spec);
-
 		val = Fvconcat (1, &val);
-		ASET (copy, FONT_TYPE_INDEX, driver_list->driver->type);
-		XSETCDR (cache, Fcons (Fcons (copy, val), XCDR (cache)));
 	      }
+
+            Lisp_Object copy = copy_font_spec (scratch_font_spec);
+            ASET (copy, FONT_TYPE_INDEX, driver_list->driver->type);
+            XSETCDR (cache, Fcons (Fcons (copy, val), XCDR (cache)));
 	  }
 	if (VECTORP (val) && ASIZE (val) > 0
 	    && (need_filtering
-- 
2.4.6


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]