MAINT: refactor StringDType static_string implementation a bit. by mhvk · Pull Request #25890 · numpy/numpy

mhvk · 2024-02-26T18:49:23Z

@ngoldbaum - this is the refactor I mentioned. It started with wanting to use just 4 flags, and trying to make that a bit more logical. But it also solves a number of subtle bugs. In particular, in quite a few places, a memcpy of the empty string was done when the idea was to keep the arena information alive -- which of course is not possible if one overwrites the offset. I'll put a few more guiding comments in-line.

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.

mhvk

See inline comments - I hope you agree this is a bit easier to follow...

mhvk · 2024-02-26T18:51:18Z

-        // empty arena, nothing to do
-        return 0;
+    size_t size = VSTRING_SIZE(str);
+    if (size == 0 ||


I felt code should alert one if it is misused (and would probably have used assert myself)

added an assert

Shall I make the assert actually act on the expression? (well, the negated one)

Then, it is not used at all in optimized code, which seems OK.

I agree that seems better

mhvk · 2024-02-26T19:04:45Z

        _npy_static_string_u *out_u = (_npy_static_string_u *)out;
-        unsigned char flags = out_u->direct_buffer.size_and_flags &
-                              ~NPY_SHORT_STRING_SIZE_MASK;
        memcpy(out_u, &empty_string_u, sizeof(_npy_static_string_u));


This copy makes sense in setting the element to a zero-length short string, but after it, the element has lost the offset in the arena, so the flags now have to indicate that.

mhvk · 2024-02-26T19:05:45Z

    return ret;
 }

+NPY_NO_EXPORT int


To match location in .h file.

