bpo-2506: Add -X noopt command line option by vstinner · Pull Request #13600 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation31 Commits2 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Add -X noopt command line option to disable compiler optimizations.
- Add sys.flags.noopt flag.
- importlib uses
.noopt.pycsuffix for .pyc files
if sys.flags.noopt is true. - Add PyConfig.optimize parameter.
- compiler: rename c_optimize to c_optimization_level,
add c_optimize.
Co-Authored-By: Yury Selivanov yury@magic.io
https://bugs.python.org/issue2506
@nedbat: Would you mind to test this change, maybe also review it?
| @@ -440,6 +441,9 @@ always available. |
|---|
| Added ``dev_mode`` attribute for the new :option:`-X` ``dev`` flag |
| and ``utf8_mode`` attribute for the new :option:`-X` ``utf8`` flag. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to change these two (and line 424-425) for consistent style :)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose to not change these on purpose, it would be an unrelated change. Someone can work on a PR to update these once this PR is merged ;-)
I ran the Python test suite with -X noopt and I got multiple failures. I fixed all of them. I had to make additional changes:
- importlib.util.cache_from_source() gets a new noopt keyword-only parameter.
- Update subprocess._optim_args_from_interpreter_flags().
- Add support.requires_compiler_optimizations()
- Update unit tests which rely on compiler optimizations for noopt: add @support.requires_compiler_optimizations.
Azure Pipelines PR: test_venv failed with a timeout (20 min) in the win32 job, but passed when run again. It's unrelated.
bpo-2506: Add -X noopt command line option [WIP] bpo-2506: Add -X noopt command line option
Oh wow. When I ran the test suite using -X noopt, I found more and more issues, and I had to modify more ane more code. I didn't expect that I would have to go up to compile() to add a new parameter. It is need indirectly by distutils which uses py_compile.
IMHO distutils must simply ignore noopt and always enable compiler optimizations. I don't see much value in distributing non optimized .pyc files on purpose. The common case is to expect optimized .pyc files.
I will not have time to finish this PR and review it carefully before Friday (Python 3.8 feature freeze), so I prefer to tag the PR as WIP.
I agree with @vstinner that worrying about compile() is unnecessary. If you view this as an analysis tool then the vast majority of it is going to be coming from .pyc files only.
It is unfortunate that in the current state of the code, you can’t control the compilation of pyc files in distutils build commands with options: the result depends on the -B/-O options passed to the python command itself.
I had changed it in distutils2 but thought it could not be changed in distutils.
https://hg.python.org/distutils2/rev/7c0a88497b5c
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very large change. Are all of them necessary? Could we encode this option as special optimization level or compiler flag? This might keep more code unchanged.
| @@ -27,7 +27,7 @@ byte-code cache files in the directory containing the source code. |
|---|
| Exception raised when an error occurs while attempting to compile the file. |
| .. function:: compile(file, cfile=None, dfile=None, doraise=False, optimize=-1, invalidation_mode=PycInvalidationMode.TIMESTAMP) |
| .. function:: compile(file, cfile=None, dfile=None, doraise=False, optimize=-1, invalidation_mode=PycInvalidationMode.TIMESTAMP, *, noopt=False) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optimize and noopt. It looks slightly weird.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know but I failed to find better names.
| PyCompilerFlags *flags, |
|---|
| int optimization_level, |
| PyArena *arena, |
| int noopt); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not it be controlled by flags or optimization_level?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optimization_level=0 still means having optimizations enabled. I cannot change to keep the backward compatibility.
Do you think that it would be better to add a new PyCF_xxx flag to avoid adding this new function?
| @@ -158,7 +158,8 @@ struct compiler { |
|---|
| PyFutureFeatures *c_future; /* pointer to module's __future__ */ |
| PyCompilerFlags *c_flags; |
| int c_optimize; /* optimization level */ |
| int c_optimize; /* optimize bytecode? */ |
| int c_optimization_level; /* optimization level */ |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use -1 for noopt?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-1 has a special meaning for compile functions. I prefer to have a separated field to avoid confusion.
@serhiy-storchaka: "This is a very large change. Are all of them necessary? Could we encode this option as special optimization level or compiler flag? This might keep more code unchanged."
In GCC, gcc -O0 really means "no optimization". In Python, sadly, for historical reasons, optimization_level=0 still means "optimize but only with a few optimizations". It's not "don't optimize". And we cannot change the meaning of optimization_level=0: it would just break too much code. That's why I chose to add a new option.
I dislike optimization_level=-1. It sounds really surprising that an optimization level can be negative.
[WIP] bpo-2506: Add -X noopt command line option bpo-2506: Add -X noopt command line option
Add -X noopt command line option to disable compiler optimizations.
distutils ignores noopt: always enable compiler optimizations.
- Add sys.flags.noopt flag.
- Add 'noopt' keyword-only parameter to builtin compile(), importlib.util.cache_from_source() and py_compile.compile().
- importlib: SourceLoader gets an additional keyword-only '_noopt' parameter. It is used by py_compile.compile().
- importlib uses
.noopt.pycsuffix for .pyc files if sys.flags.noopt is true. - Add PyConfig.optimize parameter.
- compiler: rename c_optimize to c_optimization_level, add c_optimize.
- Update subprocess._optim_args_from_interpreter_flags().
- Add support.requires_compiler_optimizations()
- Update unit tests which rely on compiler optimizations for noopt: add @support.requires_compiler_optimizations.
- Add _PyAST_Compile() and _Py_CompileString() private functions: they have an additional 'noopt' parameter.
- Set _Py_CONFIG_VERSION to 2
Co-Authored-By: Yury Selivanov yury@magic.io
I rebased my PR:
- The PR now targets Python 3.9: update the documentation, changes are now documented in What's New in Python 3.9
- Document changes
- Update compile.c for the new BEGIN_DO_NOT_EMIT_BYTECODE
Latest commit message:
commit fa659c13f8d000f7e8b229e5b701df99f778cdc6
Author: Victor Stinner <vstinner@redhat.com>
Date: Mon May 27 23:58:28 2019 +0200
[bpo-2506](https://bugs.python.org/issue2506): Add -X noopt command line option
Add -X noopt command line option to disable compiler optimizations.
distutils ignores noopt: always enable compiler optimizations.
* Add sys.flags.noopt flag.
* Add 'noopt' keyword-only parameter to
builtin compile(), importlib.util.cache_from_source() and
py_compile.compile().
* importlib: SourceLoader gets an additional keyword-only '_noopt'
parameter. It is used by py_compile.compile().
* importlib uses ``.noopt.pyc`` suffix for .pyc files
if sys.flags.noopt is true.
* Add PyConfig.optimize parameter.
* compiler: rename c_optimize to c_optimization_level,
add c_optimize.
* Update subprocess._optim_args_from_interpreter_flags().
* Add support.requires_compiler_optimizations()
* Update unit tests which rely on compiler optimizations for noopt:
add @support.requires_compiler_optimizations.
* Add _PyAST_Compile() and _Py_CompileString() private functions:
they have an additional 'noopt' parameter.
* Set _Py_CONFIG_VERSION to 2
Co-Authored-By: Yury Selivanov <yury@magic.io>
Open questions:
- Do we really want this feature? See https://bugs.python.org/issue2506 for the rationale.
- "optimize", "optimization_level", "noopt": naming is hard, I failed to find better names... The main blocker issue is that compile(optimize=0) does not disable optimization and that cannot be changed to keep the backward compatibility.
- Do we need to modify compile(), importlib.util.cache_from_source() and py_compile.compile()? I began with a minimum change but I got many test failures. So slowly, I had to modify more and more code. IMO we need to modify these 3 functions.
- How to handle PyConfig structure update? I changed _Py_CONFIG_VERSION from 1 to 2, but the codes doesn't implement ABI compatibility. This may be fixed in a separated PR: before or after this one.
- compile.c: I added c_optimization_level field to the compiler structure, but I understood that @serhiy-storchaka would prefer to set compiler.c_optimize to -1 to disable optimizations. I chose to add a new field because -1 has a special meaning for function arguments. I prefer a separated field to avoid confusion.
- compile.c: should we add a new
PyCF_xxxflag rather than adding new _Py_CompileString() and _PyAST_Compile() functions?
This PR is quite large, but I'm not sure how to split it into smaller changes. I'm not sure that it can be splitted into smaller atomic changes, since all changes are related.
- I chose to add a new field because -1 has a special meaning for function arguments. I prefer a separated field to avoid confusion.
I understand why you prefered that option, but I find it very confusing. I agree with Serhiy in that it would be ideal if we can use compiler.c_optimize to signal the no-opt state.
Open questions:
How to handle PyConfig structure update? I changed _Py_CONFIG_VERSION from 1 to 2, but the codes doesn't implement ABI compatibility. This may be fixed in a separated PR: before or after this one.
So I tested and ... it didn't work at all :-( The PEP 587 had a flaw which prevented that! I just fixed the implementation to add a "struct_size" field to PyConfig. So it becomes possible to add new fields to PyConfig in Python 3.9, and be ABI compatible with Python 3.8.
The fix: https://bugs.python.org/issue38304
Open questions: How to handle PyConfig structure update? I changed _Py_CONFIG_VERSION from 1 to 2, but the codes doesn't implement ABI compatibility. This may be fixed in a separated PR: before or after this one.
New update: the whole idea of a stable ABI for PyConfig has been abandoned. We can freely add new fields in Python 3.9 without having to care about backward compatibility.
I'm no longer interesting to push this feature. The implementation is quite intrusive and @brettcannon didn't like that I passed the "no optimization" flag to so many places. I don't need this feature myself, so I'm not really motivated to fight each individual issue.
If someone wants to complete this change, feel free to copy/fork this PR. You can start from https://github.com/python/cpython/pull/13600.patch for example.
By the way, this PR was created from an old patch by Yury Selivanov.
Reviewers
