Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Field alignment problem in struct? #137

Open
szymonlopaciuk opened this issue Jul 11, 2024 · 3 comments
Open

Field alignment problem in struct? #137

szymonlopaciuk opened this issue Jul 11, 2024 · 3 comments

Comments

@szymonlopaciuk
Copy link
Contributor

I defined a Quadrupole element with some uint8 fields (see https://github.com/xsuite/xtrack/blob/e7ac347130b09ae1208061ba34b58e5e6d62f19f/xtrack/beam_elements/elements.py#L1194) and xtrack/tests/test_elements.py::test_constructors fails when dumping and reloading the xobject (on OpenCL).

From inspecting the buffers all the meaningful data seems to actually be in the right place (bytes at 144 and 152 are zero). However, it's not clear to me if this is by chance (and we have an alignment problem) or if it's simply the fact that the buffer is not being zeroed by default on OpenCL (then we have less of a problem but we cannot assume anymore that two identical xobjects are bitwise equal).

This is the exact difference and layout:

(Pdb) ee._xobject._buffer.buffer[ee._xobject._offset:ee._xobject._offset + ee._xobject._size].reshape([25, 8])
array([[   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,  -16,   63],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   5,    0,    0,    0,    0,    0,    0,    0],
       [  17,   17,   17,   17,   17,   17, -127,   63],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,  -66,  -66,  -66,  -66,  -66,  -66,  -66],
       [   0,  -66,  -66,  -66,  -66,  -66,  -66,  -66],
       [   0,    0,    0,    0,    0,   56, -113,  -64],
       [   0,    0,    0,    0,    0,   56, -113,  -64],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0]], dtype=int8)
(Pdb) nee._xobject._buffer.buffer[nee._xobject._offset:nee._xobject._offset + nee._xobject._size].reshape([25, 8])
array([[   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,  -16,   63],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   5,    0,    0,    0,    0,    0,    0,    0],
       [  17,   17,   17,   17,   17,   17, -127,   63],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,   17,   17,   17,   17,   17, -127,   63],
       [   0,    0,    0,    0,    0,   56, -113,  -64],
       [   0,    0,    0,    0,    0,   56, -113,  -64],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0],
       [   0,    0,    0,    0,    0,    0,    0,    0]], dtype=int8)
(Pdb) pp xt.Quadrupole._XoStruct._fields
[<field0 k1 at 0>,
 <field1 k1s at 8>,
 <field2 length at 16>,
 <field3 num_multipole_kicks at 24>,
 <field4 order at 32>,
 <field5 inv_factorial_order at 40>,
 <field6 knl at 48>,
 <field7 ksl at 96>,
 <field8 edge_entry_active at 144>,
 <field9 edge_exit_active at 152>,
 <field10 _sin_rot_s at 160>,
 <field11 _cos_rot_s at 168>,
 <field12 _shift_x at 176>,
 <field13 _shift_y at 184>,
 <field14 _shift_s at 192>]
@rdemaria
Copy link
Collaborator

In general we don't zero buffers. If a type leaves some bytes not declared, it should be expected that this portion of the data can contain random bytes. Where do we use bit wise identity?

@szymonlopaciuk
Copy link
Contributor Author

szymonlopaciuk commented Jul 12, 2024

I don't think we ever do it in production code, the error I saw happens in the test xtrack/tests/test_elements.py::test_constructors. I circumvented the problem by using a uint64, which we do anyway in many other places.

I mean you're right bitwise comparison is not the right way of checking the structs are the same if we allow them to be a non-contiguous type. However, in that test we assert the memory layout since we test json dumping, so we don't have much else to go of off.

Do you think zeroing the padding by default (on OpenCL, since the rest seem to be doing it already) would be a significant overhead?

@rdemaria
Copy link
Collaborator

Do you mean initialize the opencl buffer with zeros? I don't it will cost much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants