hooks: keep hookimpls in a single list

The hookexec receives them in a single list (which is good), so make the
HookCaller keep them in a single array as well, so can avoid the list
concatenation in the hot call path.

This makes adding a hookimpl a little slower, but not by much, and it is
much colder than calling so the tradeoff makes sense. It is possible to
optimize by keeping track of the splitpoint instead of calculating it
every time, but I don't think it's worth it.

Author: bluetech

src/pluggy/_hooks.py

@@ -286,8 +286,7 @@ class _HookCaller:
         "name",
         "spec",
         "_hookexec",
-        "_wrappers",
-        "_nonwrappers",
+        "_hookimpls",
         "_call_history",
     )
 
@@ -300,8 +299,7 @@ def __init__(
     ) -> None:
         self.name: "Final" = name
         self._hookexec: "Final" = hook_execute
-        self._wrappers: "Final[List[HookImpl]]" = []
-        self._nonwrappers: "Final[List[HookImpl]]" = []
+        self._hookimpls: "Final[List[HookImpl]]" = []
         self._call_history: Optional[_CallHistory] = None
         self.spec: Optional[HookSpec] = None
         if specmodule_or_class is not None:
@@ -325,38 +323,38 @@ def is_historic(self) -> bool:
         return self._call_history is not None
 
     def _remove_plugin(self, plugin: _Plugin) -> None:
-        def remove(wrappers: List[HookImpl]) -> Optional[bool]:
-            for i, method in enumerate(wrappers):
-                if method.plugin == plugin:
-                    del wrappers[i]
-                    return True
-            return None
-
-        if remove(self._wrappers) is None:
-            if remove(self._nonwrappers) is None:
-                raise ValueError(f"plugin {plugin!r} not found")
+        for i, method in enumerate(self._hookimpls):
+            if method.plugin == plugin:
+                del self._hookimpls[i]
+                return
+        raise ValueError(f"plugin {plugin!r} not found")
 
     def get_hookimpls(self) -> List["HookImpl"]:
-        # Order is important for _hookexec
-        return self._nonwrappers + self._wrappers
+        return self._hookimpls.copy()
 
     def _add_hookimpl(self, hookimpl: "HookImpl") -> None:
         """Add an implementation to the callback chain."""
+        for i, method in enumerate(self._hookimpls):
+            if method.hookwrapper:
+                splitpoint = i
+                break
+        else:
+            splitpoint = len(self._hookimpls)
         if hookimpl.hookwrapper:
-            methods = self._wrappers
+            start, end = splitpoint, len(self._hookimpls)
         else:
-            methods = self._nonwrappers
+            start, end = 0, splitpoint
 
         if hookimpl.trylast:
-            methods.insert(0, hookimpl)
+            self._hookimpls.insert(start, hookimpl)
         elif hookimpl.tryfirst:
-            methods.append(hookimpl)
+            self._hookimpls.insert(end, hookimpl)
         else:
             # find last non-tryfirst method
-            i = len(methods) - 1
-            while i >= 0 and methods[i].tryfirst:
+            i = end - 1
+            while i >= start and self._hookimpls[i].tryfirst:
                 i -= 1
-            methods.insert(i + 1, hookimpl)
+            self._hookimpls.insert(i + 1, hookimpl)
 
     def __repr__(self) -> str:
         return f"<_HookCaller {self.name!r}>"
@@ -387,7 +385,7 @@ def __call__(self, *args: object, **kwargs: object) -> Any:
         ), "Cannot directly call a historic hook - use call_historic instead."
         self._verify_all_args_are_provided(kwargs)
         firstresult = self.spec.opts.get("firstresult", False) if self.spec else False
