FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Make finalizer use a task by Martin-Molinero · Pull Request #852 · pythonnet/pythonnet · GitHub

Make finalizer use a task - #852

Merged
filmor merged 4 commits into
pythonnet:masterfrom
QuantConnect:fix-memory-leak-finalizer-use-task
Apr 30, 2019
Merged

Make finalizer use a task#852
filmor merged 4 commits into
pythonnet:masterfrom
QuantConnect:fix-memory-leak-finalizer-use-task

Conversation

Martin-Molinero commented Apr 23, 2019
edited
Loading

Copy link
Copy Markdown
Contributor

After

Before

- The callback set by `Runtime.Py_AddPendingCall()` was not being
triggered in some cases in a multithreading environment. Replacing it
with a `Task`

filmor commented Apr 24, 2019

Copy link
Copy Markdown
Member

Great, thanks for your continuous efforts to backport fixes, it's highly appreciated. I'll look into merging this before the 2.4 release :)

codecov Bot commented Apr 27, 2019
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #852 into master will decrease coverage by 11.57%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #852       +/-   ##
==========================================
- Coverage   76.88%   65.3%   -11.58%     
==========================================
  Files          64       1       -63     
  Lines        5935     294     -5641     
  Branches      976       0      -976     
==========================================
- Hits         4563     192     -4371     
+ Misses       1041     102      -939     
+ Partials      331       0      -331
Flag Coverage Δ
#setup_linux 65.3% <ø> (ø) ⬆️
#setup_windows ?
Impacted Files Coverage Δ
setup.py 65.3% <0%> (-21.77%) ⬇️
src/runtime/pyansistring.cs
src/runtime/constructorbinder.cs
src/runtime/methodbinder.cs
src/runtime/Util.cs
src/runtime/delegatemanager.cs
src/runtime/eventbinding.cs
src/runtime/pyiter.cs
src/runtime/pynumber.cs
src/runtime/interop27.cs
... and 53 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6034640...cf17b3c. Read the comment docs.

filmor commented Apr 29, 2019

Copy link
Copy Markdown
Member

The Linux build seems to be fine apart from an unrelated error (happens on master as well). On Windows, however, for some reason this seems to stall, the builds time out during the unit tests.

filmor commented Apr 29, 2019

Copy link
Copy Markdown
Member

It trips up the DomainReloadAndGC test, it looks like there is a deadlock in DisposeAll.

Comment thread src/runtime/finalizer.cs Outdated

filmor commented Apr 29, 2019

Copy link
Copy Markdown
Member

If I remove the GIL call from DisposeAll everything works fine as far as I can tell right now.

filmor merged commit 33db56d into pythonnet:master Apr 30, 2019

filmor commented Apr 30, 2019

Copy link
Copy Markdown
Member

Great, this looks a lot better, thank you very much :)

amos402 commented Jun 14, 2019

Copy link
Copy Markdown
Member

@Martin-Molinero Using Py_AddPendingCall is aiming for no Python thread, although we called PyEval_InitThreads at Initialize, but that is a optimize point that we can prevent create a GIL on single thread environment. Also using Py.GIL may create a temporary Python thread state which takes overhead.
I think we can make a better user experience for the finalizer, we can setup a timer for finalizing queue, if the time past threshold value then create a Python thread to dispose them. That may also help for the queue size don't reach count threshold for a long time but keep the object reference.

Martin-Molinero deleted the fix-memory-leak-finalizer-use-task branch August 27, 2019 20:40
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL