bug#42482: 27.0.91; emacs modules memory leak

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#42482: 27.0.91; emacs modules memory leak

Milan Stanojević-2
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.

create_global_refs.c (1K) Download Attachment
create_global_refs_test.el (290 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#42482: 27.0.91; emacs modules memory leak

Philipp Stephani
Am Do., 23. Juli 2020 um 01:27 Uhr schrieb Milan Stanojević
<[hidden email]>:

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



Reply | Threaded
Open this post in threaded view
|

bug#42482: 27.0.91; emacs modules memory leak

Milan Stanojević-2
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 <[hidden email]> wrote:

>
> Am Do., 23. Juli 2020 um 01:27 Uhr schrieb Milan Stanojević
> <[hidden email]>:
> >
> > 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.



Reply | Threaded
Open this post in threaded view
|

bug#42482: 27.0.91; emacs modules memory leak

Philipp Stephani
Am Do., 23. Juli 2020 um 16:29 Uhr schrieb Milan Stanojević
<[hidden email]>:
>
> 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 <[hidden email]> wrote:
> >
> > Am Do., 23. Juli 2020 um 01:27 Uhr schrieb Milan Stanojević
> > <[hidden email]>:
> > >
> > > 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.



Reply | Threaded
Open this post in threaded view
|

bug#42482: 27.0.91; emacs modules memory leak

Eli Zaretskii
> From: Philipp Stephani <[hidden email]>
> Date: Thu, 23 Jul 2020 16:33:12 +0200
> Cc: [hidden email]
>
> Am Do., 23. Juli 2020 um 16:29 Uhr schrieb Milan Stanojević
> <[hidden email]>:
> >
> > 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.



Reply | Threaded
Open this post in threaded view
|

bug#42482: 27.0.91; emacs modules memory leak

Philipp Stephani
Am Sa., 25. Juli 2020 um 23:43 Uhr schrieb Philipp Stephani
<[hidden email]>:

>
> Am Do., 23. Juli 2020 um 19:45 Uhr schrieb Eli Zaretskii <[hidden email]>:
> >
> > > From: Philipp Stephani <[hidden email]>
> > > Date: Thu, 23 Jul 2020 16:33:12 +0200
> > > Cc: [hidden email]
> > >
> > > Am Do., 23. Juli 2020 um 16:29 Uhr schrieb Milan Stanojević
> > > <[hidden email]>:
> > > >
> > > > 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.



Reply | Threaded
Open this post in threaded view
|

bug#42482: 27.0.91; emacs modules memory leak

Eli Zaretskii
> From: Philipp Stephani <[hidden email]>
> Date: Sat, 1 Aug 2020 14:11:54 +0200
> Cc: Milan Stanojević <[hidden email]>,
> [hidden email]
>
> 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.



Reply | Threaded
Open this post in threaded view
|

bug#42482: 27.0.91; emacs modules memory leak

Nicolas Petton-5
Eli Zaretskii <[hidden email]> 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

signature.asc (497 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#42482: 27.0.91; emacs modules memory leak

Philipp Stephani
Am Sa., 1. Aug. 2020 um 14:55 Uhr schrieb Nicolas Petton <[hidden email]>:

>
> Eli Zaretskii <[hidden email]> 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.



Reply | Threaded
Open this post in threaded view
|

bug#42482: 27.0.91; emacs modules memory leak

Eli Zaretskii
In reply to this post by Nicolas Petton-5
> From: Nicolas Petton <[hidden email]>
> Cc: [hidden email], [hidden email]
> 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.



Reply | Threaded
Open this post in threaded view
|

bug#42482: 27.0.91; emacs modules memory leak

Nicolas Petton-5
Eli Zaretskii <[hidden email]> 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

signature.asc (497 bytes) Download Attachment