Improve import performance of assertion rewrite. Fixes #3918. #3919

fabioz · 2018-08-31T13:02:54Z

Some notes:

On my use case there are many imports (around 3000 imports) and there are many entries in sys.path (67 to be more exact) -- this makes imp.find_import very slow (it does way too many calls to os.stat in the scenario) -- on my use case imports are around 30% faster with my patch in place in Py 2 and 40% faster on Py 3 on my measurements (granted my case is probably an edge case for imp.find_import).
I took a look at https://www.python.org/dev/peps/pep-0302/ and it says the following:

The following set of methods may be implemented if support for (for example) Freeze-like tools is desirable. It consists of three additional methods which, to make it easier for the caller, each of which should be implemented, or none at all:

loader.is_package(fullname)
loader.get_code(fullname)
loader.get_source(fullname)

So, as the loader had is_package and not the others I simply removed is_package and things still seem to be working -- which is good because it was also calling imp.find_module (only on Python 3, it seems it was unused on Python 2 -- which also means imports were slower on Python 3 than on Python 2).

Fix #3918

nicoddemus

Great work @fabioz, thanks!

src/_pytest/assertion/rewrite.py

changelog/3918.bugfix.rst

…directories

nicoddemus

I fixed the tests and added unittests for the early bailout.

It would be nice to have a second set of eyes from other maintainers over this change. 👍

codecov · 2018-09-01T14:14:14Z

Codecov Report

Merging #3919 into master will decrease coverage by 1.24%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3919      +/-   ##
==========================================
- Coverage   96.24%   94.99%   -1.25%     
==========================================
  Files         108      108              
  Lines       23353    23437      +84     
  Branches        0     2332    +2332     
==========================================
- Hits        22475    22265     -210     
- Misses        878      881       +3     
- Partials        0      291     +291

Flag	Coverage Δ
#doctesting	`28.68% <53.4%> (-3.2%)`	⬇️
#nobyte	`90.05% <100%> (-3.47%)`	⬇️
#numpy	`28.32% <51.13%> (-3.11%)`	⬇️
#pexpect	`47.83% <72.34%> (-5.75%)`	⬇️
#pluggymaster	`93.74% <100%> (-1.33%)`	⬇️
#py27	`93.04% <100%> (-1.57%)`	⬇️
#py34	`92.28% <100%> (-1.6%)`	⬇️
#py35	`92.29% <100%> (-1.6%)`	⬇️
#py36	`93.29% <100%> (-1.54%)`	⬇️
#py37	`92.46% <100%> (-1.6%)`	⬇️
#trial	`31.25% <51.13%> (-2.85%)`	⬇️
#xdist	`93.22% <100%> (-1.42%)`	⬇️

Impacted Files	Coverage Δ
src/_pytest/assertion/rewrite.py	`96% <100%> (-1.17%)`	⬇️
testing/test_assertrewrite.py	`83.78% <100%> (+0.02%)`	⬆️
src/_pytest/main.py	`96.19% <100%> (-2.03%)`	⬇️
testing/freeze/runtests_script.py	`0% <0%> (-25%)`	⬇️
testing/freeze/tox_run.py	`0% <0%> (-14.29%)`	⬇️
testing/freeze/create_executable.py	`0% <0%> (-12.5%)`	⬇️
src/_pytest/assertion/__init__.py	`86.36% <0%> (-10.61%)`	⬇️
testing/test_modimport.py	`80% <0%> (-10%)`	⬇️
testing/test_argcomplete.py	`65.62% <0%> (-7.82%)`	⬇️
src/_pytest/_code/_py2traceback.py	`85.71% <0%> (-7.15%)`	⬇️
... and 61 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 019e33e...eec7081. Read the comment docs.

coveralls · 2018-09-01T14:47:35Z

Coverage increased (+0.03%) to 94.081% when pulling eec7081 on fabioz:master into 019e33e on pytest-dev:master.

fabioz

As the def is_package(self, name): was added back, it should probably use the new self._imp_find_module instead of imp.find_module.

fabioz

Also, imp.find_module is actually deprecated (seeing 3.6 docs) -- if it's only going to be used for imp.find_module, its contents can probably be extracted just for the PKG_DIRECTORY case (there's a pure-python version which may be seen in imp.find_module).

nicoddemus · 2018-09-03T13:19:16Z

As the def is_package(self, name): was added back, it should probably use the new self._imp_find_module instead of imp.find_module.

I added it back because a test was failing without it. Good catch about is_package using imp.find_module, just changed it. 👍

nicoddemus · 2018-09-03T13:29:09Z

Also, imp.find_module is actually deprecated (seeing 3.6 docs) -- if it's only going to be used for imp.find_module, its contents can probably be extracted just for the PKG_DIRECTORY case (there's a pure-python version which may be seen in imp.find_module).

Yeah, as we talked in person, it is nightmarish to keep this code working across all Python versions we support, so we are kind of forced to use the deprecated module. We will probably revisit the rewriting hook completely when we can drop Python 2 in a year or so. 😁

fabioz · 2018-09-03T13:38:32Z

Also, imp.find_module is actually deprecated (seeing 3.6 docs) -- if it's only going to be used for imp.find_module, its contents can probably be extracted just for the PKG_DIRECTORY case (there's a pure-python version which may be seen in imp.find_module).

Yeah, as we talked in person, it is nightmarish to keep this code working across all Python versions we support, so we are kind of forced to use the deprecated module. We will probably revisit the rewriting hook completely when we can drop Python 2 in a year or so. 😁

Well, in this case I was actually talking about the is_package use case (not the other case) -- and in this situation it seems to be used only on Python 3 ;)

nicoddemus · 2018-09-03T13:40:38Z

Well, in this case I was actually talking about the is_package use case (not the other case) -- and in this situation it seems to be used only on Python 3 ;)

Oh sorry, I did not catch the entirety of the message, my bad. Please feel free to experiment and push other commits here if you want. 👍

fabioz · 2018-09-05T11:15:32Z

Well, in this case I was actually talking about the is_package use case (not the other case) -- and in this situation it seems to be used only on Python 3 ;)

Oh sorry, I did not catch the entirety of the message, my bad. Please feel free to experiment and push other commits here if you want. 👍

I'll do that later on (I'd like to get this one merged first and then experiment more on that idea afterwards as one improvement is not dependent on the other).

nicoddemus · 2018-09-05T12:30:16Z

Sounds good!

RonnyPfannschmidt · 2018-09-23T10:35:05Z

@thedrow pytest is already internally using pathlib and keeps adding more uses (using the backports in pathlib2 for python <3.6) - so we can do this right now

jeffwidman · 2018-10-27T06:21:00Z

@thedrow / @RonnyPfannschmidt was the pathlib optimization ever implemented?

nicoddemus requested changes Aug 31, 2018

View reviewed changes

src/_pytest/assertion/rewrite.py Outdated Show resolved Hide resolved

changelog/3918.bugfix.rst Outdated Show resolved Hide resolved

fabioz force-pushed the master branch 2 times, most recently from f40c9d8 to d53e449 Compare August 31, 2018 15:23

fabioz and others added 2 commits August 31, 2018 12:27

Improve performance of assertion rewriting. Fixes pytest-dev#3918

d53e449

Add tests for early rewrite bailout code and handle patterns with sub…

4675912

…directories

nicoddemus approved these changes Sep 1, 2018

View reviewed changes

Fix linting

885b8a3

nicoddemus changed the title ~~Fix import performance on assertion rewrite. Fixes #3918.~~ Improve import performance on assertion rewrite. Fixes #3918. Sep 1, 2018

nicoddemus changed the title ~~Improve import performance on assertion rewrite. Fixes #3918.~~ Improve import performance of assertion rewrite. Fixes #3918. Sep 1, 2018

fabioz commented Sep 3, 2018

View reviewed changes

Make AssertionRewritingrHook use imp_find_module

eec7081

RonnyPfannschmidt merged commit 410d576 into pytest-dev:master Sep 5, 2018

iwanb mentioned this pull request Jan 31, 2020

Assertion rewriting breaks jedi due to missing InspectLoader support #6643

Open

Uh oh!

Improve import performance of assertion rewrite. Fixes #3918. #3919

Improve import performance of assertion rewrite. Fixes #3918. #3919

Uh oh!

Conversation

fabioz commented Aug 31, 2018 • edited by nicoddemus Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicoddemus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nicoddemus left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

coveralls commented Sep 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabioz left a comment

Choose a reason for hiding this comment

Uh oh!

fabioz left a comment

Choose a reason for hiding this comment

Uh oh!

nicoddemus commented Sep 3, 2018

Uh oh!

nicoddemus commented Sep 3, 2018

Uh oh!

fabioz commented Sep 3, 2018

Uh oh!

nicoddemus commented Sep 3, 2018

Uh oh!

fabioz commented Sep 5, 2018

Uh oh!

nicoddemus commented Sep 5, 2018

Uh oh!

RonnyPfannschmidt commented Sep 23, 2018

Uh oh!

jeffwidman commented Oct 27, 2018

Uh oh!

Uh oh!

fabioz commented Aug 31, 2018 •

edited by nicoddemus

Loading

nicoddemus left a comment •

edited

Loading

codecov bot commented Sep 1, 2018 •

edited

Loading

coveralls commented Sep 1, 2018 •

edited

Loading