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

Visualize #50

Merged
merged 16 commits into from
May 21, 2018
Merged

Visualize #50

merged 16 commits into from
May 21, 2018

Conversation

thewtex
Copy link
Member

@thewtex thewtex commented May 15, 2018

Some initial benchmark results from the thread pool merge by @dzenanz following the JSON storage by @hjmjohnson and plotting by @jhlegarreta .

This is a plot for revisions, but in the future we could make historical and thread-count.

I will not be able to put further time into this for the remainder of the week.

plot from api 5 1

Find an initial version here.

thewtex and others added 14 commits May 13, 2018 17:28
Required for earlier version of ITK before itkBuildInformation.h was added.
Addresses:

  examples/Core/CMakeFiles/ThreadOverheadBenchmark.dir/ThreadOverhead.cxx.o:
  In function `main':
  ThreadOverhead.cxx:(.text.startup+0x79): undefined reference to
  `ReplaceOccurrence(std::__cxx11::basic_string<char, std::char_traits<char>,
  std::allocator<char> >, std::__cxx11::basic_string<char,
  std::char_traits<char>, std::allocator<char> > const&&,
  std::__cxx11::basic_string<char, std::char_traits<char>,
  std::allocator<char> > const&&)'
This is required for TBB
@thewtex thewtex requested a review from hjmjohnson May 15, 2018 03:53
@jhlegarreta
Copy link
Member

Wow, looking great. You rock. This provides real insight into ITK itself !

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.

The visualization looks great!

@@ -114,6 +114,7 @@ def build_itk(itk_src, itk_bin):
'-DCMAKE_CXX_STANDARD:STRING=11',
'-DBUILD_TESTING:BOOL=OFF',
'-DBUILD_EXAMPLES:BOOL=OFF',
'-DBUILD_SHARED_LIBS:BOOL=ON',
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is required for TBB. I have been using TBB without this most of the time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think that is correct. I added this in an attempt to address some odd CMake warnings, however, I think it is unrelated.

But, since this does not hurt, and it should improve link time for historical benchmark tests, I will leave it.

@blowekamp
Copy link
Member

This looks good.
I would like to generate something similar on my 88 ht-core system. How do I do that?

It is expected that the command line arguments are full
paths. Automatically convert the full paths.
@blowekamp
Copy link
Member

The script evaluate-itk-performance.py expects full paths. I create a patch to fix that:
thewtex#1

I have not getting ITK to work with TBB yet.

Here is what I was able to get on my system:

88cores-itkperformancebench

@thewtex
Copy link
Member Author

thewtex commented May 20, 2018

@blowekamp that is great! I am really interested to see how what the performance looks like with the 88-core system and TBB. Were you able to get the TBB build going? We should help them improve their TBBConfig.cmake generation -- I built from source, then I ran tbb/cmake/tbb_cmake_generator.cmake, but I needed to hack the resulting TBBConfig.cmake to use the built version.

@blowekamp
Copy link
Member

I was able to build and test itk with tbb but not use itk with tbb in another project. I made some notes on the confab about it.

I also made the patch:

http://review.source.kitware.com/#/c/23436/

@thewtex
Copy link
Member Author

thewtex commented May 20, 2018

xref uxlfoundation/oneTBB#6

@thewtex
Copy link
Member Author

thewtex commented May 20, 2018

@blowekamp It looks like @dzenanz set up the TBB-ITK CMake configuration well. However, SimpleITK should set ITK_DIR with CMake configure_file in SimpleITKConfig.cmake and call CMake's find_dependency.

@thewtex thewtex merged commit 8118140 into InsightSoftwareConsortium:master May 21, 2018
@thewtex thewtex deleted the visualize branch May 21, 2018 15:05
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.

4 participants