Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.4k
gh-84436: Add static flag in PyLongObject's lv_tag#103403
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
eduardo-elizondo commentedApr 10, 2023
@markshannon here's a couple of changes to the recently introduced lv_flag so that we can use it in:#19474 |
markshannon commentedApr 11, 2023
Why do we need a static bit separate from the immortal bit? |
markshannon commentedApr 11, 2023
Also, all the checks for sign and compactness need to be as close to single machine instructions as possible. |
eduardo-elizondo commentedApr 12, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The specific place the static check is needed is here:https://github.com/python/cpython/pull/19474/files#r1156466281. It's to be compliant with the added condition of PEP683 to be able to reset the reference count if an immortal object's refcount reaches zero. For types like None, Bool, this is easy, we just add a new tp_dealloc that resets the value. For interned strings, we check the intern state. For small ints we can either check if it's a small int or use a flag somewhere in the object, like this one. In my opinion, it's much better to just do the 'expensive' small int check in tp_dealloc of the longobject but if we are already making a bit here available for "immortal longs" we might as well use it. But, I'm open to either option.
Assuming that we do want to have the static check, I think we might be able to make this cheaper if we make the immortal bit to be the least significant bit and the sign mask be the second and third least significant bits. I can draft up a change like this. However, before I do that, I'd like to get some thoughts in perhaps dropping the immortal bit in long's |
markshannon commentedApr 25, 2023
I think you misunderstood my (poorly worded) question. Why do we need a static flagin addition to an immortal flag? I worry that having two bits to check will degrade performance too much. |
markshannon commentedJun 2, 2023
eduardo-elizondo commentedJun 19, 2023
@markshannon Somehow I missed this message, my bad.
Short answer is: we don't need it! We just need one flag. Having two flags will make things cleaner in code, but harder in reasoning. I tried to explain that above but probably didn't come across correctly. Let me explain a bit more. The original reason I created this change was to address one ofGuido's comment in the Immortal Objects PR. The comment was made on the newly addedLongObject's tp_dealloc function. For context, the tp_dealloc was added so that we could restore long objects that would be decref'd to zero due to extension incompatibility. Given that we can't rely on the reference count, the way that we can check if this value has to be restored is by checking if the object is statically allocated which we assume by checking if it's a small int. This leads a rather long check: To this, Guido asked:
Thus, I created this PR to be able to use this immortal bit. All this PR does is that it exposes: Going back to your original question:
If we have the static flag we can make things a lot cleaner but it's not needed, we can keep the longer checks and we can still guarantee correctness. |
eduardo-elizondo commentedJun 19, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@markshannon Given all of the above, I instead propose to completely drop the "immortality bit" in the long value tag bits since we don't need this signal. |
encukou commentedSep 13, 2024
It looks like this PR can be closed, is that right? |
eduardo-elizondo commentedSep 14, 2024
@encukou yeah, should be safe to close |
Uh oh!
There was an error while loading.Please reload this page.
This adds a new
SIGN_STATICflag to PyLongObject'slv_tagwhich is used to determine if a given long object was statically allocated. It also reworks all the functions that useNON_SIZE_BITSto correctly account for this new flag.