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: Remove SystemInformation data from ResourceProbe, fix issue #350 #351

Merged
merged 1 commit into from
Dec 26, 2018
Merged

PERF: Remove SystemInformation data from ResourceProbe, fix issue #350 #351

merged 1 commit into from
Dec 26, 2018

Conversation

N-Dekker
Copy link
Contributor

@N-Dekker N-Dekker commented Dec 24, 2018

ITK 4.9 added system info to itk::ResourceProbe (the base class of
itk::TimeProbe and itk::MemoryProbe), by calling GetSystemInformation()
in its constructor. This function call appeared very time consuming.
Using Visual Studio 2017 (Release configuration), it was observed that a single
GetSystemInformation() function call took more than 0.1 second.

This commit removes the this->GetSystemInformation() function call from
ResourceProbe, deprecates the member function, removes the related data
members, and only retrieves the system info when and where it is actually being
used: in PrintSystemInformation and PrintJSONSystemInformation.

Fixes issue #350, "Major performance issue TimeProbe, ResourceProbe
constructor", which was based on the analysis of an elastix example
case by Theo van Walsum (@tvanwalsum).

Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

This generally looks good to me. I don't know whether anyone used GetSystemInformation via this class instead of directly, but even if they did the remedy would not be hard. It should be documented in ITK5 MigrationGuide document.

Interestingly, Mac build fails with this incomprehensible message:

Undefined symbols for architecture x86_64:
   "___cxa_deleted_virtual", referenced from:
      vtable for itk::MemoryProbe in itkMemoryProbe.cxx.o
      vtable for itk::ResourceProbe<unsigned long, double> in itkMemoryProbe.cxx.o
      vtable for itk::TimeProbe in itkTimeProbe.cxx.o
      vtable for itk::ResourceProbe<double, double> in itkTimeProbe.cxx.o
ld: symbol(s) not found for architecture x86_64

The other build have not finished as of now.

@N-Dekker
Copy link
Contributor Author

N-Dekker commented Dec 24, 2018

@dzenanz Thanks! It looks like Mac doesn't like the way I "deleted" GetSystemInformation():

  virtual void GetSystemInformation() final = delete;

It could be a compiler bug, because I think my code is fine :-) However, I could also simply just remove the member function declaration. Would that be OK to you as well?

P.S. Note that GetSystemInformation() already was a protected member function of ResourceProbe, so it was probably never called outside ResourceProbe.

@dzenanz
Copy link
Member

dzenanz commented Dec 24, 2018

I think it would be better if you properly deprecated it.

@N-Dekker
Copy link
Contributor Author

N-Dekker commented Dec 24, 2018

I think it would be better if you properly deprecated it.

Thanks @dzenanz I'll make it a deprecated member function that does completely nothing. (It did not return anything anyway.)

PS I hope it will then eventually be removed with ITK 5.1. (It does not seem useful to keep a protected member function that just does nothing in there for very long.) OK?

ITK 4.9 added system info to `itk::ResourceProbe` (the base class of
`itk::TimeProbe` and `itk::MemoryProbe`), by calling `GetSystemInformation()`
in its constructor. This function call appeared very time consuming.
Using Visual Studio 2017 (Release configuration), it was observed that a single
`GetSystemInformation()` function call took more than 0.1 second.

This commit removes the `this->GetSystemInformation()` function call from
ResourceProbe, deprecates the member function, removes the related data
members, and only retrieves the system info when and where it is actually being
used: in `PrintSystemInformation` and `PrintJSONSystemInformation`.

Fixes issue #350, "Major performance issue TimeProbe, ResourceProbe
constructor", which was based on the analysis of an `elastix` example
case by Theo van Walsum (@tvanwalsum).
Copy link
Member

@hjmjohnson hjmjohnson left a comment

Choose a reason for hiding this comment

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

Thanks! This is a great cleanup.

@hjmjohnson hjmjohnson merged commit 747f3d1 into InsightSoftwareConsortium:master Dec 26, 2018
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Jan 27, 2021
Follow-up to:
"PERF: Remove SystemInformation data from ResourceProbe, fix issue InsightSoftwareConsortium#350"
Pull request InsightSoftwareConsortium#351
Commit 747f3d1, 2018-12-24
hjmjohnson pushed a commit to hjmjohnson/ITK that referenced this pull request Nov 7, 2021
Follow-up to:
"PERF: Remove SystemInformation data from ResourceProbe, fix issue InsightSoftwareConsortium#350"
Pull request InsightSoftwareConsortium#351
Commit 747f3d1, 2018-12-24
hjmjohnson pushed a commit to hjmjohnson/ITK that referenced this pull request Dec 13, 2021
Follow-up to:
"PERF: Remove SystemInformation data from ResourceProbe, fix issue InsightSoftwareConsortium#350"
Pull request InsightSoftwareConsortium#351
Commit 747f3d1, 2018-12-24
N-Dekker added a commit to N-Dekker/ITK that referenced this pull request Apr 28, 2023
Follow-up to:
"PERF: Remove SystemInformation data from ResourceProbe, fix issue InsightSoftwareConsortium#350"
Pull request InsightSoftwareConsortium#351
Commit 747f3d1, 2018-12-24
hjmjohnson pushed a commit that referenced this pull request Apr 30, 2023
Follow-up to:
"PERF: Remove SystemInformation data from ResourceProbe, fix issue #350"
Pull request #351
Commit 747f3d1, 2018-12-24
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

Successfully merging this pull request may close these issues.

3 participants