PyObject finalizer #692

amos402 · 2018-06-24T18:20:39Z

What does this implement/fix? Explain your changes.

Added PyObject finalizer support, Python objects referred by C# can be auto collect now.
PythonException included C# call stack (well..., this added by debug passingly)

Does this close any currently open issues?

...

Any other comments?

...

Checklist

Check all those that are applicable and complete.

Make sure to include one or more tests for your change
If an enhancement PR, please create docs and at best an example
Add yourself to AUTHORS
Updated the CHANGELOG

codecov · 2018-06-24T19:18:15Z

Codecov Report

Merging #692 into master will decrease coverage by 0.21%.
The diff coverage is 65%.

@@            Coverage Diff             @@
##           master     #692      +/-   ##
==========================================
- Coverage    76.9%   76.69%   -0.22%     
==========================================
  Files          63       64       +1     
  Lines        5798     5925     +127     
  Branches      950      977      +27     
==========================================
+ Hits         4459     4544      +85     
- Misses       1025     1052      +27     
- Partials      314      329      +15

Flag	Coverage Δ
#setup_linux	`68.55% <ø> (ø)`	⬆️
#setup_windows	`75.91% <65%> (-0.2%)`	⬇️

Impacted Files	Coverage Δ
src/runtime/runtime.cs	`86.84% <100%> (+0.11%)`	⬆️
src/runtime/pyscope.cs	`57.28% <20%> (-1.14%)`	⬇️
src/runtime/delegatemanager.cs	`81.96% <38.46%> (-6.22%)`	⬇️
src/runtime/finalizer.cs	`66.33% <66.33%> (ø)`
src/runtime/pythonexception.cs	`76.66% <81.81%> (+10.56%)`	⬆️
src/runtime/pyobject.cs	`41.06% <85.71%> (+0.72%)`	⬆️

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 5ee234a...4eff81e. Read the comment docs.

dmitriyse · 2018-06-24T19:35:57Z

This is duplicate for the #532. There are several PR that are exists more than half of a year, and still not approved.

den-run-ai · 2018-06-24T20:01:07Z

@amos402 please review #532 and let me know your feedback. @dmitriyse that code in your PR is really hard to review, it is beyond my understanding of Python/CLR internals. I still give preference to @dmitriyse PR but @amos402 if there is anything extra beyond #532 in your PR, please let us know.
I'm planning to review your PRs within 2 weeks and then merge if no issues are found.

dmitriyse · 2018-06-25T03:15:23Z

@denfromufa, ok. I will update those PRs in this week.

amos402 · 2018-06-25T03:51:44Z

@denfromufa @dmitriyse I reviewed the #532 roughly, it create a thread for decreasing the reference of python objects, it would slow down Python if you get GIL oftenly.
But my solusion it's no need for a new python thread or .net thread. You can event use it that Python didn't compile with WITH_THREAD macro.
Because of Python's GIL, I suggest we should avoid use multithread on it. btw. I'm going to remove the PyEval_InitThreads call on Initialize for my next version.

Note from python document

Note
When only the main thread exists, no GIL operations are needed. This is a common situation (most Python programs do not use threads), and the lock operations slow the interpreter down a bit. Therefore, the lock is not created initially. This situation is equivalent to having acquired the lock: when there is only a single thread, all object accesses are safe. Therefore, when this function initializes the global interpreter lock, it also acquires it. Before the Python _thread module creates a new thread, knowing that either it has the lock or the lock hasn’t been created yet, it calls PyEval_InitThreads(). When this call returns, it is guaranteed that the lock has been created and that the calling thread has acquired it.

It is not safe to call this function when it is unknown which thread (if any) currently has the global interpreter lock.

This function is not available when thread support is disabled at compile time.

dmitriyse · 2018-06-25T06:16:49Z

I agree with @amos402 Py_AddPendingCall can give some benefits and my approach can be improved for Python 3.1+.
But I cannot agree right now with WeakReferences and GC.ReRegisterForFinalize approach. I need some research.

We are looking for different directions: single and multi threaded environment. Dedicated thread was the only approach that succesfully solve memory leak in the my multithreaded application.

@amos402 Please check this #541 bug on your branch, ensure that your finalizers approach also elliminates memory leak.

amos402 · 2018-06-25T09:31:46Z

@dmitriyse I try two cases, it seems no problems except PythonException was leak by original code. I fixed it on my branch.

can be improved for Python 3.1+
Why 3.1 specificed?
WeakReferences only use for query garbages, it's no need for running time.
If you want to avoid call GC.ReRegisterForFinalize, you can just make a interface for getting all python object pointer. But I think it's not necessary, garbages will put into garbage queue, with the queue's reference, they can be resurrected.

filmor · 2018-07-17T08:28:26Z

