Refactors to improve performance

[DrPaulSharp]

No description provided.

[StephenNneji]

Hey Paul, please this PR needs clarification. I can kind of see the attempt to improve memory by changing the array sizes on the input array but I am not sure where the new values come from and I think there is now a mismatch between the input values in makeCompileArgs and the result values you've set.

I don't fully understand the reasons behind the refactors to callReflectivity and if or how they improve performance

[DrPaulSharp]

Good point about the limit in makeCompileArgs - there's not much point limiting the values on the way out but not the way in! I'm hopeful that reducing the limit will improve the memory issues we have encountered, and it's probably best now to grasp the nettle and make sure we do it in all places. The code does build correctly with the reduced limit, but leave it with me and I'll see if we find any improvement in memory/performance as the limit is reduced.

[StephenNneji]

I don't think these bounds are needed. Theses values should just be Inf except where we actually have a bound, to the best of my knowledge the memory is allocated as needed so not sure the reason for the artificial handicap. @arwelHughes do you remember the original reason for the max size?

Is it possible that something like the data (problemCells{2} = inputStruct.data) can exceed our artificial limit of 10000 rows maybe someone doing a simulation or something?

[StephenNneji]

I think this is meant to be the other way around

coder.varsize('domainSldXDataCell',[2 1e4],[1 0]);

[DrPaulSharp]

Are you sure? I've written that the fixed dimension has size 2 since there are always two domains, and the variable size dimension has size up to 1e4: https://uk.mathworks.com/help/coder/ref/coder.varsize.html

[StephenNneji]

You are right, I was looking at the size of sldXData.

Please have a look at refPercentileConfidenceIntervals it does not have separate dimensions for the domains. is this a bug?

RAT/minimisers/generalUtils/bayesStats/refPercentileConfidenceIntervals.m

Line 35 in 6c6c1c5

coder.varsize('rowVals',[1 1e4],[0 1]);

[arwelHughes]

I think the memory allocation ‘under the hood’ is different. To quote from the TMW site…. · “If you do not specify upper bounds with a coder.varsize declaration and the code generator is unable to determine the upper bounds, the generated code uses dynamic memory allocation. Dynamic memory allocation can reduce the speed of generated code. To avoid dynamic memory allocation, specify the upper bounds by providing the ubounds<https://uk.mathworks.com/help/coder/ref/coder.varsize.html#mw_f8911ef0-febf-4349-b494-360c5163671e> argument. ” Although I don’t know the exact details, a fixed upper bound always allocates in stack, using Inf for the upper bound enforces DMA, so that is in heap, which is apparently slower. My understanding is that an Inf upper bound has some performance hit, regardless of the eventual size of the array… (e.g. see https://uk.mathworks.com/matlabcentral/answers/500451-why-do-i-always-need-coder-varsize-directives ) From: StephenNneji ***@***.***> Date: Tuesday, 20 August 2024 at 12:42 To: RascalSoftware/RAT ***@***.***> Cc: Hughes, Arwel (STFC,RAL,ISIS) ***@***.***>, Mention ***@***.***> Subject: Re: [RascalSoftware/RAT] Refactors to improve performance (PR #257) @StephenNneji commented on this pull request.

________________________________ In compile/fullCompile/makeCompileArgsFull.m<#257 (comment)>:

maxArraySize = 10000;

+maxDataSize = 10000; I don't think these bounds are needed. Theses values should just be Inf except where we actually have a bound, to the best of my knowledge the memory is allocated as needed so not sure the reason for the artificial handicap. @arwelHughes<https://github.com/arwelHughes> do you remember the original reason for the max size? Is it possible that something like the data (problemCells{2} = inputStruct.data) can exceed our artificial limit of 10000 rows maybe someone doing a simulation or something?

________________________________ In minimisers/generalUtils/makeEmptyBayesResultsStruct.m<#257 (comment)>:

for i = 1:nContrasts

reflectivityXData{i} = xDataCell; end if isDomains sldXData = cell(nContrasts,2); - sldXDataCell = [1 1 1; 1 1 1]; - coder.varsize('sldXDataCell',[2 1e4],[1 1]); + domainSldXDataCell = [1 1 1; 1 1 1]; + coder.varsize('domainSldXDataCell',[2 1e4],[0 1]); I think this is meant to be the other way around coder.varsize('domainSldXDataCell',[2 1e4],[1 0]); — Reply to this email directly, view it on GitHub<#257 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGMNVXVO6USMZY27P6JLOY3ZSMTQHAVCNFSM6AAAAABMOBS4UOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDENBXGY3TSMBUHE>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[StephenNneji]

@arwelHughes The link you shared says

Whenever the upper bounds are fixed variables can be still allocated in stack, so the dynamic memory alloaction may not be required (not always true !) .

Stack allocation is not guaranteed which makes sense as the stack can be as small as 1MB on some machines. From my look at the generated code we are already using the heap a lot even when we set an upper bound, it seems as longer as both dimension are not fixed it used the heap array so for example

coder.varsize('outlierChains',[1e3 1e3],[1 1]); used a bounded array on stack because both dimensions are fixed whereas coder.varsize('percentile65',[2 1e3],[0 1]) uses heap memory so we are not getting any benefit from setting the upper bound on variable sized arrays

[StephenNneji]

Should this match the other fitParams that is now a row array?

[DrPaulSharp]

Seeing as it's a simple change, yes!