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

Improve Python.NET interop performance by lostmsu · Pull Request #31 · QuantConnect/pythonnet · GitHub

/ pythonnet Public
forked from pythonnet/pythonnet

Improve Python.NET interop performance #31

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.

Sign up for GitHub

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
Martin-Molinero merged 1 commit into from
Jun 24, 2019
Conversation 32 Commits 1 Checks 0 Files changed
Merged

Improve Python.NET interop performance #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension .cs  (7) .md  (2) All 2 file types selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
1 change: 1 addition & 0 deletions AUTHORS.md
Show comments
View file
Open in desktop
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
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
- Sam Winstanley ([@swinstanley](https://github.com/swinstanley))
- Sean Freitag ([@cowboygneox](https://github.com/cowboygneox))
- Serge Weinstock ([@sweinst](https://github.com/sweinst))
- Victor Milovanov([@lostmsu](https://github.com/lostmsu))

mchandschuh Jun 19, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

👍 -- always nice to get credit!
- Ville M. Vainio ([@vivainio](https://github.com/vivainio))
- Virgil Dupras ([@hsoft](https://github.com/hsoft))
- Wenguang Yang ([@yagweb](https://github.com/yagweb))
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Show comments
View file
Open in desktop
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
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ This document follows the conventions laid out in [Keep a CHANGELOG][].

### Changed
- PythonException included C# call stack
- improved performance of calls from Python to C#

### Fixed

Expand Down
2 changes: 1 addition & 1 deletion src/runtime/classbase.cs
Show comments
View file
Open in desktop
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
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ public static int tp_is_gc(IntPtr type)
public static void tp_dealloc(IntPtr ob)
{
ManagedType self = GetManagedObject(ob);
IntPtr dict = Marshal.ReadIntPtr(ob, ObjectOffset.DictOffset(ob));
IntPtr dict = Marshal.ReadIntPtr(ob, ObjectOffset.TypeDictOffset(self.tpHandle));
if (dict != IntPtr.Zero)
{
Runtime.XDecref(dict);
Expand Down
4 changes: 2 additions & 2 deletions src/runtime/classderived.cs
Show comments
View file
Open in desktop
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
Show hidden characters
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
Expand Down Expand Up @@ -870,7 +870,7 @@ public static void Finalize(IPythonDerivedType obj)
// the C# object is being destroyed which must mean there are no more
// references to the Python object as well so now we can dealloc the
// python object.
IntPtr dict = Marshal.ReadIntPtr(self.pyHandle, ObjectOffset.DictOffset(self.pyHandle));
IntPtr dict = Marshal.ReadIntPtr(self.pyHandle, ObjectOffset.TypeDictOffset(self.tpHandle));
if (dict != IntPtr.Zero)
{
Runtime.XDecref(dict);
Expand Down
4 changes: 2 additions & 2 deletions src/runtime/clrobject.cs
Show comments
View file
Open in desktop
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
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ internal CLRObject(object ob, IntPtr tp)
long flags = Util.ReadCLong(tp, TypeOffset.tp_flags);
if ((flags & TypeFlags.Subclass) != 0)
{
IntPtr dict = Marshal.ReadIntPtr(py, ObjectOffset.DictOffset(tp));
IntPtr dict = Marshal.ReadIntPtr(py, ObjectOffset.TypeDictOffset(tp));
if (dict == IntPtr.Zero)
{
dict = Runtime.PyDict_New();
Marshal.WriteIntPtr(py, ObjectOffset.DictOffset(tp), dict);
Marshal.WriteIntPtr(py, ObjectOffset.TypeDictOffset(tp), dict);
}
}

Expand Down
123 changes: 87 additions & 36 deletions src/runtime/interop.cs
Show comments
View file
Open in desktop
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
Show hidden characters
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.Diagnostics;
using System.Runtime.InteropServices;
using System.Reflection;
using System.Text;
Expand Down Expand Up @@ -67,11 +68,47 @@ public ModulePropertyAttribute()
}
}

internal static class ManagedDataOffsets
{
static ManagedDataOffsets()
{
FieldInfo[] fi = typeof(ManagedDataOffsets).GetFields(BindingFlags.Static | BindingFlags.Public);
for (int i = 0; i < fi.Length; i++)
{
fi[i].SetValue(null, -(i * IntPtr.Size) - IntPtr.Size);
}

[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Ansi)]
internal class ObjectOffset
size = fi.Length * IntPtr.Size;
}

public static readonly int ob_data;
public static readonly int ob_dict;

Martin-Molinero Jun 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think ob_dict and ob_data are never being assigned?

lostmsu Jun 18, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

These are assigned via reflection in the static class constructor.

mchandschuh Jun 19, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

These should likely be private and perhaps a comment stating that they're assigned/accessed via reflection.

lostmsu Jun 19, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@mchandschuh the visibility on these follows visibility on similar fields in other *Offset classes. One of the reasons for that is for reflection to be able to discern between fields mapping to python object fields, and fields, that are necessary for this class methods to function properly.

I don't think comment here will do them any good, as in the next implementation they might be set by different means, and comments like this tend to be overlooked.

I intend to keep this part as is, if there are no further objections.

private static int BaseOffset(IntPtr type)
{
Debug.Assert(type != IntPtr.Zero);

mchandschuh Jun 19, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@Martin-Molinero -- Do we build w/ #DEBUG compiler flag in production?

Martin-Molinero Jun 19, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

No 👍 we build targeting the ReleaseWin/Mono configuration that doesn't use the #Debug flag nor do we add it
int typeSize = Marshal.ReadInt32(type, TypeOffset.tp_basicsize);
Debug.Assert(typeSize > 0 && typeSize <= ExceptionOffset.Size());
return typeSize;
}
public static int DataOffset(IntPtr type)
{
return BaseOffset(type) + ob_data;
}

public static int DictOffset(IntPtr type)
{
return BaseOffset(type) + ob_dict;
}

public static int Size { get { return size; } }

private static readonly int size;
}

internal static class OriginalObjectOffsets
{
static ObjectOffset()
static OriginalObjectOffsets()
{
int size = IntPtr.Size;
var n = 0; // Py_TRACE_REFS add two pointers to PyObject_HEAD
Expand All @@ -82,39 +119,58 @@ static ObjectOffset()
#endif
ob_refcnt = (n + 0) * size;
ob_type = (n + 1) * size;
ob_dict = (n + 2) * size;
ob_data = (n + 3) * size;
}

public static int magic(IntPtr ob)
public static int Size { get { return size; } }

private static readonly int size =
#if PYTHON_WITH_PYDEBUG
4 * IntPtr.Size;
#else
2 * IntPtr.Size;
#endif

#if PYTHON_WITH_PYDEBUG
public static int _ob_next;
public static int _ob_prev;
#endif
public static int ob_refcnt;
public static int ob_type;
}

[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Ansi)]
internal class ObjectOffset
{
static ObjectOffset()
{
#if PYTHON_WITH_PYDEBUG
_ob_next = OriginalObjectOffsets._ob_next;
_ob_prev = OriginalObjectOffsets._ob_prev;
#endif
ob_refcnt = OriginalObjectOffsets.ob_refcnt;
ob_type = OriginalObjectOffsets.ob_type;

size = OriginalObjectOffsets.Size + ManagedDataOffsets.Size;
}

public static int magic(IntPtr type)
{
if (IsException(ob))
{
return ExceptionOffset.ob_data;
}
return ob_data;
return ManagedDataOffsets.DataOffset(type);
}

public static int DictOffset(IntPtr ob)
public static int TypeDictOffset(IntPtr type)
{
if (IsException(ob))
{
return ExceptionOffset.ob_dict;
}
return ob_dict;
return ManagedDataOffsets.DictOffset(type);

Martin-Molinero Jun 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Could you please provide more detailed comments on why TypeDictOffset() and magic() new implementations are equivalent to the previous?

lostmsu Jun 18, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I mentioned that in the PR: Python.NET always adds these fields to the end of the objects it allocates. So offsets to them can be computed as objAddr + objSize - const, no matter the type of the object.

mchandschuh Jun 19, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It's nice to leave comments describing the why in the source code and even in the commit messages as it makes understanding the code later much easier. Consider in a time, far far away, where github ceases to exist, this perfect comment describing the why of these offsets would be lost forever :)

lostmsu Jun 19, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That's why I put the explanation into the commit message of the squashed commit.
}

public static int Size(IntPtr ob)
public static int Size(IntPtr pyType)
{
if (IsException(ob))
if (IsException(pyType))
{
return ExceptionOffset.Size();
}
#if PYTHON_WITH_PYDEBUG
return 6 * IntPtr.Size;
#else
return 4 * IntPtr.Size;
#endif

return size;
}

#if PYTHON_WITH_PYDEBUG
Expand All @@ -123,8 +179,7 @@ public static int Size(IntPtr ob)
#endif
public static int ob_refcnt;
public static int ob_type;
private static int ob_dict;
private static int ob_data;
private static readonly int size;

private static bool IsException(IntPtr pyObject)
{
Expand All @@ -141,19 +196,17 @@ internal class ExceptionOffset
static ExceptionOffset()
{
Type type = typeof(ExceptionOffset);
FieldInfo[] fi = type.GetFields();
int size = IntPtr.Size;
FieldInfo[] fi = type.GetFields(BindingFlags.Static | BindingFlags.Public);
for (int i = 0; i < fi.Length; i++)
{
fi[i].SetValue(null, (i * size) + ObjectOffset.ob_type + size);
fi[i].SetValue(null, (i * IntPtr.Size) + OriginalObjectOffsets.Size);

mchandschuh Jun 19, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

nit - why inline IntPtr.Size here -- it certainly safe to assume that the value doesn't change over the coarse of the loop.

lostmsu Jun 19, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Saves a jump to the original line to figure out what this size is supposed to be.
}
}

public static int Size()
{
return ob_data + IntPtr.Size;
size = fi.Length * IntPtr.Size + OriginalObjectOffsets.Size + ManagedDataOffsets.Size;
}

public static int Size() { return size; }

// PyException_HEAD
// (start after PyObject_HEAD)
public static int dict = 0;
Expand All @@ -167,9 +220,7 @@ public static int Size()
public static int suppress_context = 0;
#endif

// extra c# data
public static int ob_dict;
public static int ob_data;
private static readonly int size;
}


Expand Down
2 changes: 1 addition & 1 deletion src/runtime/managedtype.cs
Show comments
View file
Open in desktop
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
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ internal static ManagedType GetManagedObject(IntPtr ob)
{
IntPtr op = tp == ob
? Marshal.ReadIntPtr(tp, TypeOffset.magic())
: Marshal.ReadIntPtr(ob, ObjectOffset.magic(ob));
: Marshal.ReadIntPtr(ob, ObjectOffset.magic(tp));
if (op == IntPtr.Zero)
{
return null;
Expand Down
2 changes: 1 addition & 1 deletion src/runtime/moduleobject.cs
Show comments
View file
Open in desktop
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
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public ModuleObject(string name)
Runtime.XDecref(pyfilename);
Runtime.XDecref(pydocstring);

Marshal.WriteIntPtr(pyHandle, ObjectOffset.DictOffset(pyHandle), dict);
Marshal.WriteIntPtr(pyHandle, ObjectOffset.TypeDictOffset(tpHandle), dict);

InitializeModuleMembers();
}
Expand Down
8 changes: 4 additions & 4 deletions src/runtime/typemanager.cs
Show comments
View file
Open in desktop
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
Show hidden characters
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Collections;
using System.Collections.Generic;
using System.Reflection;
Expand Down Expand Up @@ -79,7 +79,7 @@ internal static IntPtr CreateType(Type impl)
// Set tp_basicsize to the size of our managed instance objects.
Marshal.WriteIntPtr(type, TypeOffset.tp_basicsize, (IntPtr)ob_size);

var offset = (IntPtr)ObjectOffset.DictOffset(type);
var offset = (IntPtr)ObjectOffset.TypeDictOffset(type);
Marshal.WriteIntPtr(type, TypeOffset.tp_dictoffset, offset);

InitializeSlots(type, impl);
Expand Down Expand Up @@ -117,17 +117,17 @@ internal static IntPtr CreateType(ManagedType impl, Type clrType)

IntPtr base_ = IntPtr.Zero;
int ob_size = ObjectOffset.Size(Runtime.PyTypeType);
int tp_dictoffset = ObjectOffset.DictOffset(Runtime.PyTypeType);

// XXX Hack, use a different base class for System.Exception
// Python 2.5+ allows new style class exceptions but they *must*
// subclass BaseException (or better Exception).
if (typeof(Exception).IsAssignableFrom(clrType))
{
ob_size = ObjectOffset.Size(Exceptions.Exception);
tp_dictoffset = ObjectOffset.DictOffset(Exceptions.Exception);
}

int tp_dictoffset = ob_size + ManagedDataOffsets.ob_dict;

if (clrType == typeof(Exception))
{
base_ = Exceptions.Exception;
Expand Down
Back | FazBrowse Home | New Git URL