GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-93103: Parser uses PyConfig.parser_debug instead of Py_DebugFlag by vstinner · Pull Request #93106 · python/cpython

Conversation

@vstinner

@vstinner vstinner commented May 23, 2022

Copy link
Copy Markdown
Member

Replace deprecated Py_DebugFlag with PyConfig.parser_debug in the
parser.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vstinner vstinner added the skip news label May 23, 2022
@vstinner
vstinner requested review from lysnikolaou and pablogsal as code owners May 23, 2022 11:45
@bedevere-bot bedevere-bot added the awaiting core review label May 23, 2022
pablogsal
pablogsal approved these changes May 23, 2022
View reviewed changes

@pablogsal pablogsal left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Tools/peg_generator/pegen/c_generator.py Outdated

@pablogsal pablogsal May 23, 2022

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@vstinner vstinner May 23, 2022

Copy link
Copy Markdown
Member Author

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@pablogsal pablogsal May 24, 2022

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels May 23, 2022
@vstinner

vstinner commented May 24, 2022

Copy link
Copy Markdown
Member Author

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:

  • ./configure --with-pydebug && make
  • GCC 12.1.1 using -Og optimization level

Comparison:

$ python3 -m pyperf compare_to ref.json pr.json 
Mean +- std dev: [ref] 1.40 sec +- 0.02 sec -> [pr] 1.93 sec +- 0.01 sec: 1.37x slower

Oh, it's way slower.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner
gh-93103: Parser uses PyConfig.parser_debug instead of Py_DebugFlag
6e6efe7
* 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.
@vstinner

vstinner commented May 24, 2022

Copy link
Copy Markdown
Member Author

I rewrote my PR to add a member to the Parser structure instead. New benchmark:

$ python3 -m pyperf compare_to ref.json pr2.json 
Mean +- std dev: [ref] 1.40 sec +- 0.02 sec -> [pr2] 1.43 sec +- 0.04 sec: 1.02x slower

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

pablogsal reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pablogsal
pablogsal approved these changes May 24, 2022
View reviewed changes

@pablogsal pablogsal left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vstinner
vstinner merged commit 5115a16 into python:main May 24, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label May 24, 2022
@vstinner
vstinner deleted the parser branch May 24, 2022 20:35
@vstinner

vstinner commented May 24, 2022

Copy link
Copy Markdown
Member Author

I didn't expect that the D() macro would be called so often, and that the change would have such massive impact on the performance!

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented May 24, 2022

Copy link
Copy Markdown
Member Author

Follow-up PR to better document this option (for experts only!!!): #93186

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner vstinner mentioned this pull request May 27, 2022
_PyPegen_is_memoized() has a complexity of O(n) #93289
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pablogsal pablogsal pablogsal approved these changes
@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou lysnikolaou is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@vstinner @pablogsal @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.