Repository navigation
Conversation
The current code is meant to track when a string in the arena is freed for possible re-use later, but actually zeros the offset when an entry is set to undefined, making this impossible. Furthermore, it does not track when the offset is reused for either a short string or a string on the heap, after which the arena should not longer be an option. This commit fixes both problems, aiming to keep the changes minimally intrusive.
| // No room, resort to a heap allocation. We also set the short | ||
| // flag to indicate we can no longer use the heap (because the | ||
| // offset into the arena will get lost; TODO: should this be decided | ||
| // in newemptysize?) |
There was a problem hiding this comment.
Setting both NPY_STRING_ON_HEAP and NPY_STRING_SHORT is incorrect, they are mutually exclusive and should never be simultaneously set.
Would you mind terribly if I tried my hand at a fix for the issue you discovered? Since I have more familiarity with how this code "should" be working.
There was a problem hiding this comment.
Feel free to change, but note that I just followed what was on line 461...
Also, I've got a more complete rework in progress, partially as I wanted to avoid using the short size also for a medium-size flag. I'll try to push that up shortly...
|
Close/reopen |
|
@mattip the openblas problems are probably due to yesterday's wheel uploads. |
|
Closing this in favor of #25890 which fixes this bug along with other issues. |
I've been looking at trying to make the flags a bit more logical for
StringDType(see fourth item in #25693) and in the process noted that even though the code is meant to track when a string in the arena is freed for possible re-use later, it actually zeros the offset when an entry is set to undefined, making this impossible. Furthermore, it does not track when the offset is reused for either a short string or a string on the heap, after which the arena should not longer be an option. This PR fixes both problems, aiming to keep the changes minimally intrusive. (I still hope to have a follow-up PR that tries to make the flags a bit more logical).