-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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
gh-93103: Parser uses PyConfig.parser_debug instead of Py_DebugFlag #93106
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but left one comment
@@ -32,7 +32,7 @@ | |||
#include "pegen.h" | |||
|
|||
#if defined(Py_DEBUG) && defined(Py_BUILD_CORE) | |||
# define D(x) if (Py_DebugFlag) x; | |||
# define D(x) if (_Py_GetConfig()->parser_debug) { x; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this makes the debug run slower due to the function call (is everywhere)🤔
Could you do a quick check please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change only affects the debug build. I don't know how to measure the parser performance. Any suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this only affects debug mode, but I want to know by how much.
A quick check is running with "python -d" some big python file that doesn't actually execute any code and compare the timing before and after this PR. I don't need anything exact, just a rough estimate
Benchmark with a few large Python files: import pyperf
import tokenize
# wc -l $(find -name "*.py")|sort -n
large_files = """
5259 ./Tools/clinic/clinic.py
5553 ./Lib/test/test_email/test_email.py
5577 ./Lib/test/test_argparse.py
5659 ./Lib/test/test_logging.py
5806 ./Lib/test/test_descr.py
5878 ./Lib/test/test_decimal.py
5993 ./Lib/test/_test_multiprocessing.py
6425 ./Lib/_pydecimal.py
6626 ./Lib/test/datetimetester.py
6664 ./Lib/test/test_socket.py
7325 ./Lib/test/test_typing.py
15534 ./Lib/pydoc_data/topics.py
"""
large_files = [line.split()[-1] for line in large_files.splitlines() if line.strip()]
files = []
for filename in large_files:
with tokenize.open(filename) as fp:
content = fp.read()
files.append((filename, content))
def bench(files):
for filename, content in files:
compile(content, filename, "exec")
runner = pyperf.Runner()
runner.bench_func('compile', bench, files) Python built with:
Comparison:
Oh, it's way slower. |
* Replace deprecated Py_DebugFlag with PyConfig.parser_debug in the parser. * Add Parser.debug member. * Add tok_state.debug member. * Py_FrozenMain(): Replace Py_VerboseFlag with PyConfig.verbose.
I rewrote my PR to add a member to the Parser structure instead. New benchmark:
IMO the slowdown is now acceptable. I didn't use CPU isolation for this benchmark, I was using my laptop to do other stuffs in the meanwhile ;-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking. I think the new version is very acceptable 👍
Thanks for working on this
I didn't expect that the Is this debug mode really useful for someone working on the parser? I'm surprised that it has a command line option (-d) and an environment variable (PYTHONDEBUG=1). It's very verbose :-) |
Follow-up PR to better document this option (for experts only!!!): #93186 |
Replace deprecated Py_DebugFlag with PyConfig.parser_debug in the
parser.