Skip to content

Conversation

@dcoudert
Copy link
Contributor

Fixes the memory leak in is_planar reported at https://ask.sagemath.org/question/78467/memory-leak-in-is_planar/

We now ensure that memory is released before returning the answer.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

@dcoudert dcoudert self-assigned this Jul 27, 2024
@dcoudert
Copy link
Contributor Author

dcoudert commented Jul 27, 2024

@videlec I'm not sure how to show that this fixes the memory leak. At least when I check on my laptop the memory usage with htop, it's not increasing anymore when running this very long example

sage: count = 0
....: for g in graphs.nauty_geng('-C 10'):
....:     if g.is_planar():
....:         count = count+1
....: print(count)
545808

This is consistent with https://oeis.org/A021103

@github-actions
Copy link

Documentation preview for this PR (built with commit 669f0d5; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@user202729
Copy link
Contributor

@dcoudert Maybe you can look into tests/memcheck? #36046 is an example pull request that adds a test for memcheck.

@dcoudert
Copy link
Contributor Author

I found a way to check the memory consumption. However I'm not sure I can use that to make a doctest. At least we can see here that the memory leak is fixed with this PR.

import psutil
from time import sleep

def get_process_memory():
    process = psutil.Process(os.getpid())
    mem_info = process.memory_info()
    return mem_info.rss

def my_count(n):
    leak = get_process_memory()
    count = 0
    for g in graphs.nauty_geng(f'-C {n}'):
        if g.is_planar():
            count += 1
    sleep(2)  # to ensure that subprocesses have released memory 
    leak = get_process_memory() - leak
    return count, leak

Before:

sage: my_count(8)
(2893, 11714560)
sage: my_count(9)
(36496, 350650368)

With this PR:

sage: my_count(8)
(2893, 0)
sage: my_count(9)
(36496, 0)
sage: my_count(10)
(545808, 0)

@tscrim
Copy link
Collaborator

tscrim commented Aug 6, 2024

There generally are not good ways to check against memleaks using doctests, even with #36046 IMO. Nor do I think we should add (doc)tests each time we encounter and fix a memleak, at least when it is straightforward. As such, I am setting this to a positive review.

@dcoudert
Copy link
Contributor Author

dcoudert commented Aug 6, 2024

Thanks you.

@vbraun vbraun merged commit 896eee5 into sagemath:develop Aug 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants