-
-
Notifications
You must be signed in to change notification settings - Fork 31.7k
redundant assignments to ob_size of new ints that _PyLong_New returned #71628
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
Comments
------------ current state ------------ The functions in which this happens are: With regard to relevant changes made in the past, it seems that the redundant assignment was added (in each of these six functions) on the last major rewriting of the function, or when the function was first added, and remained there to this day. The revisions in which the redundant assignments were added: ------------ proposed changes ------------ ------------ diff ------------ ------------ tests ------------ In addition, I ran 'python_d.exe -m test -j3' (on my 64-bit Windows 10) with and without the patches, and got quite the same output. |
Changes to PyLong_FromUnsignedLong() and PyLong_FromUnsignedLongLong() LGTM. I don't know whether other changes have a positive effect. Are there any microbenchmarks? There are other places in which Py_SIZE() is set to the same value. |
I am sorry, but I can't see why micro-benchmarking is needed here, as my patches only remove code that does nothing, while they don't add any new code. The assembly the compiler generates (on my PC) for 'Py_SIZE(v) = negative ? -ndigits : ndigits;' in PyLong_FromLongLong is: Am I missing anything that might cause my patches to introduce a performance penalty? I searched (all of the cpython repo) for other places in which Py_SIZE() is set to the same value, and indeed found one in Objects/longobject.c in _PyLong_Init: So I added a patch to move these assignments so they would be executed only in case the current element of small_ints wasn't already initialized. Have you spotted any other places in which Py_SIZE() is set to the same value? |
It's at least conceivable that code like Py_SIZE(v) = negative ? -ndigits : ndigits; might be compiled to something branchless on some platforms (with some sets of compiler flags). The assembly you show demonstrates that that doesn't happen on your machine, but that doesn't say anything about other current or future machines. I also prefer the original form for readability; so I agree with Serhiy that we shouldn't change it without evidence that the change improves performance. I'll remove the two obviously redundant |
New changeset 27a6ecf84f72 by Mark Dickinson in branch 'default': |
Changes to PyLong_FromUnsignedLong and PyLong_FromUnsignedLongLong applied. I've left the others; for the small int initialisation, that code isn't performance critical anyway, and I'm not entirely comfortable with assuming that PyObject_INIT_VAR will always handle negative sizes correctly. (The (ab)use of the sign bit of the ob_size field is something that's particular to the int type.) |
Misc/NEWS
so that it is managed by towncrier #552Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: