Repository navigation
Handle 64-bit systems and large memory allocations - #10076
Conversation
|
Ohh, OK, I didn't check the issue but rather misread your comment in the description. |
|
I'm in the middle to wrap up the 4.0.1 release, so you can add the changelog entry once the release is out and the changelog is cleaned up (probably tomorrow). |
| } | ||
| #else | ||
| /* This is the general solution for arbitrary widths for Py_ssize_t but is much less | ||
| readable and possibly over-kill */ |
There was a problem hiding this comment.
Definitely harder to read but very appealing since we don't need to worry about (Py)_ssize_t increasing or not. I'll defer to C-ext experts.
|
|
||
| n--; | ||
|
|
||
| #if 1 |
There was a problem hiding this comment.
Note to maintainer: I think we need to pick one and remove this "if else".
| readable and possibly over-kill */ | ||
| const size_t numbits = sizeof(Py_ssize_t)*CHAR_BIT - 1;/* remove one for the sign-bit */ | ||
| size_t shift = 1, totbits = 0; | ||
| while(totbits < numbits) { |
There was a problem hiding this comment.
How about the following to replace the whole routine? (Also takes care of n <= 1)
size_t result = 1;
while (n > 1) {
result <<= 1;
n >>= 1;
}
return result;
There was a problem hiding this comment.
This is essentially what I used as the correct code in my testing routine. The biggest difference is that the (worst-case) number of steps in this code snippet scales as the number of bits, while the other two solutions have a constant complexity of O(log_2(nbits))
There was a problem hiding this comment.
Ah, I now see I really didn't look carefully enough at the original code! Suggestion withdrawn!
|
Why don't we wrap this up then?
Thank you! |
|
To grab latest changes from Unless you run into conflicts, the above commands should be all you need. You also need to force push afterwards. Make a backup branch first if in doubt. Thanks for working on this!
Technically, it is out of scope, right? This PR explicitly states that it addresses 64-bit problem, not negative value. So, I guess, unless you have strong opinion otherwise, to remove it. |
|
@manodeep - feel free to ping me on slack if you run into any issues with the rebase |
|
@pllim Made the updates the way you suggested - think that went without any fuss. Please let me know if I need to make any other changes. Thanks! |
|
@manodeep - sorry for the hassle, but could you rebase again, but this time also squash all the commits into one? I don't think we need to preserve the history here. Thanks! |
|
@mhvk Ohh not a problem at all - I was wondering that during the rebase. I totally agree - these individual commits do not add any value to the history. Let me give that a go |
|
@mhvk Updated with a rebase + squash. Seems like it went fine and everything got squashed into one commit. |
mhvk
added
the
zzz 💤 merge-when-ci-passes
|
Thanks @manodeep! And I'm looking forward for future contributions! |
Handle 64-bit systems and large memory allocations
Now can handle memory allocations greater than 2 GB. I have both a normal and a general solution - might be good to make a decision if the general solution is worth keeping around.
Description
This pull request is to address a memory allocation issue identified in #8946