◐ Shell
reader mode source ↗
Skip to content

gh-128563: Move lltrace into the frame struct#129113

Merged
Fidget-Spinner merged 15 commits into
python:mainfrom
Fidget-Spinner:move_lltrace_to_frame
Jan 21, 2025
Merged

gh-128563: Move lltrace into the frame struct#129113
Fidget-Spinner merged 15 commits into
python:mainfrom
Fidget-Spinner:move_lltrace_to_frame

Conversation

@Fidget-Spinner

@Fidget-Spinner Fidget-Spinner commented Jan 21, 2025 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

@bedevere-app bedevere-app Bot mentioned this pull request Jan 21, 2025
Closed
markshannon
markshannon requested changes Jan 21, 2025
View reviewed changes

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

Hide comment

The change to the size of the frame can be avoided by using a bitfield.

I'm curious why the atomic loads and stores are needed.

@bedevere-app

bedevere-app Bot commented Jan 21, 2025

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@Fidget-Spinner

Fidget-Spinner commented Jan 21, 2025

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again.

@bedevere-app

bedevere-app Bot commented Jan 21, 2025

Copy link
Copy Markdown

Thanks for making the requested changes!

@markshannon: please review the changes made to this pull request.

@bedevere-app bedevere-app Bot requested a review from markshannon January 21, 2025 10:43
markshannon
markshannon reviewed Jan 21, 2025
View reviewed changes

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

Hide comment

One small issue, otherwise looks good.

lltrace should be unsigned (uint8_t), and maybe bigger, to avoid overflow. visited only needs one bit, so lltrace could use the rest.
For consistency, make visited a uint8_t in the non-debug build as well.

markshannon
markshannon approved these changes Jan 21, 2025
View reviewed changes

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

Hide comment

Looks good.

@Fidget-Spinner Fidget-Spinner merged commit 5809b25 into python:main Jan 21, 2025
@Fidget-Spinner Fidget-Spinner deleted the move_lltrace_to_frame branch January 21, 2025 14:17
@chris-eibl

chris-eibl commented Jan 21, 2025

Copy link
Copy Markdown
Member

Sorry for being late here. These lines seem to be unused, since the PR did not have to touch them?

cpython/Python/ceval.c

Lines 1066 to 1071 in 05d12ee

#ifdef Py_DEBUG
#define DPRINTF(level, ...) \
if (lltrace >= (level)) { printf(__VA_ARGS__); }
#else
#define DPRINTF(level, ...)
#endif

@Fidget-Spinner

Fidget-Spinner commented Jan 21, 2025

Copy link
Copy Markdown
Member Author

@chris-eibl it seems so. If you would like to, could you please open a PR to either update to fix them or remove them? Thanks!

@chris-eibl

chris-eibl commented Jan 22, 2025

Copy link
Copy Markdown
Member

Yeah, but since this is gonna be my first PR, I'll have to work myself through the devguide. Most probably not before the weekend ...

@chris-eibl chris-eibl mentioned this pull request Jan 25, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannon markshannon markshannon approved these changes

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Fidget-Spinner @chris-eibl @markshannon