Skip to content

bpo-2506: Add -X noopt command line option - #13600

Closed
vstinner wants to merge 2 commits into
python:masterfrom
vstinner:noopt
Closed

bpo-2506: Add -X noopt command line option#13600
vstinner wants to merge 2 commits into
python:masterfrom
vstinner:noopt

Conversation

@vstinner

@vstinnervstinner commented May 27, 2019

Copy link
Copy Markdown
Member

Add -X noopt command line option to disable compiler optimizations.

  • Add sys.flags.noopt flag.
  • 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.

Co-Authored-By: Yury Selivanov yury@magic.io

https://bugs.python.org/issue2506

@vstinner

Copy link
Copy Markdown
MemberAuthor

@nedbat: Would you mind to test this change, maybe also review it?

Comment threadDoc/library/sys.rst Outdated
Comment threadDoc/library/sys.rst Outdated

Copy link
Copy Markdown
Member

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 :)

Copy link
Copy Markdown
MemberAuthor

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 ;-)

@vstinner
vstinner requested a review from gpshead as a code ownerMay 28, 2019 16:15
@vstinner

Copy link
Copy Markdown
MemberAuthor

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.

Comment threadDoc/library/importlib.rst Outdated
@vstinner

Copy link
Copy Markdown
MemberAuthor

Azure Pipelines PR: test_venv failed with a timeout (20 min) in the win32 job, but passed when run again. It's unrelated.

@vstinnervstinner changed the title bpo-2506: Add -X noopt command line option[WIP] bpo-2506: Add -X noopt command line optionMay 29, 2019
@vstinner

Copy link
Copy Markdown
MemberAuthor

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.

@brettcannon

Copy link
Copy Markdown
Member

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.

@merwok

Copy link
Copy Markdown
Member

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

@serhiy-storchakaserhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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.

Comment threadDoc/library/py_compile.rst Outdated

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
MemberAuthor

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.

Comment threadDoc/c-api/init_config.rst Outdated
Comment threadInclude/compile.h Outdated

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
MemberAuthor

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?

Comment threadLib/test/support/__init__.py Outdated
Comment threadPython/compile.c Outdated

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
MemberAuthor

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.

@vstinner

vstinner commented Jun 2, 2019

Copy link
Copy Markdown
MemberAuthor

@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.

@vstinnervstinner changed the title [WIP] bpo-2506: Add -X noopt command line optionbpo-2506: Add -X noopt command line optionAug 20, 2019
vstinnerand others added 2 commits August 20, 2019 13:46
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>
@vstinner

vstinner commented Aug 20, 2019

Copy link
Copy Markdown
MemberAuthor

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>

@vstinner

Copy link
Copy Markdown
MemberAuthor

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_xxx flag 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.

@pablogsal

Copy link
Copy Markdown
Member
  • 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.

@vstinner

Copy link
Copy Markdown
MemberAuthor

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

@vstinner

Copy link
Copy Markdown
MemberAuthor

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.

@vstinner

vstinner commented Mar 10, 2020

Copy link
Copy Markdown
MemberAuthor

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.

Sign up for freeto 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.

9 participants

@vstinner@brettcannon@merwok@pablogsal@asvetlov@serhiy-storchaka@tirkarthi@the-knights-who-say-ni@bedevere-bot