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

PERF: Avoid redundant searches for a data element in a gdcm::DataSet #4650

Conversation

N-Dekker
Copy link
Contributor

@N-Dekker N-Dekker commented May 8, 2024

The original code requested gdcm::DataSet to search for one and the same data element three times in a row, by calling both FindDataElement and GetDataElement for the very same tag. With this commit, the search is only performed once.

Issue found by Mihail Isakov (@issakomi):
#4636 (comment)


For the record, the redundant searches appear introduced with commit 168c457, June 5, 2014:

if(ds.FindDataElement(spacingTag) &&
!ds.GetDataElement(spacingTag).IsEmpty())
{
gdcm::DataElement de = ds.GetDataElement(spacingTag);

The original code requested `gdcm::DataSet` to search for one and the same data
element three times in a row, by calling both FindDataElement and GetDataElement
for the very same tag. With this commit, the search is only performed once.

Issue found by Mihail Isakov:
InsightSoftwareConsortium#4636 (comment)
@github-actions github-actions bot added type:Performance Improvement in terms of compilation or execution time area:IO Issues affecting the IO module labels May 8, 2024
@N-Dekker N-Dekker requested a review from issakomi May 8, 2024 12:20
@N-Dekker
Copy link
Contributor Author

N-Dekker commented May 8, 2024

@dzenanz, @issakomi Thanks for your approval! I think this PR can wait until after v5.4.0. right? I just submitted the PR, so that we don't forget about it!

@N-Dekker
Copy link
Contributor Author

N-Dekker commented May 8, 2024

/azp run ITK.Linux

@N-Dekker N-Dekker marked this pull request as ready for review May 21, 2024 08:07
@N-Dekker
Copy link
Contributor Author

I think this pull request may also be merged now, as v5.4.0 has been tagged (#4603 (comment))

@thewtex thewtex merged commit 2a28689 into InsightSoftwareConsortium:master May 21, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:IO Issues affecting the IO module type:Performance Improvement in terms of compilation or execution time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants