Get rid of frozen shapes #13289
Merged
Get rid of frozen shapes #13289
Conversation
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
❌ Tests Failed✖️no tests failed ✔️61985 tests passed(2 flakes) |
12422d9 to
b0cee81
Compare
casperisfine
commented
May 9, 2025
f6a90fb to
6e3eb3c
Compare
byroot
reviewed
May 13, 2025
This comment was marked as resolved.
This comment was marked as resolved.
fa04a3e to
e076cdd
Compare
byroot
reviewed
May 28, 2025
shape.h
Outdated
Comment on lines
19
to
20
| #define SHAPE_ID_NUM_BITS 16 | ||
| #define SHAPE_ID_OFFSET_NUM_BITS 15 |
There was a problem hiding this comment.
Unfortunately we still can't make shape_id_t 32bit long on 32bit platforms, because inlines caches (either ivar or CC for attr_reader methods) need to pack both an attr_index_t and a shape_id_t in a single uintptr_t.
So we'd need to either make these caches larger, or find some creative solution.
For the time being, I think we cal still use 1bit for frozen, as it still leaves 32k shapes on 32bits.
But if we want to eliminate other shape types like T_OBJECT, we'll have to find a solution for these caches.
There was a problem hiding this comment.
#13500 would unlock all 32 bits.
e076cdd to
46dce97
Compare
Instead `shape_id_t` higher bits contain flags, and the first one
tells whether the shape is frozen.
This has multiple benefits:
- Can check if a shape is frozen with a single bit check instead of
dereferencing a pointer.
- Guarantees it is always possible to transition to frozen.
- This allow reclaiming `FL_FREEZE` (not done yet).
The downside is you have to be careful to preserve these flags
when transitioning.
46dce97 to
cac5f1f
Compare
Freezing an object changes its `shape_id` This is necessary so that `setivar` routines can use the `shape_id` as a cache key and save on checking the frozen status every time. However for `getivar` routines, this causes needless cache misses. By clearing that bit we increase hit rate in codepaths that see both frozen and mutable objects.
cac5f1f to
29a2d85
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Instead
shape_id_thigher bits contain flags, and the first one tells whether the shape is frozen.This has multiple benefits:
dereferencing a pointer.
FL_FREEZE(TODO).shape_idongetivarso that frozen status doesn't invalidate inline caches.