mhvk · 2024-02-26T19:06:43Z

                       const npy_packed_static_string *s2, npy_string_allocator *a2) {
-    if (a1 != a2 || is_not_a_vstring(s1) || is_not_a_vstring(s2)) {
+    if (a1 != a2 ||
+        is_short_string(s1) || is_short_string(s2) ||


I could see the case for reintroducing is_a_vstring here just to negate it. But with use in just a single function, it seemed fine to just write it out.

ngoldbaum · 2024-02-26T21:22:00Z

Thanks for taking a look at this! I think I'd like to combine this with the other PR just to make both of them easier to review together (and if I have time and motivation, update the NEP too so it can continue to serve as a reference). Do you mind if I push commits to your PR branch implementing that? I have some other stuff on the backburner so it might take a few days to get to.

mhvk · 2024-02-26T22:08:46Z

Yes, please just push to my PR! And for sure fine to do it together; the reason for #25882 was to separate the bugfix from the refactor, which came about mostly because my first refactor was giving segfaults, yet I knew I had encountered a real bug...

ngoldbaum · 2024-02-29T16:30:40Z

I haven't forgotten about this and am planning to work on it today so it can hopefully get included with the 2.0 RC. That said, I think we could put this in after the RC and it shouldn't have any user-visible consequences (besides bug fixes, of course).

ngoldbaum

Wow this is great. I have a couple clarifying questions below and I pushed a commit that adds a couple of assert(0) calls and cleans up / adds some more comments. I think this is ready to merge and I'd like to get it in the RC to make backports easier for chuck but maybe @seberg could weigh in too.

ngoldbaum · 2024-02-29T17:47:08Z

+#define NPY_STRING_INITIALIZED 0x40    // 0100 0000
+#define NPY_STRING_OUTSIDE_ARENA 0x20  // 0010 0000
+#define NPY_STRING_LONG 0x10           // 0001 0000
+#define NPY_STRING_FLAG_MASK 0xF0      // 1111 0000


Nice, I like that we leave the last word of the flags byte zeroed. That gives us space to write a fixed-width version of stringdtype that always uses SSO but with a user-specified width.

ngoldbaum · 2024-02-29T18:19:56Z

-        // empty arena, nothing to do
-        return 0;
+    size_t size = VSTRING_SIZE(str);
+    if (size == 0 ||


added an assert

ngoldbaum · 2024-02-29T18:39:52Z

-    unsigned char flags =
-            out_u->direct_buffer.size_and_flags & ~NPY_SHORT_STRING_SIZE_MASK;
-
-    if (size == 0) {


I think I initially added the clearing to make it easier for me to debug this. If we don't clear then fragments of old strings can survive in the arena buffer if we mutate an arena string to a smaller size and I was getting confused by that, it also seemed awkward to check for that case in newsize.

For now let's not clear the buffer and leave fragments of old strings behind since it's faster and simpler.

ngoldbaum · 2024-02-29T19:01:28Z

+        c = np.zeros_like(self.a)
+        assert_array_equal(self.get_flags(c), 0)
+        c[:] = self.a
+        assert_array_equal(self.get_flags(c), self.get_flags(self.a))


neat, in the old implementation this would have required heap allocations because we'd be enlarging strings in an array that has already been initialized but now that we have the initialized flag this "just works".

I was actually somewhat surprised it worked... But I agree it is good that it does.

Also describe general logic on top of file, and liberally add comments that hopefully will clarify the code.

mhvk · 2024-03-01T00:12:21Z

@ngoldbaum - I was perhaps unreasonably playing with making size = 0 not loose the arena, but while checking why it was not working noticed what I think is a small buglet: a copy of a string array uses the same arena as the original. Since the offsets are not shared, we do not notice this in the tests, but it still seems a bit odd. I guess it is because the dtype is not copied?

mhvk · 2024-03-01T00:55:14Z

OK, I couldn't resist: the last commit is the new bit that allows re-use of the arena after setting to an empty string (the rest is just what was there before, but with your commits squashed with mine). EDIT: I find it quite remarkable how little I had to change to make this work - the design is clearly good!!

If you agree the empty-string stuff is OK, then perhaps easiest to merge this, and deal with the shared allocators between copies separately (this is actually a bit bad, because the shared arena effectively means that memory does not get freed properly if you delete one of the copies).

p.s. I discovered that copies shared the allocator by looking at the offsets; for the copy, these were behind the strings in the original.

ngoldbaum · 2024-03-01T01:21:09Z

Good catch! We might need a hook in the copy code path to ensure copies get a unique descriptor.

mhvk · 2024-03-01T01:53:59Z

Good catch! We might need a hook in the copy code path to ensure copies get a unique descriptor.

FWIW, the following would add a test that a copy has a new allocator:

@@ -1275,6 +1275,9 @@ def test_copy(self):
         c = self.a.copy()
         assert_array_equal(self.get_flags(c), self.get_flags(self.a))
         assert_array_equal(c, self.a)
+        offsets = self.get_view(c)['offset']
+        assert offsets[2] == 1
+        assert offsets[3] == 1 + len(self.s_medium) + self.sizeofstr // 2

(it currently fails).

ngoldbaum · 2024-03-01T18:02:08Z

Let's merge this now despite the issue with copies, just in case the RC does happen this weekend. I'd like to make backports easier for Chuck.

mhvk · 2024-03-01T19:26:52Z

Thanks! On the copies, maybe @seberg has ideas about how to prevent a copy being simply assigned the same dtype as the original (and thus the same allocator for StringDType); of course, generally for dtype one would happily share the instance over multiple arrays...

seberg · 2024-03-04T08:59:01Z

On the copies, maybe @seberg has ideas about how to prevent a copy being simply assigned the same dtype as the original (and thus the same allocator for StringDType);

Isn't it just that void also needs the dtype finalization function which calls the finalization recursively on all fields? Or did my first glance go in the wrong direction?

ngoldbaum · 2024-03-05T17:03:36Z

Isn't it just that void also needs the dtype finalization function which calls the finalization recursively on all fields? Or did my first glance go in the wrong direction?

No, this has nothing to do with voids. Marten's doing a hack to even make the void view possible.

We probably just need a call to finalize_descr somewhere in the copy code path. Going to take a look at this today.

ngoldbaum · 2024-03-05T21:19:07Z

It ended up being something else but thankfully the fix was in stringdtype itself so no need to update the dtype API or fix a bug in numpy. See #25944.

mhvk added the component: numpy._core label Feb 26, 2024

github-actions Bot added the 03 - Maintenance label Feb 26, 2024

mhvk commented Feb 26, 2024

View reviewed changes

mhvk commented Feb 27, 2024

View reviewed changes

Comment thread numpy/_core/tests/test_stringdtype.py Resolved

ngoldbaum mentioned this pull request Feb 29, 2024

BUG: fix Stringdtype reinitialization after nan #25882

Closed

ngoldbaum approved these changes Feb 29, 2024

View reviewed changes

mhvk added 2 commits February 29, 2024 17:40

MAINT: refactor the flags for StringDType and simplify implementation.

fa1ca1f

Also describe general logic on top of file, and liberally add comments that hopefully will clarify the code.

TST: Check StringDType implementation, including arena

09e00b9

MAINT: allow reuse of arena after setting to empty string

2a26376

mhvk force-pushed the stringdtype-flags-refactor branch from df4fa96 to 2a26376 Compare March 1, 2024 00:50

ngoldbaum merged commit c3a32b5 into numpy:main Mar 1, 2024

mhvk deleted the stringdtype-flags-refactor branch March 1, 2024 19:23

ngoldbaum mentioned this pull request Mar 1, 2024

NEP: NEP 55 updates and add @mhvk as an author #25913

Merged

ngoldbaum mentioned this pull request Mar 5, 2024

BUG: avoid incorrect stringdtype allocator sharing from array copies #25944

Merged

Conversation

mhvk commented Feb 26, 2024

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngoldbaum commented Feb 26, 2024

Uh oh!

mhvk commented Feb 26, 2024

Uh oh!

Uh oh!

ngoldbaum commented Feb 29, 2024

Uh oh!

ngoldbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhvk commented Mar 1, 2024

Uh oh!

mhvk commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngoldbaum commented Mar 1, 2024

Uh oh!

mhvk commented Mar 1, 2024

Uh oh!

ngoldbaum commented Mar 1, 2024

Uh oh!

mhvk commented Mar 1, 2024

Uh oh!

seberg commented Mar 4, 2024

Uh oh!

ngoldbaum commented Mar 5, 2024

Uh oh!

ngoldbaum commented Mar 5, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mhvk commented Mar 1, 2024 •

edited

Loading