From 50f382579d1323817165d85bf88a394328a4e9a0 Mon Sep 17 00:00:00 2001 From: Christian Tismer Date: Tue, 11 Feb 2020 10:12:32 +0100 Subject: Avoid the GIL in non-overridden Methods In order to get better performance, we try to avoid allocating the GIL when methods have no override with a Python function. Every class gets an associated bool array that records functions where no override was found. On second call of a function, the GIL is avoided by this flag. Update 2020-02-06: The result is very promising when combined with a drastic reduction of Py_(BEGIN|END)_ALLOW_THREAD macro calls. So this could become the solution when combined with a good reduction! The Python 2 bug was circumvented by not generating the additional Py_tp_setattro functions for class QQuickItem. Task-number: PYSIDE-803 Change-Id: I0fe36edc5610b2d51bbb05f1b7507beee5088c83 Reviewed-by: Friedemann Kleint --- .../shiboken2/generator/shiboken2/cppgenerator.cpp | 65 ++++++++++++++++++---- .../shiboken2/generator/shiboken2/cppgenerator.h | 8 ++- .../generator/shiboken2/headergenerator.cpp | 13 ++++- .../generator/shiboken2/shibokengenerator.cpp | 27 +++++++++ .../generator/shiboken2/shibokengenerator.h | 6 ++ sources/shiboken2/libshiboken/bindingmanager.cpp | 8 +-- 6 files changed, 109 insertions(+), 18 deletions(-) diff --git a/sources/shiboken2/generator/shiboken2/cppgenerator.cpp b/sources/shiboken2/generator/shiboken2/cppgenerator.cpp index 64a574f9a..b4991060a 100644 --- a/sources/shiboken2/generator/shiboken2/cppgenerator.cpp +++ b/sources/shiboken2/generator/shiboken2/cppgenerator.cpp @@ -415,18 +415,17 @@ void CppGenerator::generateClass(QTextStream &s, GeneratorContext &classContext) } const AbstractMetaFunctionList &funcs = filterFunctions(metaClass); + int maxOverrides = 0; + writeCacheResetNative(s, metaClass); for (const AbstractMetaFunction *func : funcs) { const bool notAbstract = !func->isAbstract(); if ((func->isPrivate() && notAbstract && !visibilityModifiedToPrivate(func)) || (func->isModifiedRemoved() && notAbstract)) continue; - if (func->functionType() == AbstractMetaFunction::ConstructorFunction && !func->isUserAdded()) { + if (func->functionType() == AbstractMetaFunction::ConstructorFunction && !func->isUserAdded()) writeConstructorNative(s, func); - } else if ((!avoidProtectedHack() || !metaClass->hasPrivateDestructor()) - && ((func->isVirtual() || func->isAbstract()) - && (func->attributes() & AbstractMetaAttributes::FinalCppMethod) == 0)) { - writeVirtualMethodNative(s, func); - } + else if (shouldWriteVirtualMethodNative(func)) + writeVirtualMethodNative(s, func, maxOverrides++); } if (!avoidProtectedHack() || !metaClass->hasPrivateDestructor()) { @@ -696,6 +695,14 @@ void CppGenerator::generateClass(QTextStream &s, GeneratorContext &classContext) } } +void CppGenerator::writeCacheResetNative(QTextStream &s, const AbstractMetaClass *metaClass) +{ + Indentation indentation(INDENT); + s << "void " << wrapperName(metaClass) << "::resetPyMethodCache()\n{\n"; + s << INDENT << "std::fill_n(m_PyMethodCache, sizeof(m_PyMethodCache) / sizeof(m_PyMethodCache[0]), false);\n"; + s << "}\n\n"; +} + void CppGenerator::writeConstructorNative(QTextStream &s, const AbstractMetaFunction *func) { Indentation indentation(INDENT); @@ -705,6 +712,7 @@ void CppGenerator::writeConstructorNative(QTextStream &s, const AbstractMetaFunc writeFunctionCall(s, func); s << "\n{\n"; const AbstractMetaArgument *lastArg = func->arguments().isEmpty() ? nullptr : func->arguments().constLast(); + s << INDENT << "resetPyMethodCache();\n"; writeCodeSnips(s, func->injectedCodeSnips(), TypeSystem::CodeSnipPositionBeginning, TypeSystem::NativeCode, func, lastArg); s << INDENT << "// ... middle\n"; writeCodeSnips(s, func->injectedCodeSnips(), TypeSystem::CodeSnipPositionEnd, TypeSystem::NativeCode, func, lastArg); @@ -760,7 +768,9 @@ QString CppGenerator::getVirtualFunctionReturnTypeName(const AbstractMetaFunctio return QString::fromLatin1("reinterpret_cast(Shiboken::SbkType< %1 >())->tp_name").arg(func->type()->typeEntry()->qualifiedCppName()); } -void CppGenerator::writeVirtualMethodNative(QTextStream &s, const AbstractMetaFunction *func) +void CppGenerator::writeVirtualMethodNative(QTextStream &s, + const AbstractMetaFunction *func, + int cacheIndex) { //skip metaObject function, this will be written manually ahead if (usePySideExtensions() && func->ownerClass() && func->ownerClass()->isQObject() && @@ -835,6 +845,28 @@ void CppGenerator::writeVirtualMethodNative(QTextStream &s, const AbstractMetaFu s << endl; } + // PYSIDE-803: Build a boolean cache for unused overrides. + bool multi_line = retType == nullptr; // set to true when using instrumentation + s << INDENT << "if (m_PyMethodCache[" << cacheIndex << "])" << (multi_line ? " {\n" : "\n"); + { + Indentation indentation(INDENT); + s << INDENT; + if (retType) + s << "return "; + if (!func->isAbstract()) { + s << "this->::" << func->implementingClass()->qualifiedCppName() << "::"; + writeFunctionCall(s, func, Generator::VirtualCall); + } else { + if (retType) + s << ' ' << defaultReturnExpr.returnValue(); + } + if (!retType) + s << ";\n" << INDENT << "return"; + s << ";\n"; + } + if (multi_line) + s << INDENT << "}\n"; + s << INDENT << "Shiboken::GilState gil;\n"; // Get out of virtual method call if someone already threw an error. @@ -867,7 +899,10 @@ void CppGenerator::writeVirtualMethodNative(QTextStream &s, const AbstractMetaFu s << ' ' << defaultReturnExpr.returnValue(); } else { s << INDENT << "gil.release();\n"; - s << INDENT; + if (useOverrideCaching(func->ownerClass())) { + s << INDENT << "m_PyMethodCache[" << cacheIndex << "] = true;\n"; + s << INDENT; + } if (retType) s << "return "; s << "this->::" << func->implementingClass()->qualifiedCppName() << "::"; @@ -877,7 +912,7 @@ void CppGenerator::writeVirtualMethodNative(QTextStream &s, const AbstractMetaFu } } s << ";\n"; - s << INDENT<< "}\n\n"; + s << INDENT << "}\n\n"; //WS writeConversionRule(s, func, TypeSystem::TargetLangCode); @@ -5235,6 +5270,16 @@ void CppGenerator::writeSetattroFunction(QTextStream &s, AttroCheck attroCheck, Q_ASSERT(!context.forSmartPointer()); const AbstractMetaClass *metaClass = context.metaClass(); writeSetattroDefinition(s, metaClass); + // PYSIDE-803: Detect duck-punching; clear cache if a method is set. + if (attroCheck.testFlag(AttroCheckFlag::SetattroMethodOverride) + && ShibokenGenerator::shouldGenerateCppWrapper(metaClass)) { + s << INDENT << "if (value && PyCallable_Check(value)) {\n"; + s << INDENT << " auto plain_inst = " << cpythonWrapperCPtr(metaClass, QLatin1String("self")) << ";\n"; + s << INDENT << " auto inst = dynamic_cast<" << wrapperName(metaClass) << " *>(plain_inst);\n"; + s << INDENT << " if (inst)\n"; + s << INDENT << " inst->resetPyMethodCache();\n"; + s << INDENT << "}\n"; + } if (attroCheck.testFlag(AttroCheckFlag::SetattroQObject)) { s << INDENT << "Shiboken::AutoDecRef pp(reinterpret_cast(PySide::Property::getObject(self, name)));\n"; s << INDENT << "if (!pp.isNull())\n"; @@ -5897,7 +5942,7 @@ void CppGenerator::writeParentChildManagement(QTextStream &s, const AbstractMeta writeReturnValueHeuristics(s, func); } -void CppGenerator::writeReturnValueHeuristics(QTextStream &s, const AbstractMetaFunction *func, const QString &self) +void CppGenerator::writeReturnValueHeuristics(QTextStream &s, const AbstractMetaFunction *func) { AbstractMetaType *type = func->type(); if (!useReturnValueHeuristic() diff --git a/sources/shiboken2/generator/shiboken2/cppgenerator.h b/sources/shiboken2/generator/shiboken2/cppgenerator.h index e3bb2c9cf..aee1fb7d4 100644 --- a/sources/shiboken2/generator/shiboken2/cppgenerator.h +++ b/sources/shiboken2/generator/shiboken2/cppgenerator.h @@ -50,11 +50,15 @@ protected: bool finishGeneration() override; private: + void writeInitFunc(QTextStream &declStr, QTextStream &callStr, + const Indentor &indent, const QString &initFunctionName, + const TypeEntry *enclosingEntry = nullptr); + void writeCacheResetNative(QTextStream &s, const AbstractMetaClass *metaClass); void writeConstructorNative(QTextStream &s, const AbstractMetaFunction *func); void writeDestructorNative(QTextStream &s, const AbstractMetaClass *metaClass); QString getVirtualFunctionReturnTypeName(const AbstractMetaFunction *func); - void writeVirtualMethodNative(QTextStream &s, const AbstractMetaFunction *func); + void writeVirtualMethodNative(QTextStream &s, const AbstractMetaFunction *func, int cacheIndex); void writeMetaObjectMethod(QTextStream &s, const AbstractMetaClass *metaClass); void writeMetaCast(QTextStream &s, const AbstractMetaClass *metaClass); @@ -309,7 +313,7 @@ private: void writeParentChildManagement(QTextStream &s, const AbstractMetaFunction *func, bool userHeuristicForReturn); bool writeParentChildManagement(QTextStream &s, const AbstractMetaFunction *func, int argIndex, bool userHeuristicPolicy); - void writeReturnValueHeuristics(QTextStream &s, const AbstractMetaFunction *func, const QString &self = QLatin1String("self")); + void writeReturnValueHeuristics(QTextStream &s, const AbstractMetaFunction *func); void writeInitQtMetaTypeFunctionBody(QTextStream &s, GeneratorContext &context) const; /** diff --git a/sources/shiboken2/generator/shiboken2/headergenerator.cpp b/sources/shiboken2/generator/shiboken2/headergenerator.cpp index 6ae00f75b..34bba408a 100644 --- a/sources/shiboken2/generator/shiboken2/headergenerator.cpp +++ b/sources/shiboken2/generator/shiboken2/headergenerator.cpp @@ -139,10 +139,17 @@ void HeaderGenerator::generateClass(QTextStream &s, GeneratorContext &classConte s << "\n{\npublic:\n"; const AbstractMetaFunctionList &funcs = filterFunctions(metaClass); + int maxOverrides = 0; for (AbstractMetaFunction *func : funcs) { - if ((func->attributes() & AbstractMetaAttributes::FinalCppMethod) == 0) + if ((func->attributes() & AbstractMetaAttributes::FinalCppMethod) == 0) { writeFunction(s, func); + // PYSIDE-803: Build a boolean cache for unused overrides. + if (shouldWriteVirtualMethodNative(func)) + maxOverrides++; + } } + if (!maxOverrides) + maxOverrides = 1; if (avoidProtectedHack() && metaClass->hasProtectedFields()) { const AbstractMetaFieldList &fields = metaClass->fields(); @@ -182,6 +189,10 @@ void HeaderGenerator::generateClass(QTextStream &s, GeneratorContext &classConte if (usePySideExtensions()) s << INDENT << "static void pysideInitQtMetaTypes();\n"; + s << INDENT << "void resetPyMethodCache();\n"; + s << "private:\n"; + s << INDENT << "mutable bool m_PyMethodCache[" << maxOverrides << "];\n"; + s << "};\n\n"; if (!innerHeaderGuard.isEmpty()) s << "# endif // SBK_" << innerHeaderGuard << "_H\n\n"; diff --git a/sources/shiboken2/generator/shiboken2/shibokengenerator.cpp b/sources/shiboken2/generator/shiboken2/shibokengenerator.cpp index 86fca9c55..56904e44f 100644 --- a/sources/shiboken2/generator/shiboken2/shibokengenerator.cpp +++ b/sources/shiboken2/generator/shiboken2/shibokengenerator.cpp @@ -322,6 +322,15 @@ bool ShibokenGenerator::shouldGenerateCppWrapper(const AbstractMetaClass *metaCl return result; } +bool ShibokenGenerator::shouldWriteVirtualMethodNative(const AbstractMetaFunction *func) +{ + // PYSIDE-803: Extracted this because it is used multiple times. + const AbstractMetaClass *metaClass = func->ownerClass(); + return (!avoidProtectedHack() || !metaClass->hasPrivateDestructor()) + && ((func->isVirtual() || func->isAbstract()) + && (func->attributes() & AbstractMetaAttributes::FinalCppMethod) == 0); +} + void ShibokenGenerator::lookForEnumsInClassesNotToBeGenerated(AbstractMetaEnumList &enumList, const AbstractMetaClass *metaClass) { Q_ASSERT(metaClass); @@ -371,6 +380,15 @@ QString ShibokenGenerator::wrapperName(const AbstractMetaType *metaType) const return metaType->cppSignature(); } +QString ShibokenGenerator::wrapperName(const TypeEntry *type) const +{ + QString name = type->name(); + int pos = name.lastIndexOf(QLatin1String("::")); + if (pos >= 0) + name = name.remove(0, pos + 2); + return name + QLatin1String("Wrapper"); +} + QString ShibokenGenerator::fullPythonClassName(const AbstractMetaClass *metaClass) { QString fullClassName = metaClass->name(); @@ -2182,6 +2200,13 @@ bool ShibokenGenerator::injectedCodeUsesArgument(const AbstractMetaFunction *fun return false; } +bool ShibokenGenerator::useOverrideCaching(const AbstractMetaClass *metaClass) +{ + return metaClass->isPolymorphic() + && !metaClass->typeEntry()->typeFlags().testFlag(ComplexTypeEntry::NoOverrideCaching); + +} + ShibokenGenerator::AttroCheck ShibokenGenerator::checkAttroFunctionNeeds(const AbstractMetaClass *metaClass) const { AttroCheck result; @@ -2192,6 +2217,8 @@ ShibokenGenerator::AttroCheck ShibokenGenerator::checkAttroFunctionNeeds(const A result |= AttroCheckFlag::GetattroOverloads; if (usePySideExtensions() && metaClass->qualifiedCppName() == QLatin1String("QObject")) result |= AttroCheckFlag::SetattroQObject; + if (useOverrideCaching(metaClass)) + result |= AttroCheckFlag::SetattroMethodOverride; } return result; } diff --git a/sources/shiboken2/generator/shiboken2/shibokengenerator.h b/sources/shiboken2/generator/shiboken2/shibokengenerator.h index 13419b79b..24c1374ae 100644 --- a/sources/shiboken2/generator/shiboken2/shibokengenerator.h +++ b/sources/shiboken2/generator/shiboken2/shibokengenerator.h @@ -72,6 +72,7 @@ public: GetattroMask = 0x0F, SetattroQObject = 0x10, SetattroSmartPointer = 0x20, + SetattroMethodOverride = 0x40, SetattroMask = 0xF0, }; Q_DECLARE_FLAGS(AttroCheck, AttroCheckFlag); @@ -193,6 +194,7 @@ protected: /// Returns the top-most class that has multiple inheritance in the ancestry. static const AbstractMetaClass *getMultipleInheritingClass(const AbstractMetaClass *metaClass); + static bool useOverrideCaching(const AbstractMetaClass *metaClass); AttroCheck checkAttroFunctionNeeds(const AbstractMetaClass *metaClass) const; /// Returns a list of methods of the given class where each one is part of a different overload with both static and non-static method. @@ -212,6 +214,9 @@ protected: /// Verifies if the class should have a C++ wrapper generated for it, instead of only a Python wrapper. bool shouldGenerateCppWrapper(const AbstractMetaClass *metaClass) const; + /// Condition to call WriteVirtualMethodNative. Was extracted because also used to count these calls. + bool shouldWriteVirtualMethodNative(const AbstractMetaFunction *func); + /// Adds enums eligible for generation from classes/namespaces marked not to be generated. static void lookForEnumsInClassesNotToBeGenerated(AbstractMetaEnumList &enumList, const AbstractMetaClass *metaClass); /// Returns the enclosing class for an enum, or nullptr if it should be global. @@ -219,6 +224,7 @@ protected: QString wrapperName(const AbstractMetaClass *metaClass) const; QString wrapperName(const AbstractMetaType *metaType) const; + QString wrapperName(const TypeEntry *type) const; QString fullPythonClassName(const AbstractMetaClass *metaClass); QString fullPythonFunctionName(const AbstractMetaFunction *func); diff --git a/sources/shiboken2/libshiboken/bindingmanager.cpp b/sources/shiboken2/libshiboken/bindingmanager.cpp index 725150e87..1f18ed60a 100644 --- a/sources/shiboken2/libshiboken/bindingmanager.cpp +++ b/sources/shiboken2/libshiboken/bindingmanager.cpp @@ -37,6 +37,7 @@ ** ****************************************************************************/ +#include "autodecref.h" #include "basewrapper.h" #include "basewrapper_p.h" #include "bindingmanager.h" @@ -288,7 +289,7 @@ PyObject *BindingManager::getOverride(const void *cptr, const char *methodName) } } - PyObject *pyMethodName = Shiboken::String::fromCString(methodName); + Shiboken::AutoDecRef pyMethodName(Shiboken::String::fromCString(methodName)); PyObject *method = PyObject_GetAttr(reinterpret_cast(wrapper), pyMethodName); if (method && PyMethod_Check(method) @@ -302,16 +303,13 @@ PyObject *BindingManager::getOverride(const void *cptr, const char *methodName) auto *parent = reinterpret_cast(PyTuple_GET_ITEM(mro, i)); if (parent->tp_dict) { defaultMethod = PyDict_GetItem(parent->tp_dict, pyMethodName); - if (defaultMethod && PyMethod_GET_FUNCTION(method) != defaultMethod) { - Py_DECREF(pyMethodName); + if (defaultMethod && PyMethod_GET_FUNCTION(method) != defaultMethod) return method; - } } } } Py_XDECREF(method); - Py_DECREF(pyMethodName); return nullptr; } -- cgit v1.2.3