Make PeakList memory layout more robust
The following discussion from !25 (merged) should be addressed:
-
@andrewmichael.gozzard started a discussion: (+2 comments) This memory layout is hugely confusing, but I am pretty sure there is a bug in here. It appears
d_cmbchisq
is followed in memory byd_snglsnr_bg[0]
, but that meansd_snglsnr_bg[0]
should be at+ (3 * MAX_NIFO + 3) * max_npeak
, not+ (4 * MAX_NIFO) * max_npeak
as it seems to be. This seems to be because someone has assumed the coincidental 3 (from there being three variables of the same size,d_cohsnr
,d_nullsnr
, andd_cmbchisq
) to be an old hardcodedMAX_NIFO
, and hence has replaced it. This means that the gap betweend_cmbchisq
andd_snglsnr_bg[0]
is(MAX_NIFO - 3) * max_npeak
floats in size, which would cause collision and corruption ifMAX_NIFO < 3
, and allocate wasted memory ifMAX_NIFO > 3
.I have previously spoken to @timothyfrank.davies about alternative designs for this that construct cumulative offsets to make the code more readable and less error prone. I still believe we should implement that, and may begin work on it after this MR lands, but for now at least we should probably remove this gap.
Need to find a more readable way of doing this to stop it causing bugs (as it has multiple times already, it seems).
Plan is to just calculate cumulative offsets at runtime. Best plan so far is:
- Have an array of pointers to pointers to populate, and an array of their cumulative offsets
- Don't need dynamic arrays since upper bound on size is
sizeof(PeakList)/sizeof(void *)
- Don't need dynamic arrays since upper bound on size is
- Populate the arrays based on the size of each field
- Probably just lots of
pointers[i] = (void **) &d_peakpos; offsets[i] = offset; offset += max_npeak * sizeof(float); ++i;
- Probably just lots of
- Allocate as much memory as the final cumulative offset
- Iterate over the constructed arrays, assigning each pointer the corresponding offset from the allocated pointer
*pointers[i] = alloc + offset[i];
This should mean that we can update or insert a field and the subsequent fields will be updated automatically accordingly, without humans doing algebra by hand to update every field and introducing errors with any mistake.
We should consider doing an audit ahead of time to determine how much of PeakList shuold actually be in PeakList, at this point.