bug#33275: 27.0.50; Image cache pruning

bug#33275: 27.0.50; Image cache pruning

[Lars Ingebrigtsen]

I was writing a function to do some image processing, and after it had
run a few minutes, the Emacs window disappeared.

After poking around a bit, it turned out that Emacs was killed by the
out-of-memory task.  I though that Emacs must have a serious memory leak
somewhere, but `garbage-collect' didn't really seem to have anything
mysterious in it.

So after trying various things, it turned out that you can kill Emacs by
doing this:

(dolist (file (directory-files "/directory/with/many/images" t "png$"))
  (image-size (create-image file) t))

and this is because `image-size' puts the image into the image cache.
Calling `clear-image-cache' releases the memory.

I think putting it into the cache is the right thing to do, because if
you're calling `image-size' on a file, then nine out of ten times you
want to display it, and my use case here is atypical (batch-processing
of a large number of huge images), but it's still not optimal that you
can crash Emacs in this way.

So perhaps the cache should be pruned more aggressively according to
some smart algorithm or other?  Or, at the very least, an
innocuous-looking function like `image-size' should note in the doc
string should note what's going on and recommend calling `image-flush'
afterwards if we don't want to cache the image?


In GNU Emacs 27.0.50 (build 50, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2018-08-26 built on stories
Repository revision: 5c642b2dc1b666ae488225b76251750a8cf331be
Windowing system distributor 'The X.Org Foundation', version 11.0.11902000
System Description: Debian GNU/Linux 9 (stretch)


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

bug#33275: 27.0.50; Image cache pruning

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 05 Nov 2018 15:07:30 +0100
> 
> So after trying various things, it turned out that you can kill Emacs by
> doing this:
> 
> (dolist (file (directory-files "/directory/with/many/images" t "png$"))
>   (image-size (create-image file) t))
> 
> and this is because `image-size' puts the image into the image cache.
> Calling `clear-image-cache' releases the memory.
> 
> I think putting it into the cache is the right thing to do, because if
> you're calling `image-size' on a file, then nine out of ten times you
> want to display it, and my use case here is atypical (batch-processing
> of a large number of huge images), but it's still not optimal that you
> can crash Emacs in this way.

Doesn't this mean that your system's VM is misconfigured?  How is it
Emacs's fault that you've run a program that gobs so much memory as to
trigger the OS's OOM killer?

> So perhaps the cache should be pruned more aggressively according to
> some smart algorithm or other?  Or, at the very least, an
> innocuous-looking function like `image-size' should note in the doc
> string should note what's going on and recommend calling `image-flush'
> afterwards if we don't want to cache the image?

I'm okay with saying something about this in the docs, but frankly,
you could do the same in any number of ways, like by visiting a file
whose size is larger than the VM you allow your system to use, right?
So there's a potential for causing such problems in many different
ways, and warning about that only in the doc string of image-size
seems insufficient, to say the least...

bug#33275: 27.0.50; Image cache pruning

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I'm okay with saying something about this in the docs, but frankly,
> you could do the same in any number of ways, like by visiting a file
> whose size is larger than the VM you allow your system to use, right?
> So there's a potential for causing such problems in many different
> ways, and warning about that only in the doc string of image-size
> seems insufficient, to say the least...

If you visit a, say, 16GB file, then, indeed, it's expected that Emacs
grows to 16GB (or more).  But if you're just calling `image-size' on a
bunch of files (each of which is a lot smaller than that), it's rather
unexpected that that's going to blow up Emacs.

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

bug#33275: 27.0.50; Image cache pruning

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 33275@debbugs.gnu.org
> Date: Mon, 05 Nov 2018 17:18:47 +0100
> 
> If you visit a, say, 16GB file, then, indeed, it's expected that Emacs
> grows to 16GB (or more).  But if you're just calling `image-size' on a
> bunch of files (each of which is a lot smaller than that), it's rather
> unexpected that that's going to blow up Emacs.

What was the sum of sizes of all the files you used to reproduce the
problem?

bug#33275: 27.0.50; Image cache pruning

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Cc: 33275@debbugs.gnu.org
>> Date: Mon, 05 Nov 2018 17:18:47 +0100
>> 
>> If you visit a, say, 16GB file, then, indeed, it's expected that Emacs
>> grows to 16GB (or more).  But if you're just calling `image-size' on a
>> bunch of files (each of which is a lot smaller than that), it's rather
>> unexpected that that's going to blow up Emacs.
>
> What was the sum of sizes of all the files you used to reproduce the
> problem?

I think it's sufficient to run Emacs over 2GB worth of .png files to
make Emacs grow to 16GB (which will kill it on this laptop).

But it'll vary on how well-compressed the .png files are, I guess,
because I think Emacs caches the, er, pixmaps and not the files?  I
don't remember.

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

bug#33275: 27.0.50; Image cache pruning

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 33275@debbugs.gnu.org
> Date: Mon, 05 Nov 2018 17:35:51 +0100
> 
> >> If you visit a, say, 16GB file, then, indeed, it's expected that Emacs
> >> grows to 16GB (or more).  But if you're just calling `image-size' on a
> >> bunch of files (each of which is a lot smaller than that), it's rather
> >> unexpected that that's going to blow up Emacs.
> >
> > What was the sum of sizes of all the files you used to reproduce the
> > problem?
> 
> I think it's sufficient to run Emacs over 2GB worth of .png files to
> make Emacs grow to 16GB (which will kill it on this laptop).
> 
> But it'll vary on how well-compressed the .png files are, I guess,
> because I think Emacs caches the, er, pixmaps and not the files?  I
> don't remember.

I think it's reasonable to expect 2GB worth of image files to take
several times that in memory.  (Btw, visiting a 16GB file usually
takes twice that much during insert-file-contents.)

I think we should try to add some code that would call
display_malloc_warning and/or memory_full, before the system runs out
of memory.  That should be enough to prevent OOM in such cases.  Can
you spot why we never called memory_full in this case?

bug#33275: 27.0.50; Image cache pruning

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I think it's reasonable to expect 2GB worth of image files to take
> several times that in memory.  (Btw, visiting a 16GB file usually
> takes twice that much during insert-file-contents.)

It would be reasonable if the code had asked for those images to be kept
in memory, but it hasn't.  Emacs decides that on its own without saying
that it's doing that.

It'd be similarly surprising if

(dolist (file (directory-files "/directory/with/many/images" t "png$"))
  (with-temp-buffer
    (insert-file-contents-literally file)))

were to lead to Emacs growing uncontrollably.

At present, the image cache is purely based on timestamps, if I recall
correctly?  If we wish to fix this unexpected behaviour, we could also
look at the size of the image cache and perhaps try to limit it, or if
we wanted to be really fancy, we could somehow check whether there's any
references to the images anywhere (beyond the image cache itself).

I think the latter would mean adding some... fun... to the gc system?

Or does Emacs have weak hashes?  In which case the image cache could be
a weak hash and the problem would resolve itself?

> I think we should try to add some code that would call
> display_malloc_warning and/or memory_full, before the system runs out
> of memory.  That should be enough to prevent OOM in such cases.  Can
> you spot why we never called memory_full in this case?

Sorry, I'm not familiar with the memory_full stuff...

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

bug#33275: 27.0.50; Image cache pruning

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Or does Emacs have weak hashes?  In which case the image cache could be
> a weak hash and the problem would resolve itself?

Emacs does have weak hashes...  Hm...

Anyway, there's code that tries to keep the cache small:

	  /* If the number of cached images has grown unusually large,
	     decrease the cache eviction delay (Bug#6230).  */
	  delay = XFIXNUM (Vimage_cache_eviction_delay);
	  if (nimages > 40)
	    delay = 1600 * delay / nimages / nimages;

But it works solely on the number of images in the cache, and not the
size of the images.  So it's a heuristic that could be tweaked pretty
easily, I think?

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

bug#33275: 27.0.50; Image cache pruning

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 33275@debbugs.gnu.org
> Date: Mon, 05 Nov 2018 18:48:33 +0100
> 
> Anyway, there's code that tries to keep the cache small:
> 
> 	  /* If the number of cached images has grown unusually large,
> 	     decrease the cache eviction delay (Bug#6230).  */
> 	  delay = XFIXNUM (Vimage_cache_eviction_delay);
> 	  if (nimages > 40)
> 	    delay = 1600 * delay / nimages / nimages;
> 
> But it works solely on the number of images in the cache, and not the
> size of the images.  So it's a heuristic that could be tweaked pretty
> easily, I think?

I don't see why we should tweak our cache management for the benefit
of exceptional use case.  Last time we did it (with fonts) we are
still leaking the wounds.

I think it should be enough to catch excess memory usage and signal an
error.  I hope this is possible.

bug#33275: 27.0.50; Image cache pruning

[Eli Zaretskii]

> Date: Mon, 05 Nov 2018 20:14:27 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 33275@debbugs.gnu.org
> 
> I don't see why we should tweak our cache management for the benefit
> of exceptional use case.  Last time we did it (with fonts) we are
> still leaking the wounds.
        ^^^^^^^
I meant "licking", of course ;-)

> I think it should be enough to catch excess memory usage and signal an
> error.  I hope this is possible.

I guess we lost the ability to do that automatically when we stopped
using vm-limit.c on modern platforms?

We still could use the memory limit calculated by get_lim_data in
strategic places, like cache_image, to catch situations where the
cache size exceeds some fraction of what ret_lim_data returns.  Would
that work?

bug#33275: 27.0.50; Image cache pruning

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> We still could use the memory limit calculated by get_lim_data in
> strategic places, like cache_image, to catch situations where the
> cache size exceeds some fraction of what ret_lim_data returns.  Would
> that work?

If I understand you correctly, you seem to favour introducing code that
will break code that's working today (if you insert an image into a
buffer today in a memory-pressure situation, that won't signal an error
today) instead of tweaking a non-documented ad-hoc caching strategy (by
taking the image cache size into consideration), or fixing the caching
(by using weak hash tables), and I'm not quite sure why.

`image-size' didn't use to cache images, but was introduced as an
optimisation.

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

bug#33275: 27.0.50; Image cache pruning

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 33275@debbugs.gnu.org
> Date: Mon, 05 Nov 2018 20:13:27 +0100
> 
> If I understand you correctly, you seem to favour introducing code that
> will break code that's working today (if you insert an image into a
> buffer today in a memory-pressure situation, that won't signal an error
> today) instead of tweaking a non-documented ad-hoc caching strategy (by
> taking the image cache size into consideration), or fixing the caching
> (by using weak hash tables), and I'm not quite sure why.

Because your use case is not important enough to change image caching
strategy that works well for many years.

> `image-size' didn't use to cache images, but was introduced as an
> optimisation.

