Re: Debugging emacs memory management

[Dima Kogan]

Eli Zaretskii <address@hidden> writes:

>> From: Dima Kogan <address@hidden>
>> Cc: Eli Zaretskii <address@hidden>, address@hidden, address@hidden
>> Date: Mon, 05 Oct 2015 03:02:16 -0700
>> 
>> Andreas Schwab <address@hidden> writes:
>> > Dima Kogan <address@hidden> writes:
>> >
>> >> diff --git a/src/font.c b/src/font.c
>> >> index 8e06532..ca872d0 100644
>> >> --- a/src/font.c
>> >> +++ b/src/font.c
>> >> @@ -3981,7 +3981,15 @@ copy_font_spec (Lisp_Object font)
>> >>    pcdr = spec->props + FONT_EXTRA_INDEX;
>> >>    for (tail = AREF (font, FONT_EXTRA_INDEX); CONSP (tail); tail = XCDR 
>> >> (tail))
>> >>      if (!EQ (XCAR (XCAR (tail)), QCfont_entity))
>> >> -      *pcdr = Fcons (XCAR (tail), Qnil), pcdr = xcdr_addr (*pcdr);
>> >> +      {
>> >> +        if (CONSP (XCAR (tail)))
>> >
>> > This better be always true, otherwise XCAR (XCAR (tail)) would be a bug.
>> 
>> It isn't. Without that check I was getting every time.
>> 
>>   *ERROR*: Wrong type argument: listp, "monospace-10"
>
> I think what Andreas meant was this: the old code used
> XCAR (XCAR (tail)) unconditionally, which seems to indicate that
> XCAR (tail) is always a cons cell, and your test should always
> yield true.

I just looked at it again, and it was my mistake; this is always a cons
cell as it should be. I was getting the error when using Fcopy_sequence
instead of making a new cell with Fcons. The former would make a deeper
copy, but the alist entries aren't lists, so Fcopy_sequence() was
barfing. Making a new cons-cell is one-level deep, and it's deep-enough,
so it works. So yeah, my bad.


>> Surprised me too, and I haven't checked to see what was happening there.
>
> So maybe your code should include some check for QCfont_entity in the
> 'else' clause as well.

Here's the updated patch:

diff --git a/src/font.c b/src/font.c
index 8e06532..dd574ca 100644
--- a/src/font.c
+++ b/src/font.c
@@ -3981,7 +3981,12 @@ copy_font_spec (Lisp_Object font)
   pcdr = spec->props + FONT_EXTRA_INDEX;
   for (tail = AREF (font, FONT_EXTRA_INDEX); CONSP (tail); tail = XCDR (tail))
     if (!EQ (XCAR (XCAR (tail)), QCfont_entity))
-      *pcdr = Fcons (XCAR (tail), Qnil), pcdr = xcdr_addr (*pcdr);
+      {
+        *pcdr = Fcons (Fcons( XCAR (XCAR (tail)),
+                              XCDR (XCAR (tail))),
+                       Qnil);
+        pcdr = xcdr_addr (*pcdr);
+      }
 
   XSETFONT (new_spec, spec);
   return new_spec;

I'm not sure what you mean about checking for QCfont_entity. This new
patch doesn't add an if(), and there's already a QCfont_entity check
there. This is fine, right?


> Btw, isn't 12KB per frame still a lot?

Yes, I think so. There are more leaks to find.