Repository: pytest-dev/pluggy
Pull request #280 (2020-06-26 16:04:59)

Optimize hook calling a bit
~~*Note: includes PR #279, please ignore the duplicate commits (will rebase once that one is settled).*~~

This PR start to optimize the hook calling path, mostly for the benefit of pytest.

For this pytest file which we use as a useful benchmark of pytest overhead,

```py
import pytest
@pytest.mark.parametrize("x", range(5000))
def test_foo(x): pass
```

Before: `10752102 function calls (10196645 primitive calls) in 9.270 seconds`
After: `10351436 function calls (9896147 primitive calls) in 8.918 seconds`

The main change stems from looking at the stack trace of a hook call. Before this PR it was this (pytest often has several of these nested):

```
File "/pytest/src/_pytest/python.py", line 1561, in runtest
  self.ihook.pytest_pyfunc_call(pyfuncitem=self)
File "/pytest/.tox/venv/lib/python3.8/site-packages/pluggy/hooks.py", line 286, in __call__
  return self._hookexec(self, self.get_hookimpls(), kwargs)
File "/pytest/.tox/venv/lib/python3.8/site-packages/pluggy/manager.py", line 93, in _hookexec
  return self._inner_hookexec(hook, methods, kwargs)
File "/pytest/.tox/venv/lib/python3.8/site-packages/pluggy/manager.py", line 84, in <lambda>
  self._inner_hookexec = lambda hook, methods, kwargs: hook.multicall(
File "/pytest/.tox/venv/lib/python3.8/site-packages/pluggy/callers.py", line 208, in _multicall
  return outcome.get_result()
File "/pytest/.tox/venv/lib/python3.8/site-packages/pluggy/callers.py", line 80, in get_result
  raise ex[1].with_traceback(ex[2])
File "/pytest/.tox/venv/lib/python3.8/site-packages/pluggy/callers.py", line 187, in _multicall
  res = hook_impl.function(*args)
File "/pytest/src/_pytest/python.py", line 177, in pytest_pyfunc_call
  result = testfunction(**testargs)
```

This PR removes the `<lambda>` frame, and follow up PRs will (try) to remove the `_hookexec` frame and the duplicate `_multicall` frame (which is cosmetic).

bluetech: I forgot to mention, it is intended to be reviewed commit-by-commit.
goodboy: @bluetech oh nice!

I'll try to look at this over the weekend :+1: 

Patch:
diff --git a/src/pluggy/callers.py b/src/pluggy/callers.py
index f66cc94..f4719fa 100644
--- a/src/pluggy/callers.py
+++ b/src/pluggy/callers.py
@@ -6,7 +6,7 @@ import sys
 from ._result import HookCallError, _Result, _raise_wrapfail
 
 
-def _multicall(hook_impls, caller_kwargs, firstresult=False):
+def _multicall(hook_name, hook_impls, caller_kwargs, firstresult):
     """Execute a call into multiple python functions/methods and return the
     result(s).
 
diff --git a/src/pluggy/hooks.py b/src/pluggy/hooks.py
index 572c02e..cbd1022 100644
--- a/src/pluggy/hooks.py
+++ b/src/pluggy/hooks.py
@@ -247,15 +247,23 @@ class _HookCaller:
             raise TypeError("hook calling supports only keyword arguments")
         assert not self.is_historic()
 
-        if self.spec and self.spec.argnames:
-            notincall = set(self.spec.argnames) - set(kwargs.keys())
-            if notincall:
-                warnings.warn(
-                    "Argument(s) {} which are declared in the hookspec "
-                    "can not be found in this hook call".format(tuple(notincall)),
-                    stacklevel=2,
-                )
-        return self._hookexec(self, self.get_hookimpls(), kwargs)
+        # This is written to avoid expensive operations when not needed.
+        if self.spec:
+            for argname in self.spec.argnames:
+                if argname not in kwargs:
+                    notincall = tuple(set(self.spec.argnames) - kwargs.keys())
+                    warnings.warn(
+                        "Argument(s) {} which are declared in the hookspec "
+                        "can not be found in this hook call".format(notincall),
+                        stacklevel=2,
+                    )
+                    break
+
+            firstresult = self.spec.opts.get("firstresult")
+        else:
+            firstresult = False
+
+        return self._hookexec(self.name, self.get_hookimpls(), kwargs, firstresult)
 
     def call_historic(self, result_callback=None, kwargs=None):
         """Call the hook with given ``kwargs`` for all registered plugins and
