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

remove unreachable warning in old_build_ext #4224

Closed
wants to merge 1 commit into from
Closed

remove unreachable warning in old_build_ext #4224

wants to merge 1 commit into from

Conversation

asottile
Copy link

this warning is impossible because Cython.Distutils imports
Cython.Distutils.build_ext which in turn imports
Cython.Distutils.old_build_ext and the warning does not fire when
Cython.Distutils.build_ext is in the parent stack frames.

I found this particular piece of code to be contributing almost 5%
of the startup cost of an application I contribute to. The problem is
due to the use of inspect.getouterframes(...). This eventually calls
inspect.getmodule which does a sys.modules.copy() which is very slow
for lots of modules.

Here's an example piece of code which demonstrates the performance issue:

set -euxo pipefail

rm -rf foo tmpvenv out.svg

virtualenv tempvenv
tempvenv/bin/pip install -e .

mkdir foo
cat > foo/__main__.py <<EOF
import pkgutil
import foo

for _, name, _ in pkgutil.iter_modules(foo.__path__, f'{foo.__name__}.'):
    __import__(name)

import Cython.Distutils
EOF

set +x
touch foo/__init__.py foo/a{0..10000}.py
set -x

time tempvenv/bin/python -m foo

while this seems a bit out there, there's far more than 10k modules in the
code base I was dealing with.

before my change this was taking ~5 seconds. after my change it takes ~2.

this warning is impossible because `Cython.Distutils` imports
`Cython.Distutils.build_ext` which in turn imports
`Cython.Distutils.old_build_ext` and the warning does not fire when
`Cython.Distutils.build_ext` is in the parent stack frames.

I found this particular piece of code to be contributing almost 5%
of the startup cost of an application I contribute to.  The problem is
due to the use of `inspect.getouterframes(...)`.  This eventually calls
`inspect.getmodule` which does a `sys.modules.copy()` which is very slow
for lots of modules.

Here's an example piece of code which demonstrates the performance issue:

```bash
set -euxo pipefail

rm -rf foo tmpvenv out.svg

virtualenv tempvenv
tempvenv/bin/pip install -e .

mkdir foo
cat > foo/__main__.py <<EOF
import pkgutil
import foo

for _, name, _ in pkgutil.iter_modules(foo.__path__, f'{foo.__name__}.'):
    __import__(name)

import Cython.Distutils
EOF

set +x
touch foo/__init__.py foo/a{0..10000}.py
set -x

time tempvenv/bin/python -m foo
```

while this seems a bit out there, there's far more than 10k modules in the
code base I was dealing with.

before my change this was taking ~5 seconds.  after my change it takes ~2.
@asottile
Copy link
Author

asottile commented Jun 12, 2021

if it was instead wanted to speed up this code, here's an alternative which avoids the expensive inspect calls:

diff --git a/Cython/Distutils/old_build_ext.py b/Cython/Distutils/old_build_ext.py
index 7c2774aa2..8881a0291 100644
--- a/Cython/Distutils/old_build_ext.py
+++ b/Cython/Distutils/old_build_ext.py
@@ -25,19 +25,23 @@ except ImportError:
     basestring = str
 
 
-def _check_stack(path):
-    try:
-        for frame in inspect.getouterframes(inspect.currentframe(), 0):
-            if path in frame[1].replace(os.sep, '/'):
-                return True
-    except Exception:
-        pass
+def _in_build():
+    paths = (
+        'setuptools/extensions.py',
+        'pyximport/pyxbuild.py',
+    )
+    frame = inspect.currentframe()
+    while frame is not None:
+        normslashes = frame.f_code.co_filename.replace(os.sep, '/')
+        if any(path in normslashes for path in paths):
+            return True
+        frame = frame.f_back
+
     return False
 
 
-if (not _check_stack('setuptools/extensions.py')
-        and not _check_stack('pyximport/pyxbuild.py')
-        and not _check_stack('Cython/Distutils/build_ext.py')):
+if not _in_build():
     warnings.warn(
         "Cython.Distutils.old_build_ext does not properly handle dependencies "
         "and is deprecated.")

I don't think this is desirable since it will cause a warning any time someone imports Cython.Distutils not from setuptools or pyximport so that's why I opted for the code deletion instead

@scoder scoder closed this in 1748162 Jun 14, 2021
@scoder
Copy link
Contributor

scoder commented Jun 14, 2021

Thanks for reporting this. I'd like to keep the warning. The check for build_ext.py in the frames was meant to avoid showing it when someone imports "the right thing", so it's unfortunate that it doesn't work as intended.

I've commented out the code for now and added a FIXME. We should eventually repair it as intended, but having it as it is now is useless. And harmful, as you pointed out.

@scoder scoder added this to the 3.0 milestone Jun 14, 2021
@asottile
Copy link
Author

seems wrong to take authorship of the patch, but to each their own

@asottile asottile deleted the speed_up_old_build_ext_warning branch June 14, 2021 15:28
@scoder
Copy link
Contributor

scoder commented Jun 14, 2021

seems wrong to take authorship of the patch, but to each their own

Sorry for that. You're right, I could have mentioned your name in the commit comment.
Since this has a user visible effect, I'll make sure you're mentioned in the changelog (as we always do).

@asottile
Copy link
Author

seems wrong to take authorship of the patch, but to each their own

Sorry for that. You're right, I could have mentioned your name in the commit comment.
Since this has a user visible effect, I'll make sure you're mentioned in the changelog (as we always do).

don't worry, I won't be contributing again

@scoder
Copy link
Contributor

scoder commented Jun 14, 2021

Thank you for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants