Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize hook calling a bit #280

Merged
merged 4 commits into from Jul 2, 2020
Merged

Conversation

bluetech
Copy link
Member

@bluetech bluetech commented Jun 26, 2020

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,

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
Copy link
Member Author

I forgot to mention, it is intended to be reviewed commit-by-commit.

@goodboy
Copy link
Contributor

goodboy commented Jun 26, 2020

@bluetech oh nice!

I'll try to look at this over the weekend 👍

@bluetech bluetech force-pushed the optimize-call2 branch 2 times, most recently from 2d5426b to c7b3ba9 Compare June 29, 2020 12:47
A dict keys view supports set-like operations.
This check is quite expensive, try to reduce its overhead.
PluginManager adds an adapter lambda to the hook call path. This adds
overhead and makes the stack trace more messy.

Change the call convention such that the adaptation is not needed, and
remove the lambda.
@bluetech
Copy link
Member Author

Rebased, doesn't depend on other PRs now.

I also remoeved one of the micro-optimization commits, on second thought it's probably not worth it and is distracting.

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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is looping through the args everytime hoping for none missing from the call faster then just always checking what's missing using the set difference?

Seems like in average case this will be slower (assuming calls are written correctly most of the time)?
I'm not actually sure I can think of a case where this is faster?
I don't think a for loop will ever be faster then a set difference but I could be wrong?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an unscientific benchmark (Python 3.8, Archlinux), also added a variant which uses issubset. Only checks the happy path.

def old(argnames, kwargs):
    if argnames:
        notincall = set(argnames) - kwargs.keys()
        if notincall:
            pass

def old_subset(argnames, kwargs):
    if argnames:
        if not set(argnames).issubset(kwargs.keys()):
            pass

def new(argnames, kwargs):
    for argname in argnames:
        if argname not in kwargs:
            break

import timeit
kwargs = {'a': 0, 'b': 1, 'c': 2, 'd': 3, 'e': 4}
argnames = list(kwargs)
print("old:       ", timeit.timeit("old(argnames, kwargs)", "from __main__ import old, argnames, kwargs"))
print("old_subset:", timeit.timeit("old_subset(argnames, kwargs)", "from __main__ import old_subset, argnames, kwargs"))
print("new:       ", timeit.timeit("new(argnames, kwargs)", "from __main__ import new, argnames, kwargs"))

Output:

old:        0.7920419139554724
old_subset: 0.6716860989108682
new:        0.2587350399699062

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Python 3.8, Archlinux)

You're of my kind 😸

new: 0.2587350399699062

So slick; I guess for the win 🏄‍♂️

)
break

firstresult = self.spec.opts.get("firstresult")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exactly what I had in mind :)

Copy link
Contributor

@goodboy goodboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job @bluetech.

Really like the touches to the benchmark tests as well 👍

Really superb work.

@goodboy goodboy merged commit 0a064fe into pytest-dev:master Jul 2, 2020
@bluetech bluetech deleted the optimize-call2 branch September 29, 2020 06:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants