Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIX memoryview leaks and retrofit memory-manager as context-managers #33

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

ankostis
Copy link
Contributor

BREAKING API:

  • retrofit git.util.mman as context-manager,
    to release memory-mapped regions held.

    The mmap-manager(s) are re-entrant, but not thread-safe context-manager(s),
    to be used within a with ...: block, ensuring any left-overs cursors are cleaned up.
    If not entered, :meth:StaticWindowMapManager.make_cursor() and/or
    :meth:WindowCursor.use_region() will scream.

    Get them from smmap.managed_mmaps().

  • Simplify :class:SlidingWindowMapBuffer as create/close context-manager
    (no begin_access(), or end_access()).

+ Possible fixing gitpython-developers#31 by stop decrement on
destruction, rely only on `with...` resources.
+ feat(mmap): utility to check if regions have been closed (PY3-only).
+ Add PY3 compat utilities
+ doc(changes, tutorial): update on mman usage
+ doc(tutorial): update use-cases
+ doc(changes): new bullet.
+ All gitdb TCs now pass without explit release!
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 93.116% when pulling 144891b on ankostis:leaks2 into 6e55a1c on gitpython-developers:master.

1 similar comment
@coveralls
Copy link

coveralls commented Oct 27, 2016

Coverage Status

Coverage decreased (-0.7%) to 93.116% when pulling 144891b on ankostis:leaks2 into 6e55a1c on gitpython-developers:master.

@ankostis
Copy link
Contributor Author

ankostis commented Oct 27, 2016

@Byron can you check these lines where a memoryview is crated?

It took me the half month to realize that this is leaking badly, and I want you to check the history of this commits leading up to it, because I cannot understand whether they were mistake or intentional, and what would be the repercussions (ie, performance?).

@Byron Byron self-assigned this Dec 8, 2016
@Byron
Copy link
Member

Byron commented Dec 8, 2016

@ankostis Sorry for the very late reply. I am no holiday now, and am working through everything that piled up.
About the matter: I don't understand why this would leak in the first place. Does that mean it's a python 3 only issue? Also I wonder how anything can leak in the first place unless there are dependency cycles.
The comment in the respective are of the code also contains an alternative. It appears that it was chosen just for performance reasons. Maybe alternatives do by now exist too.

@ankostis
Copy link
Contributor Author

ankostis commented Dec 8, 2016

Yes, it is a python-3 issue, because the destructors are not deterministic.
Untill all memoryview for some mmemp have close, neither the memmap nor its file can be collected.

Now, in python3, you can slice memmaps as if they were memoryview, so they are not strictly needed, unless it is a performance issue. From some quick experiments I had done, I could not see any benefits, quite the contrary, but it's been a long time and I don't remember exactly.

@Byron
Copy link
Member

Byron commented Dec 8, 2016

If using direct access would be a solution, then it probably should be used instead. Correctness over performance I would say.
The main driver behind using the 'buffer' interface was to prevent copying of the slices that are read, and thus maintain the awesome properties of a memory map even in python. I do remember me digging around in python source code to see if copies were made.
However, now is a different time, and slicing memory maps directly in python 3 sounds appealing to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants