Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-95795: Move types.next_version_tag to PyInterpreterState #102343

Merged

Conversation

ericsnowcurrently
Copy link
Member

@ericsnowcurrently ericsnowcurrently commented Feb 28, 2023

Core static types will continue to use the global value. All other types will use the per-interpreter value. They all share the same range, where the global types use values < 2^16 and each interpreter uses values higher than that.

Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor note from a first readthrough. Honestly, I have no idea why this PR would cause an assertion failure in insertdict during an attribute load. Everything about that seems wrong.

Maybe try rerunning locally, but setting ENABLE_SPECIALIZATION = False in opcode.py and regenerating everything. That way we can try to figure out if specialization is indeed the culprit, or if this is maybe some freeze/deepfreeze weirdness.

the same __name__, for any __name__. Since that's a static property, it is
appropriate to declare fixed-size arrays for this. */
#define MAX_EQUIV 10
#define _Py_TYPE_BASE_VERSION_TAG (2<<16)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean (1<<16) here? This is currently the same as (1<<17)...

@ericsnowcurrently
Copy link
Member Author

FYI, this PR is blocked by code objects being shared for deep-frozen modules.

@ericsnowcurrently ericsnowcurrently enabled auto-merge (squash) April 24, 2023 21:58
@carljm
Copy link
Member

carljm commented Apr 24, 2023

It seems like this PR will effectively mean that there will now be only 2^16 type versions available to heap types? This seems too low, since we estimate (for instance) that Instagram likely has somewhere around 2^16 heap types, period, without even considering modifications to types. It seems preferable if we can allocate fewer bits to the static types (since there aren't that many) and more to the heap types.

@ericsnowcurrently ericsnowcurrently deleted the isolate-types-next-version-tag branch April 24, 2023 22:30
@carljm
Copy link
Member

carljm commented Apr 24, 2023

Ignore my above comment; I misunderstood how this PR works. There are still (2^32) - (2^16) types available to heap types, which is much more than plenty.

carljm added a commit to carljm/cpython that referenced this pull request Apr 24, 2023
* main:
  pythongh-103801: Tools/wasm linting and formatting (python#103796)
  pythongh-103673: Add missing ForkingUnixStreamServer and ForkingUnixDatagramServer socketservers (python#103674)
  pythongh-95795: Move types.next_version_tag to PyInterpreterState (pythongh-102343)
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.

4 participants