And that optimization had a reason, right?

Btw, why doesn't xmalloc return NULL in your case before invoking the
OOM killer?  Is that expected behavior on GNU/Linux systems?

bug#33275: 27.0.50; Image cache pruning

[Lars Ingebrigtsen]

(Sorry for the late response; things got in the way as usual...)

Eli Zaretskii <eliz@gnu.org> writes:

>> `image-size' didn't use to cache images, but was introduced as an
>> optimisation.
>
> And that optimization had a reason, right?

Yeah, the normal use case when you're doing images is that you're going
to end up displaying it.  And `image-size' has to render the image to
see how big it is, so during normal usage, having `image-size' cache the
image is a big win.

Hm...  Perhaps `image-size' should just have a DONT-CACHE optional
parameter?

> Btw, why doesn't xmalloc return NULL in your case before invoking the
> OOM killer?  Is that expected behavior on GNU/Linux systems?

No, you've got overcommit on Linux, so you often don't get a NULL when
you need it...

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

bug#33275: 27.0.50; Image cache pruning

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 33275@debbugs.gnu.org
> Date: Mon, 05 Nov 2018 18:48:33 +0100
> 
> At present, the image cache is purely based on timestamps, if I recall
> correctly?  If we wish to fix this unexpected behaviour, we could also
> look at the size of the image cache and perhaps try to limit it, or if
> we wanted to be really fancy, we could somehow check whether there's any
> references to the images anywhere (beyond the image cache itself).
> 
> I think the latter would mean adding some... fun... to the gc system?
> 
> Or does Emacs have weak hashes?  In which case the image cache could be
> a weak hash and the problem would resolve itself?
> [...]
> Emacs does have weak hashes...  Hm...
> 
> Anyway, there's code that tries to keep the cache small:
> 
>           /* If the number of cached images has grown unusually large,
>              decrease the cache eviction delay (Bug#6230).  */
>           delay = XFIXNUM (Vimage_cache_eviction_delay);
>           if (nimages > 40)
>             delay = 1600 * delay / nimages / nimages;
> 
> But it works solely on the number of images in the cache, and not the
> size of the images.  So it's a heuristic that could be tweaked pretty
> easily, I think?

