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-105481: generate op IDs from bytecode.c instead of hard coding them in opcode.py by iritkatriel · Pull Request #107971 · python/cpython

Conversation

@iritkatriel

@iritkatriel iritkatriel commented Aug 15, 2023 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member
  • Issue: Generate opcode metadata from bytecodes.c instead of opcode.py #105481

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Fidget-Spinner reacted with rocket emoji
All reactions
  • 🚀 1 reaction
iritkatriel added 8 commits August 15, 2023 00:12
@iritkatriel
pythongh-105481: allocate op IDs in cases generator. Generate _PyOpco…
0811c6d 
…de_Deopt as part of metadata
@iritkatriel
_PyOpcode_OpName generated by cases_generator
a10a14d
@iritkatriel
remove include
eecc84c
@iritkatriel
replacement for opcode_targets.h
96bbc8f
@iritkatriel
generate opcode IDs from bytecodes.c
e68fc03
@iritkatriel
remove opcode IDs from opcode.py. Update tests and fix specialized op…
f04d3eb 
…codes map in dis.
@iritkatriel
fix magic number
e7833dd
@iritkatriel
rename function
99a408b
@iritkatriel
iritkatriel requested review from brettcannon, ericsnowcurrently, markshannon, ncoghlan and warsaw as code owners August 15, 2023 12:00
@bedevere-bot bedevere-bot added the awaiting core review label Aug 15, 2023
@bedevere-bot bedevere-bot mentioned this pull request Aug 15, 2023
Generate opcode metadata from bytecodes.c instead of opcode.py #105481
Closed
@iritkatriel
iritkatriel marked this pull request as draft August 15, 2023 12:01
@bedevere-bot bedevere-bot removed the awaiting core review label Aug 15, 2023
iritkatriel added 3 commits August 15, 2023 13:06
@iritkatriel
add news
7615eff
@iritkatriel
update windows makefile
2f67ea8
@iritkatriel
remove _specialized_instructions, we have the same in _specialized_op…
d6ecee1 
…map now
@Eclips4 Eclips4 added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Aug 15, 2023
ericsnowcurrently
ericsnowcurrently approved these changes Aug 15, 2023
View reviewed changes

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

Mostly LGTM. I've left a couple comments where there might be something further to be done.

(I'm approving under the assumption the comments will be resolved first, one way or another.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Include/opcode_ids.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/opcode.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting merge label Aug 15, 2023
@iritkatriel

iritkatriel commented Aug 15, 2023

Copy link
Copy Markdown
Member Author

Mostly LGTM. I've left a couple comments where there might be something further to be done.

Thanks. A couple of tests for tracing/instrumentation are failing, I'm still debugging that.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brettcannon
brettcannon removed their request for review August 15, 2023 21:57
iritkatriel and others added 5 commits August 16, 2023 16:30
@iritkatriel
pythongh-103082: remove assumption that INSTRUMENTED_LINE is the last…
7337a4b 
… instrumented opcode (python#107978)
@iritkatriel
assert 255 is not used
0d6ed4b
@iritkatriel
fix _co_firsttraceable calculation in deepfreeze.py
1d55078
@iritkatriel
update RESUME hard coded value (argh)
50415d5
@iritkatriel
Merge remote-tracking branch 'upstream/main' into deopt
5543e7c
@iritkatriel
iritkatriel force-pushed the deopt branch from 750a567 to 5543e7c Compare August 16, 2023 15:32
@iritkatriel
hard code RESUME. Make sure opmaps are sorted by ID
d39233e
@iritkatriel
iritkatriel marked this pull request as ready for review August 16, 2023 16:17
@iritkatriel
iritkatriel requested a review from a team as a code owner August 16, 2023 16:17
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting merge labels Aug 16, 2023
@iritkatriel
iritkatriel requested a review from gvanrossum August 16, 2023 16:42
gvanrossum
gvanrossum approved these changes Aug 16, 2023
View reviewed changes

@gvanrossum gvanrossum 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 revolution! It's almost here!!

A few nits but no need to wait for a re-review.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_dis.py
@@ -1617,197 +1616,197 @@ def _prepare_test_cases():
Instruction = dis.Instruction

expected_opinfo_outer = [

@gvanrossum gvanrossum Aug 16, 2023

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.

This test is just preposterous. Let's commit to doing this in a different way in a future PR.

Sorry, something went wrong.

Uh oh!

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

@iritkatriel iritkatriel Aug 16, 2023

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.

@markshannon loves to hate this test too.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Tools/build/deepfreeze.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Makefile.pre.in Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Tools/cases_generator/generate_cases.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Tools/cases_generator/generate_cases.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Aug 16, 2023
@iritkatriel
implemented suggestions from code review
f5e4c1d
gvanrossum
gvanrossum approved these changes Aug 16, 2023
View reviewed changes
@iritkatriel
iritkatriel enabled auto-merge (squash) August 16, 2023 22:13
@iritkatriel
iritkatriel merged commit 665a439 into python:main Aug 16, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Aug 16, 2023
@markshannon markshannon mentioned this pull request Sep 11, 2023
Changes to specialized instructions requires a magic number change. #109256
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericsnowcurrently ericsnowcurrently ericsnowcurrently approved these changes
@gvanrossum gvanrossum gvanrossum approved these changes
@ncoghlan ncoghlan Awaiting requested review from ncoghlan ncoghlan is a code owner
@warsaw warsaw Awaiting requested review from warsaw warsaw is a code owner
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

interpreter-core (Objects, Python, Grammar, and Parser dirs)

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.

5 participants

@iritkatriel @ericsnowcurrently @gvanrossum @bedevere-bot @Eclips4
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.