bug#42482: 27.0.91; emacs modules memory leak

bug#42482: 27.0.91; emacs modules memory leak

[Milan Stanojević]

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

env-make_global_ref adds a reference to the underlying Lisp_Object
and allocates emacs_value from the global storage. env->free_global_ref
on the other hand will only remove a reference to the underlying
Lisp_Object and not free the emacs_value.

Here is a simple recipe to reproduce the problem (I only tested this
on linux). I'm attaching the necessary files.

$ gcc -shared -fpic -std=c99 -I <dir-with-emacs-module.h>
create_global_refs.c -o create_global_refs.so
$ emacs --no-splash -q -l create_global_refs.so -l create_global_refs_test.el

If you look at the memory usage of emacs (for example in htop) you'll
see that with emacs-26 it is constant but with emacs-27 the resident
memory quickly grows.

[-- Attachment #2: create_global_refs.c --]
[-- Type: text/x-csrc, Size: 1053 bytes --]

#include <string.h>
#include <emacs-module.h>

int plugin_is_GPL_compatible;

static emacs_value create_loop (emacs_env *env, ptrdiff_t _nargs, emacs_value _args[], void* _data)
{
  for (int i = 0; i < 1000000; i++)
    env->free_global_ref
      (env,
       env->make_global_ref
       (env,
        env->make_integer(env, 0)));

  char msg[] = "1M global references created and freed";
  emacs_value message = { env->make_string(env, msg, strlen(msg)) };
  env->funcall(env, env->intern(env, "message"), 1, &message);

  return env->intern(env, "nil");
}

int emacs_module_init (struct emacs_runtime *ert) {
  emacs_env* env = ert->get_environment(ert);

  emacs_value loopf = env->make_function(env, 0, 0, create_loop, "create and free 1M refs", NULL);

  emacs_value args[] = { env->intern(env, "create-refs-loop"), loopf };
  env->funcall(env, env->intern(env, "fset"), 2, args);

  emacs_value simple_loop_feature = env->intern(env, "create-global-refs");
  env->funcall(env, env->intern(env, "provide"), 1, &simple_loop_feature);

  return 0;
}

[-- Attachment #3: create_global_refs_test.el --]
[-- Type: text/x-emacs-lisp, Size: 211 bytes --]

(require 'create-global-refs)

(defun run-one-iteration-and-gc ()
  (create-refs-loop)
  (garbage-collect))

(switch-to-buffer (get-buffer-create "*Messages*"))

(run-with-timer 0.1 1 'run-one-iteration-and-gc)

bug#42482: 27.0.91; emacs modules memory leak

[Philipp Stephani]

Am Do., 23. Juli 2020 um 01:27 Uhr schrieb Milan Stanojević
<mstanojevic@janestreet.com>:
>
> env-make_global_ref adds a reference to the underlying Lisp_Object
> and allocates emacs_value from the global storage. env->free_global_ref
> on the other hand will only remove a reference to the underlying
> Lisp_Object and not free the emacs_value.
>
> Here is a simple recipe to reproduce the problem (I only tested this
> on linux). I'm attaching the necessary files.
>
> $ gcc -shared -fpic -std=c99 -I <dir-with-emacs-module.h>
> create_global_refs.c -o create_global_refs.so
> $ emacs --no-splash -q -l create_global_refs.so -l create_global_refs_test.el
>
> If you look at the memory usage of emacs (for example in htop) you'll
> see that with emacs-26 it is constant but with emacs-27 the resident
> memory quickly grows.

Thanks for the report. I've fixed this in commit
5c5eb9790898e4ab10bcbbdb6871947ed3018569; the fix is slightly
different from what you proposed in that it stores the emacs_value
object in the global references hashtable, but it should have the same
effect. At least I can't reproduce the symptom any more after that
commit.

bug#42482: 27.0.91; emacs modules memory leak

[Milan Stanojević]

Thank you for the quick fix.
Is there a chance this also goes to emacs-27 branch so it can be in
the emacs 27.1 when it gets released?

On Thu, Jul 23, 2020 at 8:07 AM Philipp Stephani <p.stephani2@gmail.com> wrote:
>
> Am Do., 23. Juli 2020 um 01:27 Uhr schrieb Milan Stanojević
> <mstanojevic@janestreet.com>:
> >
> > env-make_global_ref adds a reference to the underlying Lisp_Object
> > and allocates emacs_value from the global storage. env->free_global_ref
> > on the other hand will only remove a reference to the underlying
> > Lisp_Object and not free the emacs_value.
> >
> > Here is a simple recipe to reproduce the problem (I only tested this
> > on linux). I'm attaching the necessary files.
> >
> > $ gcc -shared -fpic -std=c99 -I <dir-with-emacs-module.h>
> > create_global_refs.c -o create_global_refs.so
> > $ emacs --no-splash -q -l create_global_refs.so -l create_global_refs_test.el
> >
> > If you look at the memory usage of emacs (for example in htop) you'll
> > see that with emacs-26 it is constant but with emacs-27 the resident
> > memory quickly grows.
>
> Thanks for the report. I've fixed this in commit
> 5c5eb9790898e4ab10bcbbdb6871947ed3018569; the fix is slightly
> different from what you proposed in that it stores the emacs_value
> object in the global references hashtable, but it should have the same
> effect. At least I can't reproduce the symptom any more after that
> commit.

bug#42482: 27.0.91; emacs modules memory leak

[Philipp Stephani]

Am Do., 23. Juli 2020 um 16:29 Uhr schrieb Milan Stanojević
<mstanojevic@janestreet.com>:
>
> Thank you for the quick fix.
> Is there a chance this also goes to emacs-27 branch so it can be in
> the emacs 27.1 when it gets released?

I think backporting the fix should be fine, as the fix is rather
localized and fixes a regression. Eli?

>
> On Thu, Jul 23, 2020 at 8:07 AM Philipp Stephani <p.stephani2@gmail.com> wrote:
> >
> > Am Do., 23. Juli 2020 um 01:27 Uhr schrieb Milan Stanojević
> > <mstanojevic@janestreet.com>:
> > >
> > > env-make_global_ref adds a reference to the underlying Lisp_Object
> > > and allocates emacs_value from the global storage. env->free_global_ref
> > > on the other hand will only remove a reference to the underlying
> > > Lisp_Object and not free the emacs_value.
> > >
> > > Here is a simple recipe to reproduce the problem (I only tested this
> > > on linux). I'm attaching the necessary files.
> > >
> > > $ gcc -shared -fpic -std=c99 -I <dir-with-emacs-module.h>
> > > create_global_refs.c -o create_global_refs.so
> > > $ emacs --no-splash -q -l create_global_refs.so -l create_global_refs_test.el
> > >
> > > If you look at the memory usage of emacs (for example in htop) you'll
> > > see that with emacs-26 it is constant but with emacs-27 the resident
> > > memory quickly grows.
> >
> > Thanks for the report. I've fixed this in commit
> > 5c5eb9790898e4ab10bcbbdb6871947ed3018569; the fix is slightly
> > different from what you proposed in that it stores the emacs_value
> > object in the global references hashtable, but it should have the same
> > effect. At least I can't reproduce the symptom any more after that
> > commit.

bug#42482: 27.0.91; emacs modules memory leak

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Thu, 23 Jul 2020 16:33:12 +0200
> Cc: 42482-done@debbugs.gnu.org
> 
> Am Do., 23. Juli 2020 um 16:29 Uhr schrieb Milan Stanojević
> <mstanojevic@janestreet.com>:
> >
> > Thank you for the quick fix.
> > Is there a chance this also goes to emacs-27 branch so it can be in
> > the emacs 27.1 when it gets released?
> 
> I think backporting the fix should be fine, as the fix is rather
> localized and fixes a regression. Eli?

How well was it tested?  The change is not exactly trivial.  But if
you are satisfied with the testing enough to have this in emacs-27,
I'm okay with that.

Thanks.

bug#42482: 27.0.91; emacs modules memory leak

[Milan Stanojević]

I backported the change to emacs-27 branch locally and tried a simple
test using our real-world emacs module against it and the change seems
to fix the memory leak. Our module (and the test I tried) is written
in ocaml (calls to/from emacs are via C bindings) and relies on
interplay between ocaml and emacs gcs to not leak memory, so that
working is a good sign.

Compared to emacs-26, it uses a bit more memory, but I think that is
expected. The main thing is that memory is stable.









On Thu, Jul 23, 2020 at 1:45 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Thu, 23 Jul 2020 16:33:12 +0200
> > Cc: 42482-done@debbugs.gnu.org
> >
> > Am Do., 23. Juli 2020 um 16:29 Uhr schrieb Milan Stanojević
> > <mstanojevic@janestreet.com>:
> > >
> > > Thank you for the quick fix.
> > > Is there a chance this also goes to emacs-27 branch so it can be in
> > > the emacs 27.1 when it gets released?
> >
> > I think backporting the fix should be fine, as the fix is rather
> > localized and fixes a regression. Eli?
>
> How well was it tested?  The change is not exactly trivial.  But if
> you are satisfied with the testing enough to have this in emacs-27,
> I'm okay with that.
>
> Thanks.

bug#42482: 27.0.91; emacs modules memory leak

[Philipp Stephani]

Am Do., 23. Juli 2020 um 19:45 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Thu, 23 Jul 2020 16:33:12 +0200
> > Cc: 42482-done@debbugs.gnu.org
> >
> > Am Do., 23. Juli 2020 um 16:29 Uhr schrieb Milan Stanojević
> > <mstanojevic@janestreet.com>:
> > >
> > > Thank you for the quick fix.
> > > Is there a chance this also goes to emacs-27 branch so it can be in
> > > the emacs 27.1 when it gets released?
> >
> > I think backporting the fix should be fine, as the fix is rather
> > localized and fixes a regression. Eli?
>
> How well was it tested?  The change is not exactly trivial.  But if
> you are satisfied with the testing enough to have this in emacs-27,
> I'm okay with that.

I'd like to have a few more test cases around global references in
emacs-27, as the current test cases only test some simple/success
cases, and we'd probably want to test at least a few more edge cases
(e.g. freeing global references in a different order than allocating
them). I've added two more test cases on master and will see that I
can add a few more in the coming days.

bug#42482: 27.0.91; emacs modules memory leak

[Philipp Stephani]

Am Sa., 25. Juli 2020 um 23:43 Uhr schrieb Philipp Stephani
<p.stephani2@gmail.com>:
>
> Am Do., 23. Juli 2020 um 19:45 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
> >
> > > From: Philipp Stephani <p.stephani2@gmail.com>
> > > Date: Thu, 23 Jul 2020 16:33:12 +0200
> > > Cc: 42482-done@debbugs.gnu.org
> > >
> > > Am Do., 23. Juli 2020 um 16:29 Uhr schrieb Milan Stanojević
> > > <mstanojevic@janestreet.com>:
> > > >
> > > > Thank you for the quick fix.
> > > > Is there a chance this also goes to emacs-27 branch so it can be in
> > > > the emacs 27.1 when it gets released?
> > >
> > > I think backporting the fix should be fine, as the fix is rather
> > > localized and fixes a regression. Eli?
> >
> > How well was it tested?  The change is not exactly trivial.  But if
> > you are satisfied with the testing enough to have this in emacs-27,
> > I'm okay with that.
>
> I'd like to have a few more test cases around global references in
> emacs-27, as the current test cases only test some simple/success
> cases, and we'd probably want to test at least a few more edge cases
> (e.g. freeing global references in a different order than allocating
> them). I've added two more test cases on master and will see that I
> can add a few more in the coming days.

I've now added a few more tests and backported the fix to the release branch.

bug#42482: 27.0.91; emacs modules memory leak

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sat, 1 Aug 2020 14:11:54 +0200
> Cc: Milan Stanojević <mstanojevic@janestreet.com>, 
> 	42482@debbugs.gnu.org
> 
> I've now added a few more tests and backported the fix to the release branch.

I think it's too late for Emacs 27.1, sorry.  The tarball is already
made.

bug#42482: 27.0.91; emacs modules memory leak

[Nicolas Petton]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> I've now added a few more tests and backported the fix to the release branch.
>
> I think it's too late for Emacs 27.1, sorry.  The tarball is already
> made.

If the bugfix is important enough, I can build a new RC tomorrow and set
the release date to the 8th of Aug.

Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

bug#42482: 27.0.91; emacs modules memory leak

[Philipp Stephani]

Am Sa., 1. Aug. 2020 um 14:55 Uhr schrieb Nicolas Petton <nico@petton.fr>:
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> >> I've now added a few more tests and backported the fix to the release branch.
> >
> > I think it's too late for Emacs 27.1, sorry.  The tarball is already
> > made.
>
> If the bugfix is important enough, I can build a new RC tomorrow and set
> the release date to the 8th of Aug.


I don't feel strongly either way. My expectation for global references
is that they should mainly be used for the module equivalent of defvar
and friends, in which case the bug isn't that severe because nobody
needs millions of defvars. OTOH, people might use global references
for other means, including allocating them dynamically, in which case
leaking them can become annoying.

bug#42482: 27.0.91; emacs modules memory leak

[Eli Zaretskii]

> From: Nicolas Petton <nico@petton.fr>
> Cc: 42482@debbugs.gnu.org, mstanojevic@janestreet.com
> Date: Sat, 01 Aug 2020 14:55:33 +0200
> 
> > I think it's too late for Emacs 27.1, sorry.  The tarball is already
> > made.
> 
> If the bugfix is important enough, I can build a new RC tomorrow and set
> the release date to the 8th of Aug.

Unfortunately, a much more serious bug was reported and fixed today,
so we will need a new RC from the latest emacs-27 branch.

Thanks.

bug#42482: 27.0.91; emacs modules memory leak

[Milan Stanojević]

I guess this fix will be included now in 27.1 but I just wanted to say
that OCaml emacs module library (https://github.com/janestreet/ecaml)
uses global refs everywhere so the leak would be very noticeable (that
is how we found out in the first place).

I'm not sure this would have been enough to postpone the release date
since I don't think ecaml has that many users (We do have 500+ devs
using and relying on ecaml but it is really a single emacs+ecaml
installation and we can patch emacs sources or just wait for 27.2)



On Sun, Aug 2, 2020 at 1:26 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Nicolas Petton <nico@petton.fr>
> > Cc: 42482@debbugs.gnu.org, mstanojevic@janestreet.com
> > Date: Sat, 01 Aug 2020 14:55:33 +0200
> >
> > > I think it's too late for Emacs 27.1, sorry.  The tarball is already
> > > made.
> >
> > If the bugfix is important enough, I can build a new RC tomorrow and set
> > the release date to the 8th of Aug.
>
> Unfortunately, a much more serious bug was reported and fixed today,
> so we will need a new RC from the latest emacs-27 branch.
>
> Thanks.

bug#42482: 27.0.91; emacs modules memory leak

[Nicolas Petton]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> If the bugfix is important enough, I can build a new RC tomorrow and set
>> the release date to the 8th of Aug.
>
> Unfortunately, a much more serious bug was reported and fixed today,
> so we will need a new RC from the latest emacs-27 branch.

That's ok :)

I'll make a new one this evening.

Nico

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]