After thinking about this and looking at the code, I don't think I
understand your proposal(s).  Here are the issues that I saw with what
I think you proposed:

 . The hash table used by the image cache is not a Lisp hash-table, so
   weakness doesn't apply.  We cache pointers to C structs, so using a
   Lisp hash-tables would need "some work".
 . The code you show above is called from redisplay, so it will not be
   executed as long as a Lisp program (such as the one you show at the
   beginning of this discussion) runs, and you will still have the OOM
   killer get you before that code get a chance to clear the cache.
 . Running such code from GC could be tricky, because freeing images
   needs to remove references of those images from the glyph matrices,
   and that cannot be safely done from arbitrary places.
 . I don't think you can ensure GC will run during your program
   anyway, because most of the memory allocated by your program is not
   used to cons Lisp objects, so maybe_gc called by the Lisp
   interpreter will most probably decide there's no need for GC and do
   nothing.  Therefore, I don't think that avoiding to cache the
   images would prevent the OOM situation in your case.

I think if we want to prevent the OOM killer from killing Emacs due to
many cached images, we should inspect inside cache_image the current
VM size of the process (like system_process_attributes does), compare
it to the memory limits, and display a warning and/or signal an error
when we get dangerously close to the limit.  If we want to avoid
similar situations with other objects, we could do these tests inside
xmalloc and mmap_alloc/mmap_realloc.  (We used to do something similar
in the past, but that code needed a libc hook for memory allocation
from the OS, something I understand we cannot rely on now.)

IOW, I don't see how we could prevent being OOM-killed, except if we
monitor the committed memory at the point where it is allocated, as
otherwise the opportunistic memory allocation strategy can always trip
us.

bug#33275: 27.0.50; Image cache pruning

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> After thinking about this and looking at the code, I don't think I
> understand your proposal(s).  Here are the issues that I saw with what
> I think you proposed:
>
>  . The hash table used by the image cache is not a Lisp hash-table, so
>    weakness doesn't apply.  We cache pointers to C structs, so using a
>    Lisp hash-tables would need "some work".

Yeah, it'd have to be rewritten substantially to use Lisp hash tables.

>  . The code you show above is called from redisplay, so it will not be
>    executed as long as a Lisp program (such as the one you show at the
>    beginning of this discussion) runs, and you will still have the OOM
>    killer get you before that code get a chance to clear the cache.

That's true, but calling the cache pruning function from the function
that enters images into the cache would be trivial, I think.

>  . Running such code from GC could be tricky, because freeing images
>    needs to remove references of those images from the glyph matrices,
>    and that cannot be safely done from arbitrary places.

Ah, I see.  Yes, that's a major complication...

> I think if we want to prevent the OOM killer from killing Emacs due to
> many cached images, we should inspect inside cache_image the current
> VM size of the process (like system_process_attributes does), compare
> it to the memory limits, and display a warning and/or signal an error
> when we get dangerously close to the limit.

I think that sounds like a good idea, yes.  Having such a check is good
for Emacs sanity, but we should also have a way for programmers to avoid
ending up in that pathological situation to begin with.

So either a DONT-CACHE parameter to `image-size' or just a docstring
thing.  :-)

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

bug#33275: 27.0.50; Image cache pruning

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 33275@debbugs.gnu.org
> Date: Fri, 09 Nov 2018 13:26:50 +0100
> 
> So either a DONT-CACHE parameter to `image-size' or just a docstring
> thing.  :-)

How about both?

bug#33275: 27.0.50; Image cache pruning

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> So either a DONT-CACHE parameter to `image-size' or just a docstring
>> thing.  :-)
>
> How about both?

DONT-CACHE seems unnecessary since `image-flush' exists, so I've just
added a reference to that function in the `image-size' function.  It
should point people who need this towards doing something like

(dolist (file (directory-files-recursively "~/pics" "jpg$"))
  (let ((spec (create-image file)))
    (image-size spec t)
    (image-flush spec)))


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

bug#33275: 27.0.50; Image cache pruning

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 33275@debbugs.gnu.org
> Date: Mon, 05 Nov 2018 18:36:39 +0100
> 
> It would be reasonable if the code had asked for those images to be kept
> in memory, but it hasn't.  Emacs decides that on its own without saying
> that it's doing that.

You cannot assume anything about memory management while a Lisp
program runs.

> It'd be similarly surprising if
> 
> (dolist (file (directory-files "/directory/with/many/images" t "png$"))
>   (with-temp-buffer
>     (insert-file-contents-literally file)))
> 
> were to lead to Emacs growing uncontrollably.

That it doesn't is just sheer luck: the way we manage buffer memory is
special.  With any other Lisp object, it could well grow
uncontrollably.  Our way of keeping that in check is to signal
memory-full error when we are about to run out of memory.  Not sure
yet why this doesn't happen in your original recipe.

> > I think we should try to add some code that would call
> > display_malloc_warning and/or memory_full, before the system runs out
> > of memory.  That should be enough to prevent OOM in such cases.  Can
> > you spot why we never called memory_full in this case?
> 
> Sorry, I'm not familiar with the memory_full stuff...

OK, I will try to look into this soon.

bug#33275: 27.0.50; Image cache pruning

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> It would be reasonable if the code had asked for those images to be kept
>> in memory, but it hasn't.  Emacs decides that on its own without saying
>> that it's doing that.
>
> You cannot assume anything about memory management while a Lisp
> program runs.

In general, if you don't have a reference to an object, you can be
pretty sure that Emacs is going to garbage-collect it.  If you can't
assume that, then programming in Emacs Lisp becomes impossible.

Fortunately that's not the case, and programming in Emacs Lisp is nice
and easy.

>> It'd be similarly surprising if
>> 
>> (dolist (file (directory-files "/directory/with/many/images" t "png$"))
>>   (with-temp-buffer
>>     (insert-file-contents-literally file)))
>> 
>> were to lead to Emacs growing uncontrollably.
>
> That it doesn't is just sheer luck: the way we manage buffer memory is
> special.  With any other Lisp object, it could well grow
> uncontrollably.

What other non-referenced Lisp object can realistically make Emacs grow
in this way?

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

bug#33275: 27.0.50; Image cache pruning

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 33275@debbugs.gnu.org
> Date: Mon, 05 Nov 2018 20:06:41 +0100
> 
> In general, if you don't have a reference to an object, you can be
> pretty sure that Emacs is going to garbage-collect it.

Of course.  But you cannot control when that GC will happen.

> > That it doesn't is just sheer luck: the way we manage buffer memory is
> > special.  With any other Lisp object, it could well grow
> > uncontrollably.
> 
> What other non-referenced Lisp object can realistically make Emacs grow
> in this way?

Theoretically, anything.  In practice, I've seen such situations with
fonts.