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 <Friedemann.Kleint@qt.io>

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<PyTypeObject *>(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<PyObject *>(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<PyObject *>(wrapper), pyMethodName);
 
     if (method && PyMethod_Check(method)
@@ -302,16 +303,13 @@ PyObject *BindingManager::getOverride(const void *cptr, const char *methodName)
             auto *parent = reinterpret_cast<PyTypeObject *>(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;
 }