-        return self._hookexec(self.name, self.get_hookimpls(), kwargs, firstresult)
+        return self._hookexec(self.name, self._hookimpls, kwargs, firstresult)
 
     def call_historic(
         self,
@@ -406,7 +404,7 @@ def call_historic(
         self._call_history.append((kwargs, result_callback))
         # Historizing hooks don't return results.
         # Remember firstresult isn't compatible with historic.
-        res = self._hookexec(self.name, self.get_hookimpls(), kwargs, False)
+        res = self._hookexec(self.name, self._hookimpls, kwargs, False)
         if result_callback is None:
             return
         if isinstance(res, list):
@@ -429,13 +427,12 @@ def call_extra(
             "tryfirst": False,
             "specname": None,
         }
-        hookimpls = self.get_hookimpls()
+        hookimpls = self._hookimpls.copy()
         for method in methods:
             hookimpl = HookImpl(None, "<temp>", method, opts)
             # Find last non-tryfirst nonwrapper method.
             i = len(hookimpls) - 1
-            until = len(self._nonwrappers)
-            while i >= until and hookimpls[i].tryfirst:
+            while i >= 0 and hookimpls[i].tryfirst and not hookimpls[i].hookwrapper:
                 i -= 1
             hookimpls.insert(i + 1, hookimpl)
         firstresult = self.spec.opts.get("firstresult", False) if self.spec else False

testing/test_details.py

@@ -34,13 +34,18 @@ def x1meth2(self):
     pm = MyPluginManager(hookspec.project_name)
     pm.register(Plugin())
     pm.add_hookspecs(Spec)
-    assert not pm.hook.x1meth._nonwrappers[0].hookwrapper
-    assert not pm.hook.x1meth._nonwrappers[0].tryfirst
-    assert not pm.hook.x1meth._nonwrappers[0].trylast
-    assert not pm.hook.x1meth._nonwrappers[0].optionalhook
 
-    assert pm.hook.x1meth2._wrappers[0].tryfirst
-    assert pm.hook.x1meth2._wrappers[0].hookwrapper
+    hookimpls = pm.hook.x1meth.get_hookimpls()
+    assert len(hookimpls) == 1
+    assert not hookimpls[0].hookwrapper
+    assert not hookimpls[0].tryfirst
+    assert not hookimpls[0].trylast
+    assert not hookimpls[0].optionalhook
+
+    hookimpls = pm.hook.x1meth2.get_hookimpls()
+    assert len(hookimpls) == 1
+    assert hookimpls[0].hookwrapper
+    assert hookimpls[0].tryfirst
 
 
 def test_warn_when_deprecated_specified(recwarn) -> None:
@@ -127,6 +132,6 @@ def myhook(self):
 
     plugin = Plugin()
     pname = pm.register(plugin)
-    assert repr(pm.hook.myhook._nonwrappers[0]) == (
+    assert repr(pm.hook.myhook.get_hookimpls()[0]) == (
         f"<HookImpl plugin_name={pname!r}, plugin={plugin!r}>"
     )

testing/test_hookcaller.py

@@ -61,7 +61,7 @@ def he_method2() -> None:
     def he_method3() -> None:
         pass
 
-    assert funcs(hc._nonwrappers) == [he_method1, he_method2, he_method3]
+    assert funcs(hc.get_hookimpls()) == [he_method1, he_method2, he_method3]
 
 
 def test_adding_nonwrappers_trylast(hc: _HookCaller, addmeth: AddMeth) -> None:
@@ -77,7 +77,7 @@ def he_method1() -> None:
     def he_method1_b() -> None:
         pass
 
-    assert funcs(hc._nonwrappers) == [he_method1, he_method1_middle, he_method1_b]
+    assert funcs(hc.get_hookimpls()) == [he_method1, he_method1_middle, he_method1_b]
 
 
 def test_adding_nonwrappers_trylast3(hc: _HookCaller, addmeth: AddMeth) -> None:
@@ -97,7 +97,7 @@ def he_method1_c() -> None:
     def he_method1_d() -> None:
         pass
 
-    assert funcs(hc._nonwrappers) == [
+    assert funcs(hc.get_hookimpls()) == [
         he_method1_d,
         he_method1_b,
         he_method1_a,
@@ -118,7 +118,7 @@ def he_method1_b() -> None:
     def he_method1() -> None:
         pass
 
-    assert funcs(hc._nonwrappers) == [he_method1, he_method1_middle, he_method1_b]
+    assert funcs(hc.get_hookimpls()) == [he_method1, he_method1_middle, he_method1_b]
 
 
 def test_adding_nonwrappers_tryfirst(hc: _HookCaller, addmeth: AddMeth) -> None:
@@ -134,7 +134,7 @@ def he_method1_middle() -> None:
     def he_method1_b() -> None:
         pass
 
-    assert funcs(hc._nonwrappers) == [he_method1_middle, he_method1_b, he_method1]
+    assert funcs(hc.get_hookimpls()) == [he_method1_middle, he_method1_b, he_method1]
 
 
 def test_adding_wrappers_ordering(hc: _HookCaller, addmeth: AddMeth) -> None:
@@ -150,8 +150,11 @@ def he_method1_middle() -> None:
     def he_method3() -> None:
         pass
 
-    assert funcs(hc._nonwrappers) == [he_method1_middle]
-    assert funcs(hc._wrappers) == [he_method1, he_method3]
+    assert funcs(hc.get_hookimpls()) == [
+        he_method1_middle,
+        he_method1,
+        he_method3,
+    ]
 
 
 def test_adding_wrappers_ordering_tryfirst(hc: _HookCaller, addmeth: AddMeth) -> None:
@@ -163,8 +166,116 @@ def he_method1() -> None:
     def he_method2() -> None:
         pass
 
-    assert hc._nonwrappers == []
-    assert funcs(hc._wrappers) == [he_method2, he_method1]
+    assert funcs(hc.get_hookimpls()) == [he_method2, he_method1]
+
+
+def test_adding_wrappers_complex(hc: _HookCaller, addmeth: AddMeth) -> None:
+    assert funcs(hc.get_hookimpls()) == []
+
+    @addmeth(hookwrapper=True, trylast=True)
+    def m1() -> None:
+        ...
+
+    assert funcs(hc.get_hookimpls()) == [m1]
+
+    @addmeth()
+    def m2() -> None:
+        ...
+
+    assert funcs(hc.get_hookimpls()) == [m2, m1]
+
+    @addmeth(trylast=True)
+    def m3() -> None:
+        ...
+
+    assert funcs(hc.get_hookimpls()) == [m3, m2, m1]
+
+    @addmeth(hookwrapper=True)
+    def m4() -> None:
+        ...
+
+    assert funcs(hc.get_hookimpls()) == [m3, m2, m1, m4]
+
+    @addmeth(hookwrapper=True, tryfirst=True)
+    def m5() -> None:
+        ...
+
+    assert funcs(hc.get_hookimpls()) == [m3, m2, m1, m4, m5]
+
+    @addmeth(tryfirst=True)
+    def m6() -> None:
+        ...
+
+    assert funcs(hc.get_hookimpls()) == [m3, m2, m6, m1, m4, m5]
+
+    @addmeth()
+    def m7() -> None:
+        ...
+
+    assert funcs(hc.get_hookimpls()) == [m3, m2, m7, m6, m1, m4, m5]
+
+    @addmeth(hookwrapper=True)
+    def m8() -> None:
+        ...
+
+    assert funcs(hc.get_hookimpls()) == [m3, m2, m7, m6, m1, m4, m8, m5]
+
+    @addmeth(trylast=True)
+    def m9() -> None:
+        ...
+
+    assert funcs(hc.get_hookimpls()) == [m9, m3, m2, m7, m6, m1, m4, m8, m5]
+
+    @addmeth(tryfirst=True)
+    def m10() -> None:
+        ...
+
+    assert funcs(hc.get_hookimpls()) == [m9, m3, m2, m7, m6, m10, m1, m4, m8, m5]
+
+    @addmeth(hookwrapper=True, trylast=True)
+    def m11() -> None:
+        ...
+
+    assert funcs(hc.get_hookimpls()) == [m9, m3, m2, m7, m6, m10, m11, m1, m4, m8, m5]
+
+    @addmeth(hookwrapper=True)
+    def m12() -> None:
+        ...
+
+    assert funcs(hc.get_hookimpls()) == [
+        m9,
+        m3,
+        m2,
+        m7,
+        m6,
+        m10,
+        m11,
+        m1,
+        m4,
+        m8,
+        m12,
+        m5,
+    ]
+
+    @addmeth()
+    def m13() -> None:
+        ...
+
+    assert funcs(hc.get_hookimpls()) == [
+        m9,
+        m3,
+        m2,
+        m7,
+        m13,
+        m6,
+        m10,
+        m11,
+        m1,
+        m4,
+        m8,
+        m12,
+        m5,
+    ]
 
 
 def test_hookspec(pm: PluginManager) -> None: