-
Notifications
You must be signed in to change notification settings - Fork 124
Improved Python Wrapping #239
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
Conversation
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
It can be removed for Cortex 9.
There is significant overhead in acquiring the GIL and entering python looking for virtual method overrides, and this has been showing up in several profiles recently, and also badly affecting thread scaling. Often the overhead was being incurred for classes which were wrapped but for instanced which weren't actually subclassed in python or defining any overrides. This new mechanism detects whether or not an object is subclassed in Python at construction, and avoids ever entering Python looking for overrides for non-subclassed objects. It also avoids creating reference cycles and the associated garbage collection in this case. Additionally, the provision of wrapper classes specific to different base classes makes wrapping a large class hierarchy much easier and less error prone. The RunTimeTypedWrapper automatically adds the appropriate virtual method forwarders whereas before every piece of wrapping code had to manually insert the IECOREPYTHON_RUNTIMETYPEDWRAPPERFNS macro. This style of wrapping has been in use in Gaffer for a while and has worked well there. Currently, none of the existing Cortex bindings are using the new wrapping mechanism - for now it is intended to be used in Gaffer first where the performance benefits will be felt the most.
Intrusive pointers come at a cost, and there's no need to use them here.
Prior to this we were relying on the built in Python defaults, which compared equal only between the exact same python objects. Since we can't guarantee that a single RefCounted object is always represented by only one Python object, this default doesn't always give us the right answer. This has always been a problem for non-wrapped objects, but is now a problem even for wrapped classes if they use the new style wrappers and haven't been subclassed in Python. Ideally in the future we'll fix the object identity problem once and for all, for both wrapped and non-wrapped objects, but for now this is a good improvement.
The default hash is based on the address of the Python object, basing it on the address of the underlying C++ object prevents issues caused by two python objects referencing the same underlying RefCounted object.
Great work John! I'm looking forward to apply the new wrappers in Cortex 9! |
Fixed a duplicate method name in RefCountedTest, fixed a typo in RunTimeTypedBinding.h and deprecated the old constructor for WrapperGarbageCollector.
Thanks for spotting that error Lucio - I've fixed it along with a couple of other small tidy ups. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We have a few issues with the way we have wrapped classes in Python to date, using the IECorePython::Wrapper class :
This pull request implements a new style of wrapping which solves all these problems. In its constructor, it determines whether the instance is of a Python subclass or not, and provides this information via an isSubclassed() method. This can be called without acquiring the GIL, and is used to avoid looking for method overrides when that is not necessary. When an instance isn't subclassed we also avoid making a circular reference, and avoid all garbage collection overhead. The new wrappers also automatically provide the appropriate virtual overrides, with no need for macros.
There are two potential downsides to this change :
Right now, Cortex has both styles of wrapping available, and all existing classes are wrapped using the old style - nothing has changed for Cortex users. I'm about to put in a pull request for Gaffer which uses the new style, as it's critical for performance there. In that pull request you'll see some of the gotchas with the new approach, and how to deal with them. Once we've transitioned over to Gaffer using the new style, and got any problems ironed out we can move Cortex to the new style too, and remove all the old deprecated code - but I suggest we don't do that till the next major version.