Skip to content

Improve performance of unwrapping .NET objects passed from Python #930

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

Merged
merged 13 commits into from
May 16, 2020

Conversation

filmor
Copy link
Member

@filmor filmor commented Aug 2, 2019

Upstreams QuantConnect#31 by @lostmsu.

Comment by the author:

This addresses the following scenario:

  1. A C# object a is created and filled with some data.
  2. a is passed via Python.NET to Python. To do that Python.NET creates a wrapper object w, and stores reference to a in one of its fields.
  3. Python code later passes w back to C#, e.g. calls SomeCSharpMethod(w).
  4. Python.NET has to unwrap w, so it reads the reference to a from it.

Prior to this change in 4. Python.NET had to determine what kind of an object a is. If it is an exception, a different offset needed to be used. That check was very expensive (up to 4 calls into Python interpreter).

This change replaces that check with computing offset unconditionally by subtracting a constant from the object size (which is read from the wrapper), thus avoiding calls to Python interpreter.

@filmor filmor requested a review from lostmsu August 2, 2019 10:46
This addresses the following scenario:

1. A C# object `a` is created and filled with some data.
2. `a` is passed via Python.NET to Python. To do that Python.NET
   creates a wrapper object `w`, and stores reference to `a` in one of
   its fields.
3. Python code later passes `w` back to C#, e.g. calls `SomeCSharpMethod(w)`.
4. Python.NET has to unwrap `w`, so it reads the reference to `a` from it.

Prior to this change in 4. Python.NET had to determine what kind of an
object `a` is. If it is an exception, a different offset needed to be
used. That check was very expensive (up to 4 calls into Python
interpreter).

This change replaces that check with computing offset unconditionally
by subtracting a constant from the object size (which is read from the wrapper),
thus avoiding calls to Python interpreter.
@filmor filmor force-pushed the losttech-perf-interop branch from 2dea84a to 39add65 Compare August 2, 2019 10:46
@codecov-io
Copy link

codecov-io commented Nov 21, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #930      +/-   ##
==========================================
- Coverage   86.66%   86.33%   -0.34%     
==========================================
  Files           1        1              
  Lines         300      300              
==========================================
- Hits          260      259       -1     
- Misses         40       41       +1     
Flag Coverage Δ
#setup_linux 64.66% <ø> (-0.67%) ⬇️
#setup_windows 72.00% <ø> (ø)
Impacted Files Coverage Δ
setup.py 86.33% <0.00%> (-0.34%) ⬇️

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 782eff8...d3d6e50. Read the comment docs.

@lostmsu lostmsu self-assigned this Feb 21, 2020
@lostmsu
Copy link
Member

lostmsu commented Feb 21, 2020

Now we need to update the perf goal accordingly. @filmor, can cherry-pick c2ddaa0 into this branch?

Tried to do it myself, but got

! [remote rejected] filmor/losttech-perf-interop -> filmor/losttech-perf-interop (permission denied)

Copy link
Member

@lostmsu lostmsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs c2ddaa0 to be cherry-picked into this branch.

@filmor
Copy link
Member Author

filmor commented Mar 7, 2020

Sorry for the delay, cherry-picking didn't work (I assume the given commit is not in any of the branches on the main fork).

@lostmsu
Copy link
Member

lostmsu commented Mar 7, 2020

Hm, it shows in this fork. This is a very small change though, you can copy it manually.

@filmor
Copy link
Member Author

filmor commented Mar 8, 2020

I did (just before commenting ;)).

@filmor filmor merged commit 4a92d80 into pythonnet:master May 16, 2020
@filmor filmor deleted the losttech-perf-interop branch May 16, 2020 20:34
lostmsu added a commit to losttech/pythonnet that referenced this pull request Jun 15, 2020
AlexCatarino pushed a commit to QuantConnect/pythonnet that referenced this pull request Jun 29, 2020
…thonnet#930)

This addresses the following scenario:

1. A C# object `a` is created and filled with some data.
2. `a` is passed via Python.NET to Python. To do that Python.NET
   creates a wrapper object `w`, and stores reference to `a` in one of
   its fields.
3. Python code later passes `w` back to C#, e.g. calls `SomeCSharpMethod(w)`.
4. Python.NET has to unwrap `w`, so it reads the reference to `a` from it.

Prior to this change in 4. Python.NET had to determine what kind of an
object `a` is. If it is an exception, a different offset needed to be
used. That check was very expensive (up to 4 calls into Python
interpreter).

This change replaces that check with computing offset unconditionally
by subtracting a constant from the object size (which is read from the wrapper),
thus avoiding calls to Python interpreter.

Co-authored-by: Victor Milovanov <lost@losttech.software>
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