FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-107265: Fix code_hash for ENTER_EXECUTOR case by corona10 · Pull Request #108188 · python/cpython · GitHub

/ cpython Public

gh-107265: Fix code_hash for ENTER_EXECUTOR case - #108188

Merged
gvanrossum merged 7 commits into
python:mainfrom
corona10:gh-107265-hash
Aug 21, 2023
Merged

gh-107265: Fix code_hash for ENTER_EXECUTOR case#108188
gvanrossum merged 7 commits into
python:mainfrom
corona10:gh-107265-hash

Conversation

corona10 commented Aug 21, 2023
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

corona10 commented Aug 21, 2023
edited
Loading

Copy link
Copy Markdown
Member Author

@gvanrossum cc @markshannon

I updated the code_richcompare not to modify the code object at all.

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@markshannon Can you please confirm that this addresses your concern? (I'm sorry it slipped by me when I reviewed the previous PR.)

Comment thread Objects/codeobject.c
Comment thread Objects/codeobject.c Outdated

Copy link
Copy Markdown
Member

Would it better to ignore co_code_adaptive, and use co_code for comparisons?
It will definitely be slower, but it will be correct.

Ultimately we will want to compare by identity, I think.

Copy link
Copy Markdown
Member

Ultimately we will want to compare by identity, I think.

I'm no so sure, I expect that would cause too much breakage.

Copy link
Copy Markdown
Member

Would it better to ignore co_code_adaptive, and use co_code for comparisons?

Or factor out the normalization code involved in constructing co_code so it can be reused by compare and hash.

Copy link
Copy Markdown
Member

Would it better to ignore co_code_adaptive, and use co_code for comparisons?

Or factor out the normalization code involved in constructing co_code so it can be reused by compare and hash.

Never mind, that's already factored out (deopt_code()) but it modifies the bytecode array in place.

Copy link
Copy Markdown
Member

We keep changing the hash and equality functions, so I don't really see how another change will break anything, apart from assumptions in the compiler.

corona10 commented Aug 21, 2023
edited
Loading

Copy link
Copy Markdown
Member Author

Would it better to ignore co_code_adaptive, and use co_code for comparisons?
It will definitely be slower, but it will be correct.

IIUC, we need to update _PyCode_CODE for comparisons or add a new macro. It's worth experimenting with it.
I would like to do it in separate PR including the performance overhead comparison.

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM.

Copy link
Copy Markdown
Member

We keep changing the hash and equality functions, so I don't really see how another change will break anything, apart from assumptions in the compiler.

It seems pretty fundamental that co == co.replace(), which comparing by identity would break (and we rely in many places on .replace() always creating a new code object, with no specializations or executors, and all caches reset). IMO any field that can be changed through code.replace(xxx=yyy) should be included in equality, and no others. The hash should use a pragmatic subset of these that satisfies the required relationship between hash and equality and can be computed quickly.

carljm commented Aug 21, 2023
edited
Loading

Copy link
Copy Markdown
Member

In #101346 I tried to change code objects to compare by identity, and in the process I reached the same conclusion as @gvanrossum.

Making co != co.replace() (or, similarly, compile(source_string, ...) != compile(source_string, ...)) is a much bigger change than any tweaks to the details of code object comparison that have happened up until now.

gvanrossum merged commit e6db23f into python:main Aug 21, 2023
corona10 deleted the gh-107265-hash branch August 21, 2023 23:29
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL