Improve benchmark for distributed tree #78

masterleinad · 2019-06-11T18:45:35Z

Changes:

Properly align output,
Only print information on MPI rank 0,
Print relevant run parameters.
Replace overlap by shift such that values different from one and zero have a more intuitive meaning.

masterleinad · 2019-06-11T18:45:54Z

CMakeLists.txt

@@ -13,7 +13,7 @@ if(ArborX_ENABLE_MPI)
  find_package(MPI REQUIRED)
  target_link_libraries(ArborX	INTERFACE MPI::MPI_CXX)
 endif()
-#target_compile_features(ArborX INTERFACE cxx_std_14)


What do we want here?

I see, we are running into our old friend

CMake Error in test/CMakeLists.txt: No known features for CXX compiler "GNU" version 7.4.0.

aprokop · 2019-06-11T18:54:05Z

@masterleinad Can you please post the output before and after?

masterleinad · 2019-06-11T18:59:48Z

before:

$ mpiexec -np 2 ./ArborX_DistributedTree.exe
ArborX version: 0.9 (dev)
ArborX hash   : 0b73981
ArborX version: 0.9 (dev)
ArborX hash   : 0b73981
contruction done
knn done
radius done
========================================

TimeMonitor results over 2 processors
Timer Name	MinOverProcs	MeanOverProcs	MaxOverProcs
----------------------------------------
construction	0.0406743	0.0406757	0.0406771
knn	1.03524	1.03534	1.03543
radius	0.57317	0.573342	0.573514
========================================

after:

$ mpiexec -np 2 ./ArborX_DistributedTree.exe
ArborX version: 0.9 (dev)
ArborX hash   : 0b73981

Running with arguments:
perform knn search      : true
perform radius search   : true
#points/MPI process     : 50000
#queries/MPI process    : 20000
size of shift           : 1
dimension               : 3

contruction done
knn done
radius done
==========================================================

TimeMonitor results over 2 processors
Timer Name   | MinOverProcs | MeanOverProcs | MaxOverProcs
----------------------------------------------------------
construction | 4.230867e-02 |  4.230957e-02 | 4.231046e-02
knn          | 1.042842e+00 |  1.043093e+00 | 1.043345e+00
radius       | 5.693861e-01 |  5.695536e-01 | 5.697212e-01
==========================================================

dalg24

It is fine to use C++14. You just cannot use the target_compile_feature() for now because of nvcc_wrapper. The build arguments passed to the Kokkos generate_makefile.bash script will need to be adjusted in the .jenkins file.

dalg24 · 2019-06-11T19:30:59Z

examples/distributed_tree_driver/distributed_tree_driver.cpp

+    // Initialize with length of "Timer Name"
+    std::size_t max_section_length = 10;
+
+    for (unsigned int i = 0; i < n_timers; ++i)


warning comparison between signed and unsigned

examples/distributed_tree_driver/distributed_tree_driver.cpp

dalg24 · 2019-06-11T19:45:57Z

examples/distributed_tree_driver/distributed_tree_driver.cpp

+    {
+      std::rotate(kokkos_argv, help_it + 1, kokkos_argv + kokkos_argc);
+      --kokkos_argc;
+    }


Why did you decide to rotate rather than swapping with the last argument?

Swapping now.

I just wasted a bunch of time checking that swap(x, x) was safe (will happen if you only have the help flag passed has argument) :/

Anyway that's fine

dalg24 · 2019-06-11T19:49:03Z

examples/distributed_tree_driver/distributed_tree_driver.cpp

