Merge pull request #864 from kmod/perf3

more getattr rewriting

minibenchmarks/django_template.py

 import os
 import sys
-sys.path.append(os.path.join(os.path.dirname(__file__), "../test/testsuite/lib/django"))
+sys.path.append(os.path.join(os.path.dirname(__file__), "../test/lib/django"))
 
 from django.template.base import Origin, Template, Context, TemplateDoesNotExist
 from django.conf import settings

minibenchmarks/pyxl_bench.py

 import os
 import sys
 
-sys.path.append(os.path.join(os.path.dirname(__file__), "../test/testsuite/lib/pyxl/"))
+sys.path.append(os.path.join(os.path.dirname(__file__), "../test/lib/pyxl/"))
 
 from pyxl.codec.register import pyxl_transform_string
 

minibenchmarks/sqlalchemy_imperative.py

 import os
 import sys
-sys.path.append(os.path.join(os.path.dirname(__file__), "../test/testsuite/lib/sqlalchemy/lib"))
+sys.path.append(os.path.join(os.path.dirname(__file__), "../test/lib/sqlalchemy/lib"))
 
 from sqlalchemy import Column, ForeignKey, Integer, String, Table, MetaData
 from sqlalchemy.ext.declarative import declarative_base

src/capi/typeobject.cpp

@@ -953,16 +953,12 @@ static PyObject* call_attribute(PyObject* self, PyObject* attr, PyObject* name)
 }
 
 /* Pyston change: static */ PyObject* slot_tp_getattr_hook(PyObject* self, PyObject* name) noexcept {
-    try {
-        assert(name->cls == str_cls);
-        return slotTpGetattrHookInternal(self, (BoxedString*)name, NULL);
-    } catch (ExcInfo e) {
-        setCAPIException(e);
-        return NULL;
-    }
+    assert(name->cls == str_cls);
+    return slotTpGetattrHookInternal<CAPI>(self, (BoxedString*)name, NULL);
 }
 
-Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs* rewrite_args) {
+template <ExceptionStyle S>
+Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs* rewrite_args) noexcept(S == CAPI) {
     STAT_TIMER(t0, "us_timer_slot_tpgetattrhook", SLOT_AVOIDABILITY(self));
 
     PyObject* getattr, *getattribute, * res = NULL;
@@ -987,7 +983,10 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
 
         /* No __getattr__ hook: use a simpler dispatcher */
         self->cls->tp_getattro = slot_tp_getattro;
-        return slot_tp_getattro(self, name);
+        Box* r = slot_tp_getattro(self, name);
+        if (S == CXX && !r)
+            throwCAPIException();
+        return r;
     }
 
     /* speed hack: we could use lookup_maybe, but that would resolve the
@@ -1031,8 +1030,13 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
             try {
                 res = getattrInternalGeneric(self, name, &grewrite_args, false, false, NULL, NULL);
             } catch (ExcInfo e) {
-                if (!e.matches(AttributeError))
-                    throw e;
+                if (!e.matches(AttributeError)) {
+                    if (S == CAPI) {
+                        setCAPIException(e);
+                        return NULL;
+                    } else
+                        throw e;
+                }
 
                 grewrite_args.out_success = false;
                 res = NULL;
@@ -1046,8 +1050,13 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
             try {
                 res = getattrInternalGeneric(self, name, NULL, false, false, NULL, NULL);
             } catch (ExcInfo e) {
-                if (!e.matches(AttributeError))
-                    throw e;
+                if (!e.matches(AttributeError)) {
+                    if (S == CAPI) {
+                        setCAPIException(e);
+                        return NULL;
+                    } else
+                        throw e;
+                }
                 res = NULL;
             }
         }
@@ -1058,8 +1067,12 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
         if (res == NULL) {
             if (PyErr_ExceptionMatches(PyExc_AttributeError))
                 PyErr_Clear();
-            else
-                throwCAPIException();
+            else {
+                if (S == CAPI)
+                    return NULL;
+                else
+                    throwCAPIException();
+            }
         }
     }
 
@@ -1091,9 +1104,9 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
         assert(PyString_CHECK_INTERNED(name) == SSTATE_INTERNED_IMMORTAL);
         crewrite_args.arg1 = rewrite_args->rewriter->loadConst((intptr_t)name, Location::forArg(1));
 
-        res = callattrInternal<CXX>(self, _getattr_str, LookupScope::CLASS_ONLY, &crewrite_args, ArgPassSpec(1), name,
-                                    NULL, NULL, NULL, NULL);
-        assert(res);
+        res = callattrInternal<S>(self, _getattr_str, LookupScope::CLASS_ONLY, &crewrite_args, ArgPassSpec(1), name,
+                                  NULL, NULL, NULL, NULL);
+        assert(res || S == CAPI);
 
         if (!crewrite_args.out_success)
             rewrite_args = NULL;
@@ -1105,15 +1118,18 @@ Box* slotTpGetattrHookInternal(Box* self, BoxedString* name, GetattrRewriteArgs*
         // the rewrite_args and non-rewrite_args case the same.
         // Actually, we might have gotten to the point that doing a runtimeCall on an instancemethod is as
         // fast as a callattr, but that hasn't typically been the case.
-        res = callattrInternal<CXX>(self, _getattr_str, LookupScope::CLASS_ONLY, NULL, ArgPassSpec(1), name, NULL, NULL,
-                                    NULL, NULL);
-        assert(res);
+        res = callattrInternal<S>(self, _getattr_str, LookupScope::CLASS_ONLY, NULL, ArgPassSpec(1), name, NULL, NULL,
+                                  NULL, NULL);
+        assert(res || S == CAPI);
     }
 
     if (rewrite_args)
         rewrite_args->out_success = true;
     return res;
 }
+// Force instantiation of the template
+template Box* slotTpGetattrHookInternal<CAPI>(Box* self, BoxedString* name, GetattrRewriteArgs* rewrite_args);
+template Box* slotTpGetattrHookInternal<CXX>(Box* self, BoxedString* name, GetattrRewriteArgs* rewrite_args);
 
 /* Pyston change: static */ PyObject* slot_tp_new(PyTypeObject* self, PyObject* args, PyObject* kwds) noexcept {
     STAT_TIMER(t0, "us_timer_slot_tpnew", SLOT_AVOIDABILITY(self));

src/capi/typeobject.h

@@ -54,7 +54,8 @@ PyObject* tp_new_wrapper(PyTypeObject* self, BoxedTuple* args, Box* kwds) noexce
 int slot_tp_init(PyObject* self, PyObject* args, PyObject* kwds) noexcept;
 
 class GetattrRewriteArgs;
-Box* slotTpGetattrHookInternal(Box* self, BoxedString* attr, GetattrRewriteArgs* rewrite_args);
+template <ExceptionStyle S>
+Box* slotTpGetattrHookInternal(Box* self, BoxedString* attr, GetattrRewriteArgs* rewrite_args) noexcept(S == CAPI);
 }
 
 #endif