@@ -265,11 +273,11 @@ class _HookCaller:
         non-``None`` result obtained from a hook implementation.
         """
         self._call_history.append((kwargs or {}, result_callback))
-        # historizing hooks don't return results
-        res = self._hookexec(self, self.get_hookimpls(), kwargs)
+        # Historizing hooks don't return results.
+        # Remember firstresult isn't compatible with historic.
+        res = self._hookexec(self.name, self.get_hookimpls(), kwargs, False)
         if result_callback is None:
             return
-        # XXX: remember firstresult isn't compat with historic
         for x in res or []:
             result_callback(x)
 
@@ -291,7 +299,7 @@ class _HookCaller:
         """
         if self.is_historic():
             for kwargs, result_callback in self._call_history:
-                res = self._hookexec(self, [method], kwargs)
+                res = self._hookexec(self.name, [method], kwargs, False)
                 if res and result_callback is not None:
                     result_callback(res[0])
 
diff --git a/src/pluggy/manager.py b/src/pluggy/manager.py
index 7f37822..bba2f95 100644
--- a/src/pluggy/manager.py
+++ b/src/pluggy/manager.py
@@ -71,16 +71,12 @@ class PluginManager:
         self._plugin_distinfo = []
         self.trace = _tracing.TagTracer().get("pluginmanage")
         self.hook = _HookRelay()
-        self._inner_hookexec = lambda hook, methods, kwargs: _multicall(
-            methods,
-            kwargs,
-            firstresult=hook.spec.opts.get("firstresult") if hook.spec else False,
-        )
+        self._inner_hookexec = _multicall
 
-    def _hookexec(self, hook, methods, kwargs):
+    def _hookexec(self, hook_name, methods, kwargs, firstresult):
         # called from all hookcaller instances.
         # enable_tracing will set its own wrapping function at self._inner_hookexec
-        return self._inner_hookexec(hook, methods, kwargs)
+        return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
 
     def register(self, plugin, name=None):
         """ Register a plugin and return its canonical name or ``None`` if the name
@@ -311,10 +307,12 @@ class PluginManager:
         """
         oldcall = self._inner_hookexec
 
-        def traced_hookexec(hook, hook_impls, kwargs):
-            before(hook.name, hook_impls, kwargs)
-            outcome = _Result.from_call(lambda: oldcall(hook, hook_impls, kwargs))
-            after(outcome, hook.name, hook_impls, kwargs)
+        def traced_hookexec(hook_name, hook_impls, kwargs, firstresult):
+            before(hook_name, hook_impls, kwargs)
+            outcome = _Result.from_call(
+                lambda: oldcall(hook_name, hook_impls, kwargs, firstresult)
+            )
+            after(outcome, hook_name, hook_impls, kwargs)
             return outcome.get_result()
 
         self._inner_hookexec = traced_hookexec


Test patch:
diff --git a/testing/benchmark.py b/testing/benchmark.py
index cca4a75..7eec411 100644
--- a/testing/benchmark.py
+++ b/testing/benchmark.py
@@ -6,18 +6,11 @@ from pluggy import HookspecMarker, HookimplMarker
 from pluggy.hooks import HookImpl
 from pluggy.callers import _multicall
 
+
 hookspec = HookspecMarker("example")
 hookimpl = HookimplMarker("example")
 
 
-def MC(methods, kwargs, firstresult=False):
-    hookfuncs = []
-    for method in methods:
-        f = HookImpl(None, "<temp>", method, method.example_impl)
-        hookfuncs.append(f)
-    return _multicall(hookfuncs, kwargs, firstresult=firstresult)
-
-
 @hookimpl
 def hook(arg1, arg2, arg3):
     return arg1, arg2, arg3
@@ -38,9 +31,15 @@ def wrappers(request):
     return [wrapper for i in range(request.param)]
 
 
-def inner_exec(methods):
-    return MC(methods, {"arg1": 1, "arg2": 2, "arg3": 3})
-
-
 def test_hook_and_wrappers_speed(benchmark, hooks, wrappers):
-    benchmark(inner_exec, hooks + wrappers)
+    def setup():
+        hook_name = "foo"
+        hook_impls = []
+        for method in hooks + wrappers:
+            f = HookImpl(None, "<temp>", method, method.example_impl)
+            hook_impls.append(f)
+        caller_kwargs = {"arg1": 1, "arg2": 2, "arg3": 3}
+        firstresult = False
+        return (hook_name, hook_impls, caller_kwargs, firstresult), {}
+
+    benchmark.pedantic(_multicall, setup=setup)
diff --git a/testing/test_multicall.py b/testing/test_multicall.py
index 79f179f..554ed19 100644
--- a/testing/test_multicall.py
+++ b/testing/test_multicall.py
@@ -14,7 +14,7 @@ def MC(methods, kwargs, firstresult=False):
     for method in methods:
         f = HookImpl(None, "<temp>", method, method.example_impl)
         hookfuncs.append(f)
-    return caller(hookfuncs, kwargs, firstresult=firstresult)
+    return caller("foo", hookfuncs, kwargs, firstresult)
 
 
 def test_keyword_args():