GeezerVison

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

GeezerVison is showing github.com — gh-128563: Move lltrace into the frame struct by Fidget-Spinner · Pull Request #129113 · python/cpython

Conversation

Fidget-Spinner commented Jan 21, 2025 •

edited by bedevere-app Bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

Issue: A new tail-calling interpreter for significantly better interpreter performance #128563

Sorry, something went wrong.

Uh oh!

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

All reactions


          Move lltrace into the frame struct

f79e7d1

Fidget-Spinner requested a review from markshannon as a code owner

January 21, 2025 01:34

bedevere-app Bot mentioned this pull request

Jan 21, 2025

A new tail-calling interpreter for significantly better interpreter performance #128563

Closed

bedevere-app Bot added the awaiting core review label

Jan 21, 2025

Fidget-Spinner added the skip news label

Jan 21, 2025

Fidget-Spinner added 2 commits

January 21, 2025 09:43


          fix free-threaded

da0d7ae


          Cleanup

989f841

Fidget-Spinner requested a review from ericsnowcurrently as a code owner

January 21, 2025 01:46

Fidget-Spinner added 6 commits

January 21, 2025 11:05


          Try fix on Windows


          Fix test_wasi

d3d1dbf


          fix for real

e062681


          8 -> 16

6ba4e59


          Revert "8 -> 16"

959ba82

This reverts commit 6ba4e59.


          reduce diff

134d2a0

markshannon requested changes

Jan 21, 2025

View reviewed changes

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

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Include/internal/pycore_frame.h Resolved

Uh oh!

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

Comment thread

Lib/test/test_sys.py Resolved

Uh oh!

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

Comment thread

Python/bytecodes.c Resolved

Uh oh!

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

Comment thread

Python/bytecodes.c Resolved

Uh oh!

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

bedevere-app Bot removed the awaiting core review label

Jan 21, 2025

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-app Bot added the awaiting changes label

Jan 21, 2025

Fidget-Spinner added 4 commits

January 21, 2025 18:28


          Address review

732c6b4


          Merge remote-tracking branch 'upstream/main' into move_lltrace_to_frame

c7bdb4f


          reduce diff

a28f2c5


          fix build

ef209db

Fidget-Spinner commented Jan 21, 2025

Copy link

Copy Markdown

Member Author

I have made the requested changes; please review again.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-app Bot added awaiting change review and removed awaiting changes labels

Jan 21, 2025

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-app Bot requested a review from markshannon

January 21, 2025 10:43

markshannon reviewed

Jan 21, 2025

View reviewed changes

Comment thread

Include/internal/pycore_frame.h Outdated

		char owner;
		#ifdef Py_DEBUG
		char visited:4;
		char lltrace:4;

markshannon Jan 21, 2025 •

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.

Why 4?
You only need one bit (although it is best to use uint8_t as the signedness of char is implementation defined.

My mistake, lltrace is not boolean.

Since char might be signed, it is best to make this a uint8_t to avoid overflow for PYTHON_LLTRACE=9

Sorry, something went wrong.

Uh oh!

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

All reactions

markshannon reviewed

Jan 21, 2025

View reviewed changes

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

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.

Sorry, something went wrong.

Uh oh!

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

All reactions


          Address review

3f51e08


          fix visited

f2cd968

markshannon approved these changes

Jan 21, 2025

View reviewed changes

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

Looks good.

Sorry, something went wrong.

Uh oh!

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

All reactions

bedevere-app Bot added awaiting merge and removed awaiting change review labels

Jan 21, 2025

Fidget-Spinner merged commit 5809b25 into python:main

Jan 21, 2025

bedevere-app Bot removed the awaiting merge label

Jan 21, 2025

Fidget-Spinner deleted the move_lltrace_to_frame branch

January 21, 2025 14:17

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

All reactions

Sorry, something went wrong.

Uh oh!

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

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

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

Fidget-Spinner 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.

chris-eibl mentioned this pull request

Jan 25, 2025

Remove unused DPRINTF in ceval.c #129282

Merged

Reviewers

markshannon

markshannon approved these changes

ericsnowcurrently

Awaiting requested review from ericsnowcurrently

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

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 01402520 — 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.

GeezerVison

Conversation

Fidget-Spinner commented Jan 21, 2025 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-app Bot commented Jan 21, 2025

Uh oh!

Fidget-Spinner commented Jan 21, 2025

Uh oh!

bedevere-app Bot commented Jan 21, 2025

Uh oh!

markshannon Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

chris-eibl commented Jan 21, 2025

Uh oh!

Fidget-Spinner commented Jan 21, 2025

Uh oh!

chris-eibl commented Jan 22, 2025

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fidget-Spinner commented Jan 21, 2025 •

edited by bedevere-app Bot

Loading

Uh oh!

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

markshannon Jan 21, 2025 •

edited

Loading

Uh oh!

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