@@ -110,10 +122,11 @@ class TimeMonitor
                  n_timers, MPI_DOUBLE, comm);
    if (comm_rank == 0)
    {
-      os << "========================================\n\n";
+      os << std::string(max_section_length + 46, '=') << "\n\n";


Where does the 46 come from?

dalg24 · 2019-06-11T19:49:24Z

examples/distributed_tree_driver/distributed_tree_driver.cpp

    if (comm_size == 1)
    {
-      os << "========================================\n\n";
+      os << std::string(max_section_length + 15, '=') << "\n\n";


Where does the 15 come from?

dalg24 · 2019-06-11T20:02:17Z

Also the name of the PR is misleading. Please, at the very least, edit the description and make a list or an enumeration to emphasize the changes for controlling the overlap between regions.

dalg24 · 2019-06-11T20:04:28Z

BTW #78 (comment) demonstrates something interesting I had already ran into. The hash returned by the runtime is garbage if you do not reconfigure.

Rombur · 2019-06-12T12:24:56Z

examples/distributed_tree_driver/distributed_tree_driver.cpp

+
+    // Initialize with length of "Timer Name"
+    std::size_t const max_section_length = std::accumulate(
+        _data.begin(), _data.end(), std::size_t(10),


The length of "Timer name".

Update comment to avoid question. Say "The initial value of the sum (3rd argument) correspond to the length of ..."

Or define a variable that holds the label and get its size.

Rombur · 2019-06-12T12:25:11Z

examples/distributed_tree_driver/distributed_tree_driver.cpp

+      std::string const header_without_timer_name = " | GlobalTime";
+      int const header_width =
+          max_section_length + std::max<int>(header_without_timer_name.size(),
+                                             std::cout.precision() + 9);


The width we need to print a floating point number in scientific format apart from the precision assuming the exponent needs two characters.

Add comment or update code to make it obvious

dalg24 · 2019-06-12T15:44:30Z

examples/distributed_tree_driver/distributed_tree_driver.cpp

@@ -30,7 +31,8 @@ struct HelpPrinted
 {
 };

-// Poor man's replacement for Teuchos::TimeMonitor
+// The TimeMonitor class can be used to measure for a series of events, i.e. it
+// represents a set of timers of type Timer.


FWIW I think the original comment was better because it gives context on the design of TimeMonitor

dalg24 · 2019-06-12T15:51:01Z

examples/distributed_tree_driver/distributed_tree_driver.cpp

+
+    // Initialize with length of "Timer Name"
+    std::size_t const max_section_length = std::accumulate(
+        _data.begin(), _data.end(), std::size_t(10),


Update comment to avoid question. Say "The initial value of the sum (3rd argument) correspond to the length of ..."

Or define a variable that holds the label and get its size.

dalg24 · 2019-06-12T15:52:07Z

examples/distributed_tree_driver/distributed_tree_driver.cpp

+      std::string const header_without_timer_name = " | GlobalTime";
+      int const header_width =
+          max_section_length + std::max<int>(header_without_timer_name.size(),
+                                             std::cout.precision() + 9);


Add comment or update code to make it obvious

examples/distributed_tree_driver/distributed_tree_driver.cpp

dalg24 · 2019-06-18T22:46:37Z

examples/distributed_tree_driver/distributed_tree_driver.cpp

+
+    // Initialize with length of "Timer Name"
+    std::size_t const max_section_length = std::accumulate(
+        _data.begin(), _data.end(), std::string("Timer Name").size(),


(Minor comment) I would have defined a variable for the label and reuse below.

dalg24 · 2019-06-18T22:49:06Z

Merge when you are ready. You may chose to ignore my last comment. I would be fine with you adding the Arguments class you introduced in #83 before we merge.

masterleinad · 2019-06-19T12:50:30Z

Since you are requesting changes to the Arguments struct already, let us deal with it in the other pull request.

masterleinad commented Jun 11, 2019

View reviewed changes

dalg24 reviewed Jun 11, 2019

View reviewed changes

masterleinad changed the title ~~Beautify ArborX_DistributedTree output~~ Improve ArborX_DistributedTree Jun 11, 2019

masterleinad mentioned this pull request Jun 12, 2019

Make sure the recorded git hash is recent #79

Merged

Rombur reviewed Jun 12, 2019

View reviewed changes

dalg24 changed the title ~~Improve ArborX_DistributedTree~~ Improve benchmark for distributed tree Jun 12, 2019

dalg24 reviewed Jun 12, 2019

View reviewed changes

masterleinad added 5 commits June 12, 2019 14:42

Beautify ArborX_DistributedTree output

bf3793c

Don't use C++14

7709ba8

Swap '--help' to the end instead

e2069ea

Clarify magic numbers and east const

5d63b80

Remove more magic numbers, improve comments

f478e4b

masterleinad force-pushed the beautify_arborx_distributedtree branch from c083ae2 to f478e4b Compare June 12, 2019 18:42

dalg24 approved these changes Jun 12, 2019

View reviewed changes

masterleinad mentioned this pull request Jun 18, 2019

Compute confidence intervals in DistributedTreeDriver #83

Closed

dalg24 reviewed Jun 18, 2019

View reviewed changes

Use variable for the header label

d1c515c

masterleinad merged commit 51384df into arborx:master Jun 19, 2019

masterleinad deleted the beautify_arborx_distributedtree branch June 19, 2019 13:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve benchmark for distributed tree #78

Improve benchmark for distributed tree #78

masterleinad commented Jun 11, 2019 •

edited by dalg24

masterleinad Jun 11, 2019

masterleinad Jun 11, 2019

aprokop commented Jun 11, 2019

masterleinad commented Jun 11, 2019

dalg24 left a comment

dalg24 Jun 11, 2019

dalg24 Jun 11, 2019

masterleinad Jun 12, 2019

dalg24 Jun 12, 2019

dalg24 Jun 11, 2019

dalg24 Jun 11, 2019

dalg24 commented Jun 11, 2019

dalg24 commented Jun 11, 2019

Rombur Jun 12, 2019

masterleinad Jun 12, 2019

dalg24 Jun 12, 2019

Rombur Jun 12, 2019

masterleinad Jun 12, 2019

dalg24 Jun 12, 2019

dalg24 Jun 12, 2019

dalg24 Jun 12, 2019

dalg24 Jun 12, 2019

dalg24 Jun 18, 2019

dalg24 commented Jun 18, 2019

masterleinad commented Jun 19, 2019

Improve benchmark for distributed tree #78

Improve benchmark for distributed tree #78

Conversation

masterleinad commented Jun 11, 2019 • edited by dalg24

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aprokop commented Jun 11, 2019

masterleinad commented Jun 11, 2019

dalg24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalg24 commented Jun 11, 2019

dalg24 commented Jun 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalg24 commented Jun 18, 2019

masterleinad commented Jun 19, 2019

masterleinad commented Jun 11, 2019 •

edited by dalg24