BUG: Fix copy performance regression for structured arrays - #29270

So CI fails because of this

dtype = np.dtype([('a', {'names': ['aa', 'ab'], 'formats': ['f', 'f'], 'offsets': [0, 4]}, (2, 3))])
sparse_dtype = np.dtype([('a', {'names': ['ab'], 'formats': ['f'], 'offsets': [4]}, (2, 3))])

def test_sparse_field_assignment(self):
          arr = np.zeros(3, self.dtype)
          sparse_arr = arr.view(self.sparse_dtype)
      
          sparse_arr[...] = np.finfo(np.float32).max
          # dtype is reduced when accessing the field, so shape is (3, 2, 3):
  >       assert_array_equal(arr["a"]["aa"], np.zeros((3, 2, 3)))
  E       AssertionError: 
  E       Arrays are not equal
  E       
  E       Mismatched elements: 15 / 18 (83.3%)
  E       Max absolute difference among violations: 1.36988107e-17
  E       Max relative difference among violations: inf
  E        ACTUAL: array([[[-1.369881e-17,  0.000000e+00,  2.802597e-45],
  E               [ 2.242078e-44,  1.121039e-44,  5.605194e-45]],
  E       ...
  E        DESIRED: array([[[0., 0., 0.],
  E               [0., 0., 0.]],
  E       ...

It seems to me that the shortcut I added is triggering here

sparse_arr[...] = np.finfo(np.float32).max

which causes the test to fail. In my mind, in this case we shouldn't be using the shortcut to copy - I would expect is_dtype_struct_simple_unaligned_layout(dst_dtype) to return false for sparse_arr, but it returns true and therefore the shortcut is applied. Maybe a maintainer could give an advice on how to address this? Thanks

Yeah, but it seems is_dtype_struct_simple_unaligned_layout isn't even used anywhere anymore.
The problem is nesting, this dtype has a nested structured dtype and fails to check that. We could probably fix that there. I do wonder if pushing the logic lower may make sense (unless the "is view" logic aligns, but I don't think it does :/).
(I suspect it doesn't, rather pushing it up may a bit but that is the flags idea.)

What do you mean by "pushing the logic lower"?

What do you mean by "pushing the logic lower"?

Sorry, nvm. you are already as low as it gets, I thought we might be able to include the logic in that follows, but it's effectively at the same place.
We should possibly move it because it avoids duplicating a lot of work two times probably, but it won't help with the underlying issue, unless PyArray_GetDTypeTransferFunction informs us that this is a trivial copy.

Signaling a trivially copyable would probably be the most useful thing to do, but it may require doing so in general in casts or maybe as a dtype flag (I am not 100% sure a dtype flag covers everything, but probably; The viewable flag might help, but I don't think it does unfortunately.).
So my guess would be the best way may be a new "trivially copyable" flag on the dtype instance (if dtypes are equivalent and trivially copyable, we can assume a memcpy is OK -- the opposite "not trivially copyable" may actually be more practical, because in practice it is just struct that has holes that could contain references even).

As a quicker fix that doesn't add a flag, I honestly don't have much of an idea except recursing or just giving up when you find an embedded structured dtype (and keep the assumption that all others without references are fine).
If it is good enough for you, I might lean towards ignoring embedded structures. It's just simpler/quicker, and if you have a case where the embedded structure is large that copy will use the optimization anyway.

(Although, I would be happy to investigate a new flag, since that seems useful in a lot more places to do similar optimizations.)

Ignoring embedded structures sounds good to me, I'll try this out. Agree a flag might be a better solution long-term :)

Thanks, could you see how annoying it is to fold the logic into the branch after wards? Otherwise PyArray_EquivTypes and the check, basically add 2x the same work that we'll be doing in that branch again.

(If it's annoying, I think I can accept not doing it. I hid the fields in NumPy 2, with the hope that we could have a better way to store this information that extract it from tuples every time again and again anyway...)

so I've played around with it and couldn't quite figure out how to access the "underlying" field type information. basically in the example from the test:

dtype = np.dtype([('a', {'names': ['aa', 'ab'], 'formats': ['f', 'f'], 'offsets': [0, 4]}, (2, 3))])
sparse_dtype = np.dtype([('a', {'names': ['ab'], 'formats': ['f'], 'offsets': [4]}, (2, 3))])

I could get the description of the field a as the one of type "void", and helpers like PyDataType_FIELDS didn't return anything when called on a (i. e. sparse_dtype['a'].fields is None / PyDataType_HASFIELDS(fld_type) == 0). So I'm not sure how to get the information that a has some subfields and what their properties (like offsets) are.

I could just check that the field dtype is not void, I'd go with that if there is no better way, but it seems to be too broad of a check

didn't return anything when called on a (i. e. sparse_dtype['a'].fields is None / PyDataType_HASFIELDS(fld_type) == 0). So I'm not sure how to get the information that a has some subfields and what their properties (like offsets) are.

That dtype is a subarray dtype, which should be OK, you could check the base I guess (of course, that could be another subarray dtype.

I dunno, my gut feeling is that if you don't take the trivial route here, adding the flag is probably actually the easier solution (and that could be a negative slag on a whim that such dtypes are rare).
It does require finding the 1-2 places where structured dtypes get created oddly and need to propagate flags, but that's about it probably.

I've updated the PR: added a NPY_NOT_TRIVIALLY_COPYABLE flag (negative flag as you suggested, since most dtypes are trivially copyable) and use it to enable memcpy fast path for structured arrays.

@seberg would you have a chance to take another look?

Hi @seberg, just wanted to check this is still on your radar. I've pushed changes addressing your review comments - let me know if you'd like anything else adjusted.

Thanks, I think this makes sense, but I need to bring it up once at least in the meeting (maybe ping the mailing list just in case).

My thoughts are along these lines:

• This might break things in principle, so we do need at least a release note (if you like you can write one).
• If we are even a bit worried about breaking people. We may want a way to set the flag explicitly as a work-around.
• I think on the up-side, this flag will be useful for other things (we should actually add a small static inline helper that checks for this one or REFCHECK, I think) -- useful for parts of code that have implementations using raw memcpy.
  (That is things like the fasttake function and advanced indexing.)

But overall, I am not really worried about breaking people. It is super weird to create a struct and rely on alignment to leave holes that we actually want to access.
So a brief release note would be nice, but otherwise it is probably just formality to ask a few others :).

If you are excited, adding a helper like PyDataType_TRIVIALLY_COPYABLE() would be nice (maybe even search for a few places to use it, but I understand that is a bit tough when you don't know the code-base).

EDIT: Oooops, sorry, you have the ISTRIVIALLYCOPYABLE already of course... Could/should be used in a few other places still. If you care, you can try the speed for advanced indexing with this dtype for example.

Regarding the trivially_copyable opt-out kwarg I'm not sure this is necessary in practice. This PR restores the original memcpy behavior for contiguous structured dtypes. So users would have had memcpy semantics (including padding bytes being copied) in the version before that regression was introduced. I wouldn't expect anyone to have started relying on padding byte preservation in the narrow window since then.

Happy to implement the opt-out if others feel it's needed though, something like a adding a trivially_copyable=False keyword argument to np.dtype() would be not too much to do.

So users would have had memcpy semantics (including padding bytes being copied) in the version before that regression was introduced.

Yeah, except that has been a bit too long that this was changed :). Either way, I agree, this is interesting, but probably only for cases we have covered (i.e. arr[["field1", "field3"]]).
So I think we should be OK (and you can still pass offsets= as a funny work around).

I am actually more interested in setting trivially_copyable=True, larger align=True alignments are the norm e.g. on GPUs and without it it would be a bit awkward to set the flag.

But, that is a good followup!

@seberg wanted to confirm the current state: you will bring this change up the next internal meeting due to it being a breaking change and get approval that this change can merge?

Are there any outstanding comments/changes you would like to be addressed in this PR? There are a couple of optional follow-ups, but nothing blocking this PR from merging, right?

Arg, thanks for the ping. I had forgotten to post what the conclusion was a bit:

• Nobody had a serious concern about this, I may still ping the mailing list, but overall I suspect we should try this.
• For the pickling problem what came up is that we could introduce a new version in unpickling but not start using it. That way when we start to use it would be less bumpy because it would still be possible to load it on e.g. versions 2.N-2, just not older.
  • I might suggest introducing a minor version in this new version, since a minor version would have been sufficient here (that could just be the NumPy version itself).

That said, I am not sure I want to force that on this PR.

One thing that actually would be good is to proof the big speedup by adding an asv benchmark.

Benchmark results:

| Change   | Before [xxxxxxxx] <main>   | After [xxxxxxxx] <bonpyt:structured-array-copy>            |   Ratio | Benchmark (Parameter)                                                                                        |
|----------|----------------------------|------------------------------------------------------------|---------|--------------------------------------------------------------------------------------------------------------|
| -        | 341±5μs                    | 104±2μs                                                    |    0.31 | bench_io.CopyStructured.time_copy(100, [('a', 'f8'), ('b', 'f8'), ('c', 'f8')])                              |
| -        | 352±8μs                    | 104±3μs                                                    |    0.3  | bench_io.CopyStructured.time_assign(100, [('a', 'f8'), ('b', 'f8'), ('c', 'f8')])                            |
| -        | 3.97±0.4ms                 | 1.15±0.09ms                                                |    0.29 | bench_io.CopyStructured.time_copy(1000, [('a', 'f8'), ('b', 'f8'), ('c', 'f8')])                             |
| -        | 4.07±0.3ms                 | 1.10±0.1ms                                                 |    0.27 | bench_io.CopyStructured.time_assign(1000, [('a', 'f8'), ('b', 'f8'), ('c', 'f8')])                           |
| -        | 32.8±0.4μs                 | 4.88±0.09μs                                                |    0.15 | bench_io.CopyStructured.time_assign(10, [('a', 'f8'), ('b', 'f8'), ('c', 'f8')])                             |
| -        | 33.3±0.4μs                 | 5.03±0.04μs                                                |    0.15 | bench_io.CopyStructured.time_copy(10, [('a', 'f8'), ('b', 'f8'), ('c', 'f8')])                               |
| -        | 593±10μs                   | 87.5±2μs                                                   |    0.15 | bench_io.CopyStructured.time_copy(100, [('x', 'i4'), ('y', 'i4'), ('z', 'i4'), ('w', 'i4'), ('v', 'i4')])    |
| -        | 587±10μs                   | 84.8±0.9μs                                                 |    0.14 | bench_io.CopyStructured.time_assign(100, [('x', 'i4'), ('y', 'i4'), ('z', 'i4'), ('w', 'i4'), ('v', 'i4')])  |
| -        | 6.55±0.5ms                 | 888±60μs                                                   |    0.14 | bench_io.CopyStructured.time_copy(1000, [('x', 'i4'), ('y', 'i4'), ('z', 'i4'), ('w', 'i4'), ('v', 'i4')])   |
| -        | 6.68±0.4ms                 | 894±70μs                                                   |    0.13 | bench_io.CopyStructured.time_assign(1000, [('x', 'i4'), ('y', 'i4'), ('z', 'i4'), ('w', 'i4'), ('v', 'i4')]) |
| -        | 59.4±1μs                   | 4.20±0.07μs                                                |    0.07 | bench_io.CopyStructured.time_assign(10, [('x', 'i4'), ('y', 'i4'), ('z', 'i4'), ('w', 'i4'), ('v', 'i4')])   |
| -        | 59.5±0.1μs                 | 4.40±0.06μs                                                |    0.07 | bench_io.CopyStructured.time_copy(10, [('x', 'i4'), ('y', 'i4'), ('z', 'i4'), ('w', 'i4'), ('v', 'i4')])     |


SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE INCREASED.

@seberg when you have a chance can you update the labels on the PR? Right now it has Needs Release Note which I've already added.

Do you think this will get into the next point release (2.4.3)?

Do you think this will get into the next point release (2.4.3)?

No, we don't backport such changes, 2.4.x is bug fixes only. It will be in 2.5 (mid of the year).

(we could backport, but I think there is a chance of regression?!)

Agree with not backporting this change, let's wait for the next release (2.5)

OK, I think it's time to just get this in, thanks @bonpyt. There are probably smaller follow-ups that would be nice, but none or vital enough to block this on.