Reduce calls to item.ihook

[lgeiger]

When scrolling through #7338 I noticed that the item.ihook property is called fairly often. There are two place where this access can be cached with trivial changes. When running empty.py calls to ihook are reduced from 14039 to 13039. While this doesn't result in significant performance gains I think it might still be useful to include since it doesn't increase code complexity.

[asottile]

would a better patch be to attach ihook directly instead of lazily? I'm not sure assigning a local variable for an attribute access is obvious / idiomatic enough and will likely regress

[lgeiger]

would a better patch be to attach ihook directly instead of lazily? I'm not sure assigning a local variable for an attribute access is obvious / idiomatic enough and will likely regress

I agree, making ihook a @cached_property would be much nicer, do you think that could be proplemativ? I only opted for assigning a local variable since I saw it in some other places in the code as well.

[bluetech]

Thanks for looking at this @lgeiger.

I prefer to avoid a cache in complex cases like this because it likely increases memory usage (didn't measure), and possibly has cache invalidation issues, and the gain is not big.

Preferably we make it not slow in the first place. At least we should know what makes it slow exactly...

So I prefer the initial version, which is a clear improvement.

[RonnyPfannschmidt]

I do wonder if a lru cache for the fsproxies would be a good path for this

there can be thousands of tests per file, so creating a instance per item when it's per path seems excessive

[lgeiger]

Thanks for the comments! I reverted to the original version and will take another look at it next week.

[nicoddemus]

I do wonder if a lru cache for the fsproxies would be a good path for this

In session.gethookproxy? I agree, but we can consider not using lru_cache because it keeps cached results in a "global" cache. A simple manual, local cache, might do the trick then (no need to keep a bound to the cache size I think).

Looking at the implementation, it seems a bit costly indeed to be done all the time, so a good candidate for caching:

pytest/src/_pytest/nodes.py

Lines 546 to 560 in 1ae4182

	def _gethookproxy(self, fspath: py.path.local):
	# check if we have the common case of running
	# hooks with all conftest.py files
	pm = self.config.pluginmanager
	my_conftestmodules = pm._getconftestmodules(
	fspath, self.config.getoption("importmode")
	)
	remove_mods = pm._conftest_plugins.difference(my_conftestmodules)
	if remove_mods:
	# one or more conftests are not in use at this fspath
	proxy = FSHookProxy(pm, remove_mods)
	else:
	# all plugins are active for this fspath
	proxy = self.config.hook
	return proxy

But _getconftestmodules and _conftest_plugins seem problematic now that I take a second look at it...

Given that it is not that simple, I think we can go forward with this PR and think of a better solution later.

[bluetech]

I prefer we don't cache this; it calls _getconftestmodules (which is cached itself) which imports stuff and whatnot, it depends on config and _conftest_plugins which can conceivable mutate, and it has a not-perfectly-clear inheritance structure, so it seems too dangerous. And the benefits are not great. I think we should look into optimizing the function itself, instead.

The current version is a simple win however, so 👍 on that.