src/runtime/finalizer.cs

+        private ConcurrentQueue<IDisposable> _objQueue = new ConcurrentQueue<IDisposable>();
+
+        [UnmanagedFunctionPointer(CallingConvention.Cdecl)]
+        private delegate int PedingCall(IntPtr arg);


PendingCall?

The calling type for Py_AddPendingCall

I know, there's just a typo in the name ;)

filmor · 2018-07-17T08:30:44Z

src/runtime/finalizer.cs

+
+        public event EventHandler<CollectArgs> CollectOnce;
+
+        private ConcurrentQueue<IDisposable> _objQueue = new ConcurrentQueue<IDisposable>();


We already have a few places that fail in presence of multiple appdomains, this would too, I guess. Can you add a comment on this?

What failures will have occurred, can you give me some examples?

filmor · 2018-07-17T08:32:02Z

src/runtime/finalizer.cs

+        private bool _pending = false;
+        private readonly object _collectingLock = new object();
+        public int Threshold { get; set; }
+        public bool Enable { get; set; }


Why are these public?

Make users can adjust the finalizer's configuration.

filmor · 2018-07-17T09:18:05Z

src/runtime/finalizer.cs

+            }
+            Instance.DisposeAll();
+            Instance.CallPendingFinalizers();
+            Runtime.PyErr_Clear();


Why are you calling this?

Make sure no garbage left and clear the pending calls.
Pending calls wouldn't clear after calling Py_Finalize, if reset the .net environment, the function pointers would point to unknown memory and cause crash on calling them.

Why are you calling PyErr_Clear?

emmm...It seems I should expose the exceptions in DisposeAll after each obj.Dispose()

filmor · 2018-07-17T09:19:15Z

src/runtime/finalizer.cs

+
+        private void AddPendingCollect()
+        {
+            lock (_collectingLock)


This lock should probably encompass the whole function.

Why it needs to be lock whole function, the flag already set.
I can't image any situations that Py_AddPendingCall will be called in multiple threads on same time.

I don't follow, either you expect multiple concurrent executions, then the lock needs to surround the whole function, or you don't, then you don't need any locking.

Oh, yes, thank you for you reminder, I neglect that there is another store at line 114.

filmor · 2018-07-17T10:33:44Z

src/runtime/runtime.cs

@@ -198,6 +199,8 @@ public class Runtime
        internal static bool IsPython2 = pyversionnumber < 30;
        internal static bool IsPython3 = pyversionnumber >= 30;

+        public static int MainManagedThreadId { get; internal set; }


Isn't this rather a private set?

Yes, it should be. I will modify it later.

filmor · 2018-07-17T10:34:20Z

src/runtime/runtime.cs

@@ -350,6 +354,7 @@ internal static void Initialize()

        internal static void Shutdown()
        {
+            Finalizer.Shutdown();


Is there a particular reason for doing the Finalizer shutdown first? I would have expected it to come last.

It seems there is no particular reason for that. I can't remember why I put it on first, maybe just put it casually....

Synchronized fixed

filmor · 2018-08-24T11:00:11Z

src/runtime/finalizer.cs

+            if (_pending)
+            {
+                return;
+            }


Is this optimisation really needed? If not, I'd rather like the code to be obviously safe.

Just a simple double-check, make it check without lock at first.

den-run-ai · 2018-10-20T04:34:34Z

@amos402 some git conflicts with master branch now.

…finalizer

den-run-ai · 2018-10-22T19:05:02Z

This PR causes appveyor CI to get stuck for one hour on each build.

amos402 · 2018-10-24T17:03:12Z

This PR causes appveyor CI to get stuck for one hour on each build.

😥I still can't figure out why it just crash on 3.7 on Linux

amos402 · 2018-10-25T19:13:40Z

Already fixed it. It crash on 3.7 because I changed PYTHONMALLOC in test case.

…finalizer

amos402 · 2018-11-23T02:55:31Z

I added ref count check on debug mode, it should be helpful for finding the bugs like this

            PyString s1 = new PyString("test_string");
            // s2 steal a reference from s1
            PyString s2 = new PyString(s1.Handle);

amos402 added 8 commits June 24, 2018 03:20

Finalizer for PyObject

8083f3b

Avoid test interdependency

af33e74

Add source to .csproj

6d9f897

Make sure recover the environment

7140fd0

Add StackTrace of C# exception

f66697d

Clean up the test and interface

799d37e

Update CHANGELOG.md

cb55163

Mono doesn't have GC.WaitForFullGCComplete

bfc0392

Fixed PythonException leak

59b614d

den-run-ai and others added 3 commits July 4, 2018 21:45

Merge branch 'master' into pyobject-finalizer

569cd94

Fixed nPython.exe crash on Shutdown

f4f5032

Merge branch 'master' into pyobject-finalizer

0967a12

filmor reviewed Jul 23, 2018

View reviewed changes

filmor added this to the 2.4.0 milestone Jul 23, 2018

amos402 added 2 commits August 4, 2018 16:19

Add error handler

f071c55

Synchronized fixed

Merge branch 'master' into pyobject-finalizer

cfda491

filmor reviewed Aug 24, 2018

View reviewed changes

filmor and others added 4 commits August 30, 2018 13:55

Merge branch 'master' into pyobject-finalizer

f6c6e42

Make collect callback without JIT

0d96641

Merge branch 'master' into pyobject-finalizer

34713f7

Merge branch 'master' into pyobject-finalizer

b4e30ac

Merge remote-tracking branch 'remotes/upstream/master' into pyobject-…

f836ffa

…finalizer

Add pending marker

a4bb82d

amos402 force-pushed the pyobject-finalizer branch from d9a4c8e to a4bb82d Compare October 23, 2018 20:08

amos402 added 2 commits October 25, 2018 01:03

Merge branch 'master' into pyobject-finalizer

21da86d

Remove PYTHONMALLOC setting

5254c65

filmor mentioned this pull request Nov 6, 2018

Interrupting running Python code from another thread #766

Closed

filmor added the next label Nov 15, 2018

amos402 added 3 commits November 22, 2018 16:36

Merge remote-tracking branch 'remotes/upstream/master' into pyobject-…

916e85e

…finalizer

Fix ref count error

eee3683

Add ref count check for helping discover the bugs of decref too much

cee8e17

amos402 added 2 commits November 25, 2018 23:25

Fix ref count error

247e2d9

typo error

90c67ca

filmor mentioned this pull request Jan 18, 2019

Release 2.4.0 #509

Closed

filmor removed this from the 2.4.0 milestone Feb 5, 2019

Merge branch 'master' into pyobject-finalizer

6d68d70

Martin-Molinero mentioned this pull request Mar 28, 2019

Fix memory leak add pyobject finalizer QuantConnect/pythonnet#27

Merged

Merge branch 'master' into pyobject-finalizer

4eff81e

filmor merged commit f83c884 into pythonnet:master Apr 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PyObject finalizer #692

PyObject finalizer #692

amos402 commented Jun 24, 2018

codecov bot commented Jun 24, 2018 •

edited

Loading

dmitriyse commented Jun 24, 2018

den-run-ai commented Jun 24, 2018 •

edited

Loading

dmitriyse commented Jun 25, 2018

amos402 commented Jun 25, 2018

dmitriyse commented Jun 25, 2018 •

edited

Loading

amos402 commented Jun 25, 2018

filmor Jul 17, 2018

amos402 Jul 23, 2018

filmor Jul 23, 2018

filmor Jul 17, 2018

amos402 Jul 23, 2018

filmor Jul 17, 2018

amos402 Jul 23, 2018

filmor Jul 17, 2018

amos402 Jul 23, 2018

filmor Jul 23, 2018

amos402 Jul 24, 2018

filmor Jul 17, 2018

amos402 Jul 23, 2018

filmor Jul 23, 2018

amos402 Jul 24, 2018

filmor Jul 17, 2018

amos402 Jul 23, 2018

filmor Jul 17, 2018

amos402 Jul 23, 2018

filmor Aug 24, 2018

amos402 Aug 24, 2018

den-run-ai commented Oct 20, 2018

den-run-ai commented Oct 22, 2018

amos402 commented Oct 24, 2018 •

edited

Loading

amos402 commented Oct 25, 2018

amos402 commented Nov 23, 2018


		public event EventHandler<CollectArgs> CollectOnce;

		private ConcurrentQueue<IDisposable> _objQueue = new ConcurrentQueue<IDisposable>();

PyObject finalizer #692

PyObject finalizer #692

Conversation

amos402 commented Jun 24, 2018

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Checklist

codecov bot commented Jun 24, 2018 • edited Loading

Codecov Report

dmitriyse commented Jun 24, 2018

den-run-ai commented Jun 24, 2018 • edited Loading

dmitriyse commented Jun 25, 2018

amos402 commented Jun 25, 2018

dmitriyse commented Jun 25, 2018 • edited Loading

amos402 commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

den-run-ai commented Oct 20, 2018

den-run-ai commented Oct 22, 2018

amos402 commented Oct 24, 2018 • edited Loading

amos402 commented Oct 25, 2018

amos402 commented Nov 23, 2018

codecov bot commented Jun 24, 2018 •

edited

Loading

den-run-ai commented Jun 24, 2018 •

edited

Loading

dmitriyse commented Jun 25, 2018 •

edited

Loading

amos402 commented Oct 24, 2018 •

edited

Loading