src/runtime/builtin_modules/builtins.cpp

@@ -472,6 +472,8 @@ Box* delattrFunc(Box* obj, Box* _str) {
 }
 
 static Box* getattrFuncHelper(Box* return_val, Box* obj, BoxedString* str, Box* default_val) noexcept {
+    assert(PyString_Check(str));
+
     if (return_val)
         return return_val;
 
@@ -492,21 +494,24 @@ static Box* getattrFuncHelper(Box* return_val, Box* obj, BoxedString* str, Box*
 template <ExceptionStyle S>
 Box* getattrFuncInternal(BoxedFunctionBase* func, CallRewriteArgs* rewrite_args, ArgPassSpec argspec, Box* arg1,
                          Box* arg2, Box* arg3, Box** args, const std::vector<BoxedString*>* keyword_names) {
-    if (argspec != ArgPassSpec(2, 0, false, false) || argspec != ArgPassSpec(3, 0, false, false)) {
-        static Box* defaults[] = { NULL };
-        bool rewrite_success = false;
-        rearrangeArguments(ParamReceiveSpec(3, 1, false, false), NULL, "getattr", defaults, rewrite_args,
-                           rewrite_success, argspec, arg1, arg2, arg3, args, NULL, keyword_names);
-        if (!rewrite_success)
-            rewrite_args = NULL;
-    }
+    static Box* defaults[] = { NULL };
+    bool rewrite_success = false;
+    rearrangeArguments(ParamReceiveSpec(3, 1, false, false), NULL, "getattr", defaults, rewrite_args, rewrite_success,
+                       argspec, arg1, arg2, arg3, args, NULL, keyword_names);
+    if (!rewrite_success)
+        rewrite_args = NULL;
 
     Box* obj = arg1;
     Box* _str = arg2;
     Box* default_value = arg3;
 
     if (rewrite_args) {
-        if (!PyString_Check(_str))
+        // We need to make sure that the attribute string will be the same.
+        // Even though the passed string might not be the exact attribute name
+        // that we end up looking up (because we need to encode it or intern it),
+        // guarding on that object means (for strings and unicode) that the string
+        // value is fixed.
+        if (!PyString_CheckExact(_str) && !PyUnicode_CheckExact(_str))
             rewrite_args = NULL;
         else
             rewrite_args->arg2->addGuard((intptr_t)arg2);
@@ -531,10 +536,8 @@ Box* getattrFuncInternal(BoxedFunctionBase* func, CallRewriteArgs* rewrite_args,
     }
 
     BoxedString* str = static_cast<BoxedString*>(_str);
-    if (!PyString_CHECK_INTERNED(str)) {
+    if (!PyString_CHECK_INTERNED(str))
         internStringMortalInplace(str);
-        rewrite_args = NULL;
-    }
 
     Box* rtn;
     RewriterVar* r_rtn;
@@ -554,8 +557,10 @@ Box* getattrFuncInternal(BoxedFunctionBase* func, CallRewriteArgs* rewrite_args,
     }
 
     if (rewrite_args) {
-        RewriterVar* final_rtn = rewrite_args->rewriter->call(
-            false, (void*)getattrFuncHelper, r_rtn, rewrite_args->arg1, rewrite_args->arg2, rewrite_args->arg3);
+        assert(PyString_CHECK_INTERNED(str) == SSTATE_INTERNED_IMMORTAL);
+        RewriterVar* r_str = rewrite_args->rewriter->loadConst((intptr_t)str, Location::forArg(2));
+        RewriterVar* final_rtn = rewrite_args->rewriter->call(false, (void*)getattrFuncHelper, r_rtn,
+                                                              rewrite_args->arg1, r_str, rewrite_args->arg3);
 
         if (S == CXX)
             rewrite_args->rewriter->checkAndThrowCAPIException(final_rtn);

src/runtime/objmodel.cpp

@@ -1406,20 +1406,12 @@ Box* getattrInternalEx(Box* obj, BoxedString* attr, GetattrRewriteArgs* rewrite_
         if (obj->cls->tp_getattro && obj->cls->tp_getattro != PyObject_GenericGetAttr) {
             STAT_TIMER(t0, "us_timer_slowpath_tpgetattro", 10);
 
-            if (obj->cls->tp_getattro == slot_tp_getattr_hook && S == CXX) {
-                return slotTpGetattrHookInternal(obj, attr, rewrite_args);
+            if (obj->cls->tp_getattro == slot_tp_getattr_hook) {
+                return slotTpGetattrHookInternal<S>(obj, attr, rewrite_args);
             }
 
             Box* r = obj->cls->tp_getattro(obj, attr);
 
-            if (!r) {
-                if (S == CAPI) {
-                    ensureCAPIExceptionSet();
-                    return r;
-                } else
-                    throwCAPIException();
-            }
-
             // If attr is immortal, then we are free to write an embedded reference to it.
             // Immortal are (unfortunately) common right now, so this is an easy way to get
             // around the fact that we don't currently scan ICs for GC references, but eventually
@@ -1436,6 +1428,15 @@ Box* getattrInternalEx(Box* obj, BoxedString* attr, GetattrRewriteArgs* rewrite_
                 rewrite_args->out_rtn = r_rtn;
                 rewrite_args->out_success = true;
             }
+
+            if (!r) {
+                if (S == CAPI) {
+                    ensureCAPIExceptionSet();
+                    return r;
+                } else
+                    throwCAPIException();
+            }
+
             return r;
         }
 
@@ -2832,11 +2833,12 @@ Box* _callattrEntry(Box* obj, BoxedString* attr, CallattrFlags flags, Box* arg1,
             rewrite_args.args = rewriter->getArg(6);
         rtn = callattrInternal<S>(obj, attr, scope, &rewrite_args, argspec, arg1, arg2, arg3, args, keyword_names);
 
+        assert(!(S == CAPI && flags.null_on_nonexistent));
         if (!rewrite_args.out_success) {
             rewriter.reset(NULL);
         } else if (rtn) {
             rewriter->commitReturning(rewrite_args.out_rtn);
-        } else if (flags.null_on_nonexistent) {
+        } else if ((S == CAPI && !rtn) || flags.null_on_nonexistent) {
             rewriter->commitReturning(rewriter->loadConst(0, rewriter->getReturnDestination()));
         }
     } else {
@@ -3755,26 +3757,20 @@ Box* runtimeCallInternal(Box* obj, CallRewriteArgs* rewrite_args, ArgPassSpec ar
             assert((obj->cls->tp_call == NULL) == (typeLookup(obj->cls, call_str, NULL) == NULL));
         }
 
-        try {
-            if (rewrite_args) {
-                rtn = callattrInternal<S>(obj, call_str, CLASS_ONLY, rewrite_args, argspec, arg1, arg2, arg3, args,
-                                          keyword_names);
-            } else {
-                rtn = callattrInternal<S>(obj, call_str, CLASS_ONLY, NULL, argspec, arg1, arg2, arg3, args,
-                                          keyword_names);
-            }
-        } catch (ExcInfo e) {
-            if (S == CAPI) {
-                setCAPIException(e);
-                return NULL;
-            } else
-                throw e;
+        if (rewrite_args) {
+            rtn = callattrInternal<S>(obj, call_str, CLASS_ONLY, rewrite_args, argspec, arg1, arg2, arg3, args,
+                                      keyword_names);
+        } else {
+            rtn = callattrInternal<S>(obj, call_str, CLASS_ONLY, NULL, argspec, arg1, arg2, arg3, args, keyword_names);
         }
 
         if (!rtn) {
             if (S == CAPI) {
-                if (!PyErr_Occurred())
+                if (!PyErr_Occurred()) {
+                    if (rewrite_args)
+                        rewrite_args->out_success = false;
                     PyErr_Format(TypeError, "'%s' object is not callable", getTypeName(obj));
+                }
                 return NULL;
             } else
                 raiseExcHelper(TypeError, "'%s' object is not callable", getTypeName(obj));