unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#42482: 27.0.91; emacs modules memory leak
@ 2020-07-22 23:25 Milan Stanojević
  2020-07-23 12:06 ` Philipp Stephani
  0 siblings, 1 reply; 14+ messages in thread
From: Milan Stanojević @ 2020-07-22 23:25 UTC (permalink / raw)
  To: 42482

[-- 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)

^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#42482: 27.0.91; emacs modules memory leak
  2020-07-22 23:25 bug#42482: 27.0.91; emacs modules memory leak Milan Stanojević
@ 2020-07-23 12:06 ` Philipp Stephani
  2020-07-23 14:29   ` Milan Stanojević
  0 siblings, 1 reply; 14+ messages in thread
From: Philipp Stephani @ 2020-07-23 12:06 UTC (permalink / raw)
  To: Milan Stanojević; +Cc: 42482-done

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.





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#42482: 27.0.91; emacs modules memory leak
  2020-07-23 12:06 ` Philipp Stephani
@ 2020-07-23 14:29   ` Milan Stanojević
  2020-07-23 14:33     ` Philipp Stephani
  0 siblings, 1 reply; 14+ messages in thread
From: Milan Stanojević @ 2020-07-23 14:29 UTC (permalink / raw)
  To: Philipp Stephani; +Cc: 42482-done

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.





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#42482: 27.0.91; emacs modules memory leak
  2020-07-23 14:29   ` Milan Stanojević
@ 2020-07-23 14:33     ` Philipp Stephani
  2020-07-23 17:45       ` Eli Zaretskii
  0 siblings, 1 reply; 14+ messages in thread
From: Philipp Stephani @ 2020-07-23 14:33 UTC (permalink / raw)
  To: Milan Stanojević; +Cc: 42482-done

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.





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#42482: 27.0.91; emacs modules memory leak
  2020-07-23 14:33     ` Philipp Stephani
@ 2020-07-23 17:45       ` Eli Zaretskii
  2020-07-23 19:11         ` Milan Stanojević
  2020-07-25 21:43         ` Philipp Stephani
  0 siblings, 2 replies; 14+ messages in thread
From: Eli Zaretskii @ 2020-07-23 17:45 UTC (permalink / raw)
  To: Philipp Stephani; +Cc: 42482, mstanojevic

> 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.





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#42482: 27.0.91; emacs modules memory leak
  2020-07-23 17:45       ` Eli Zaretskii
@ 2020-07-23 19:11         ` Milan Stanojević
  2020-07-25 21:43         ` Philipp Stephani
  1 sibling, 0 replies; 14+ messages in thread
From: Milan Stanojević @ 2020-07-23 19:11 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Philipp Stephani, 42482

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.





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#42482: 27.0.91; emacs modules memory leak
  2020-07-23 17:45       ` Eli Zaretskii
  2020-07-23 19:11         ` Milan Stanojević
@ 2020-07-25 21:43         ` Philipp Stephani
  2020-08-01 12:11           ` Philipp Stephani
  1 sibling, 1 reply; 14+ messages in thread
From: Philipp Stephani @ 2020-07-25 21:43 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 42482, Milan Stanojević

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.





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#42482: 27.0.91; emacs modules memory leak
  2020-07-25 21:43         ` Philipp Stephani
@ 2020-08-01 12:11           ` Philipp Stephani
  2020-08-01 12:37             ` Eli Zaretskii
  0 siblings, 1 reply; 14+ messages in thread
From: Philipp Stephani @ 2020-08-01 12:11 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 42482, Milan Stanojević

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.





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#42482: 27.0.91; emacs modules memory leak
  2020-08-01 12:11           ` Philipp Stephani
@ 2020-08-01 12:37             ` Eli Zaretskii
  2020-08-01 12:55               ` Nicolas Petton
  0 siblings, 1 reply; 14+ messages in thread
From: Eli Zaretskii @ 2020-08-01 12:37 UTC (permalink / raw)
  To: Philipp Stephani; +Cc: 42482, mstanojevic

> 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.





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#42482: 27.0.91; emacs modules memory leak
  2020-08-01 12:37             ` Eli Zaretskii
@ 2020-08-01 12:55               ` Nicolas Petton
  2020-08-01 14:47                 ` Philipp Stephani
  2020-08-02 17:25                 ` Eli Zaretskii
  0 siblings, 2 replies; 14+ messages in thread
From: Nicolas Petton @ 2020-08-01 12:55 UTC (permalink / raw)
  To: Eli Zaretskii, Philipp Stephani; +Cc: 42482, mstanojevic

[-- 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 --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#42482: 27.0.91; emacs modules memory leak
  2020-08-01 12:55               ` Nicolas Petton
@ 2020-08-01 14:47                 ` Philipp Stephani
  2020-08-02 17:25                 ` Eli Zaretskii
  1 sibling, 0 replies; 14+ messages in thread
From: Philipp Stephani @ 2020-08-01 14:47 UTC (permalink / raw)
  To: Nicolas Petton; +Cc: 42482, Milan Stanojević

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.





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#42482: 27.0.91; emacs modules memory leak
  2020-08-01 12:55               ` Nicolas Petton
  2020-08-01 14:47                 ` Philipp Stephani
@ 2020-08-02 17:25                 ` Eli Zaretskii
  2020-08-03 23:02                   ` Milan Stanojević
  2020-08-04  8:05                   ` Nicolas Petton
  1 sibling, 2 replies; 14+ messages in thread
From: Eli Zaretskii @ 2020-08-02 17:25 UTC (permalink / raw)
  To: Nicolas Petton; +Cc: p.stephani2, 42482, mstanojevic

> 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.





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#42482: 27.0.91; emacs modules memory leak
  2020-08-02 17:25                 ` Eli Zaretskii
@ 2020-08-03 23:02                   ` Milan Stanojević
  2020-08-04  8:05                   ` Nicolas Petton
  1 sibling, 0 replies; 14+ messages in thread
From: Milan Stanojević @ 2020-08-03 23:02 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Philipp Stephani, 42482, Nicolas Petton

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.





^ permalink raw reply	[flat|nested] 14+ messages in thread

* bug#42482: 27.0.91; emacs modules memory leak
  2020-08-02 17:25                 ` Eli Zaretskii
  2020-08-03 23:02                   ` Milan Stanojević
@ 2020-08-04  8:05                   ` Nicolas Petton
  1 sibling, 0 replies; 14+ messages in thread
From: Nicolas Petton @ 2020-08-04  8:05 UTC (permalink / raw)
  To: Eli Zaretskii, Nicolas Petton; +Cc: p.stephani2, 42482, mstanojevic

[-- 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 --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-08-04  8:05 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-07-22 23:25 bug#42482: 27.0.91; emacs modules memory leak Milan Stanojević
2020-07-23 12:06 ` Philipp Stephani
2020-07-23 14:29   ` Milan Stanojević
2020-07-23 14:33     ` Philipp Stephani
2020-07-23 17:45       ` Eli Zaretskii
2020-07-23 19:11         ` Milan Stanojević
2020-07-25 21:43         ` Philipp Stephani
2020-08-01 12:11           ` Philipp Stephani
2020-08-01 12:37             ` Eli Zaretskii
2020-08-01 12:55               ` Nicolas Petton
2020-08-01 14:47                 ` Philipp Stephani
2020-08-02 17:25                 ` Eli Zaretskii
2020-08-03 23:02                   ` Milan Stanojević
2020-08-04  8:05                   ` Nicolas Petton

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).