Skip to content

ctpdev: checking input size instead of output size#12881

Merged
shahor02 merged 5 commits intoAliceO2Group:devfrom
lietava:ctpdev
Mar 20, 2024
Merged

ctpdev: checking input size instead of output size#12881
shahor02 merged 5 commits intoAliceO2Group:devfrom
lietava:ctpdev

Conversation

@lietava
Copy link
Contributor

@lietava lietava commented Mar 18, 2024

No description provided.

@github-actions
Copy link
Contributor

REQUEST FOR PRODUCTION RELEASES:
To request your PR to be included in production software, please add the corresponding labels called "async-" to your PR. Add the labels directly (if you have the permissions) or add a comment of the form (note that labels are separated by a ",")

+async-label <label1>, <label2>, !<label3> ...

This will add <label1> and <label2> and removes <label3>.

The following labels are available
async-2023-pbpb-apass
async-2023-pp-apass1
async-2022-pp-apass6
async-2022-pp-apass4
async-mc
async-data

@lietava
Copy link
Contributor Author

lietava commented Mar 19, 2024

HI @shahor02 , doing now input size check instead of output. But I am not sure how to do it. Please, have a look.

@lietava lietava marked this pull request as ready for review March 20, 2024 09:26
@lietava lietava requested a review from a team as a code owner March 20, 2024 09:26
Copy link
Collaborator

@aferrero2707 aferrero2707 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, apart from the suggested use of uint64_t for the variables.

bool mDoDigits = true;
o2::pmr::vector<CTPDigit> mOutputDigits;
int mMaxOutputSize = 0;
int mMaxInputSize = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use uint64_t to allow for large values.

mOutputLumiInfo.inp2 = inp2;
mMaxOutputSize = ctx.options().get<int>("max-output-size");
LOG(info) << "CTP reco init done. Inputs decoding here:" << decodeinps << " DoLumi:" << mDoLumi << " DoDigits:" << mDoDigits << " NTF:" << mNTFToIntegrate << " Lumi inputs:" << lumiinp1 << ":" << inp1 << " " << lumiinp2 << ":" << inp2 << " Max errors:" << maxerrors << " Max output size:" << mMaxOutputSize;
mMaxInputSize = ctx.options().get<int>("max-input-size");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace int by uint64_t

std::vector<InputSpec> filter{InputSpec{"filter", ConcreteDataTypeMatcher{"CTP", "RAWDATA"}, Lifetime::Timeframe}};
{
size_t payloadSize = 0;
for (const auto& ref : o2::framework::InputRecordWalker(inputs, filter)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here, use uint64_t instead of size_t, to protect from large values.

const auto dh = o2::framework::DataRefUtils::getHeader<o2::header::DataHeader*>(ref);
payloadSize += o2::framework::DataRefUtils::getPayloadSize(ref);
}
if ((mMaxInputSize > 0) && (payloadSize > (size_t)mMaxInputSize)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to cast to size_t if they are both defined as uint64_t

@lietava
Copy link
Contributor Author

lietava commented Mar 20, 2024

@aferrero2707 : as far as I can see your comment contradict @shahor02 comments - see resolved. So I leave it to you to discuss with @shahor02 .

@shahor02
Copy link
Collaborator

I think it is already safe: the max allowed size (mMaxInputSize) cannot be more than 2 GB/TF (700 GB/s) and with size_t for payloadSize we are safe no not wrap to 0 in the accumulation whatever is the real payload size.

@aferrero2707
Copy link
Collaborator

@lietava @shahor02 then for me it is fine, I have no strong objections. I admit I have not done the math, just went for an extra-safe suggestion.

@lietava
Copy link
Contributor Author

lietava commented Mar 20, 2024

@aferrero2707 @shahor02 Thanks.

@lietava
Copy link
Contributor Author

lietava commented Mar 20, 2024

Please, merge.

@shahor02 shahor02 merged commit 3cddbf8 into AliceO2Group:dev Mar 20, 2024
andreasmolander pushed a commit to andreasmolander/AliceO2 that referenced this pull request Apr 12, 2024
* dev: checking max input size

* clang

* dev: input size check

* clang

* fix: size_t
andreasmolander pushed a commit to andreasmolander/AliceO2 that referenced this pull request Apr 12, 2024
* dev: checking max input size

* clang

* dev: input size check

* clang

* fix: size_t
mwinn2 pushed a commit to mwinn2/AliceO2 that referenced this pull request Apr 25, 2024
* dev: checking max input size

* clang

* dev: input size check

* clang

* fix: size_t
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants