Repository navigation
BUG: Significant copy performance regression for structured arrays #28194
Description
Describe the issue:
We have observed a performance regression when copying a large structured array.
Before:
START
0.3912498950958252
0.12642884254455566
0.1266167163848877
0.12655949592590332
0.1264510154724121
After:
START
2.4238221645355225
2.457045316696167
2.1723172664642334
2.454754114151001
2.4571428298950195
We have tracked it down to this MR: #18398
The previous code had a special case for copying packed structured arrays (dtype_transfer.c) leading to the whole array getting copied as one.
If we copy the array as one, the itemsize and stride are the same, therefore Numpy can dispatch to a contiguous copy rather than a strided copy here: lowlevel_strided_loops.c.src)
What happens after the MR is that we copy the fields individually, but the data for a single field is not contiguous in memory so we have to do a strided copy.
We managed to fix it by putting the special case into the void_to_void_get_loop, but it would be great to get this fixed upstream and you might have a nicer way of getting the performance back:
diff --git a/numpy/_core/src/multiarray/convert_datatype.c b/numpy/_core/src/multiarray/convert_datatype.c
index a0f09b71a3..39967be732 100644
--- a/numpy/_core/src/multiarray/convert_datatype.c
+++ b/numpy/_core/src/multiarray/convert_datatype.c
@@ -3212,7 +3212,23 @@ void_to_void_get_loop(
{
if (PyDataType_NAMES(context->descriptors[0]) != NULL ||
PyDataType_NAMES(context->descriptors[1]) != NULL) {
- if (get_fields_transfer_function(
+
+ int is_builtin = context->descriptors[0]->type_num < NPY_NTYPES_LEGACY && context->descriptors[1]->type_num < NPY_NTYPES_LEGACY;
+
+ if (PyArray_EquivTypes(context->descriptors[0], context->descriptors[1]) &&
+ !PyDataType_REFCHK(context->descriptors[0]) && !PyDataType_REFCHK(context->descriptors[1]) &&
+ ( !PyDataType_HASFIELDS(context->descriptors[1]) ||
+ is_dtype_struct_simple_unaligned_layout(context->descriptors[1])) && is_builtin )
+ {
+ *out_loop = PyArray_GetStridedCopyFn(0,
+ strides[0], strides[1],
+ context->descriptors[0]->elsize);
+ if (out_loop == NULL){
+ return -1;
+ }
+ *out_transferdata = NULL;
+ }
+ else if (get_fields_transfer_function(
aligned, strides[0], strides[1],
context->descriptors[0], context->descriptors[1],
move_references, out_loop, out_transferdata,
Reproduce the code example:
import sys
import numpy as np
import time
dtype = np.dtype([
('a', np.float32),
('b', np.float32),
('c', np.float32),
('d', np.float32),
('e', np.float32),
('f', np.float32),
('g', np.float32),
('h', np.float32),
('i', np.float32),
])
SHAPE = (100000000,)
def test(dtype, shape):
x = np.zeros(shape=SHAPE, dtype=dtype)
y = np.zeros(shape=SHAPE, dtype=dtype)
def f():
x[:] = y[:]
for _ in range(5):
start = time.time()
f()
end = time.time()
print(end-start, flush=True)
test(dtype, SHAPE)Error message:
Python and NumPy Versions:
Regression between 1.20.3 and 1.21 (but still happens on latest master).
Runtime Environment:
No response
Context for the issue:
No response
Activity
seberg commentedon Jan 20, 2025
Hmmm, I don't remember if there may have been an intentional angle to this or whether it was just unintentional when re-organizing the cast setup.
The place where you put it seems about right and if you are up for it a PR would be very welcome. There are two things I think that would be good to look into on first thought:
get_fields_transfer_functionthat function needs all the offsets, etc. anyway, so it would avoid duplicating work, I suspect (and be simpler if we consider that we can delete that helper).dtypeincludes a nested structured dtype that does not have a packed layout.And if that is a problem, my first thought for a good solution would be if we could query a dtype on whether it is "trivially assignable/copyable". But that might require a new flag? And I am not sure if the propagation/setting logic is obvious.
(If it is a gigantic difference one could think about special casing neested structured dtypes here.)
bonpyt commentedon Jan 20, 2025
I'm happy to have a go at a PR.
Regarding your points:
seberg commentedon Mar 14, 2025
Just saw this and thought that a PR is still nice. The logic here is actually correct it seems to me (i.e. the code would only trigger for packed structs, although we still should move it into
get_fields_transfer_function, I think. Just to avoid doing work twice).