Remove superfluous OpenMP barriers #2098

stinebuu · 2021-07-02T12:40:38Z

This PR fixes #1441.

Tested on Piz Daint and JUSUF.

stinebuu · 2021-08-30T08:54:46Z

@suku248 @wschenck Would you have time to review this this week? Or should we try to find other reviewers?

heplesser · 2021-09-02T22:25:14Z

@stinebuu I think we should wait for the original reviewers on this one, even if that should delay it to NEST 3.2.

jougs · 2021-09-17T09:19:49Z

@suku248, @wschenck: ping!

jougs · 2021-11-11T08:10:24Z

@suku248, @wschenck: could you please indicate if you are available for reviewing this or not? Thanks!

terhorstd · 2022-01-17T08:20:43Z

Last call for passengers @suku248 and @wschenck …

wschenck

I think the removal of these barriers is sound and reasonable with minimal changes to the measured runtime of deliver_spike_data.

wschenck · 2022-01-19T19:48:45Z

nestkernel/event_delivery_manager.cpp

@@ -446,7 +442,6 @@ EventDeliveryManager::gather_spike_data_( const thread tid,
        decrease_buffer_size_spike_data_ = false;
      }
    }
-#pragma omp barrier



I agree that we don't need this barrier since gather_completed_checker_.any_false() also invokes a barrier.

wschenck · 2022-01-19T19:50:20Z

nestkernel/event_delivery_manager.cpp

@@ -706,7 +701,6 @@ EventDeliveryManager::gather_target_data( const thread tid )
    if ( gather_completed_checker_.all_true() )
    {
      set_complete_marker_target_data_( assigned_ranks, send_buffer_position );
-#pragma omp barrier
    }


Agree here: Superfluous barrier because of preceding invocation of gather_completed_checker_.all_true().

wschenck · 2022-01-19T19:51:55Z

nestkernel/event_delivery_manager.cpp

@@ -726,7 +720,6 @@ EventDeliveryManager::gather_target_data( const thread tid )

    const bool distribute_completed = distribute_target_data_buffers_( tid );
    gather_completed_checker_[ tid ].logical_and( distribute_completed );
-#pragma omp barrier



Agree here: Superfluous barrier because of following invocation of gather_completed_checker_.any_false().

wschenck · 2022-01-19T19:52:51Z

nestkernel/event_delivery_manager.cpp

@@ -736,7 +729,6 @@ EventDeliveryManager::gather_target_data( const thread tid )
        buffer_size_target_data_has_changed_ = kernel().mpi_manager.increase_buffer_size_target_data();
      }
    }
-#pragma omp barrier


Agree here: Superfluous barrier because of preceding invocation of gather_completed_checker_.any_false().

I agree with removing it, but I think the reason is that we will hit the following implicit barrier when checking for the while condition.

wschenck · 2022-01-19T19:53:59Z

nestkernel/event_delivery_manager.cpp

-// Exit gather loop if all local threads and remote processes are
-// done.
-#pragma omp barrier
-


With barrier, we measure for deliver_spike_data the runtime of the slowest thread; without barrier, we measure most likely the average runtime over all threads. I think, both variants are ok.

Code provisions made for benchmarking should not reduce speed under production conditions, so I think it is sensible to remove this barrier.

Would this fall into the category of performance instrumentation that was discussed some time ago? We could make this barrier dependent on a cmake flag, but it might also confuse more than it helps. In any case, for sure this important difference in measurement must also be expressed in the documentation of the kernel timers!
@stinebuu, could you check if this difference is clear enough in the docs?

Would this be a problem for comparing benchmarks between NEST versions with/without this barrier?

wschenck · 2022-01-19T20:03:12Z

Sorry for not answering earlier, I completely missed that some of the large number of emails on the nest-simulator mailing list contained my name. I will change the settings of my email program to avoid this in the future.

heplesser · 2022-01-20T06:26:22Z

Sorry for not answering earlier, I completely missed that some of the large number of emails on the nest-simulator mailing list contained my name. I will change the settings of my email program to avoid this in the future.

Hi @wschenck! Thank you for your review!

jougs · 2022-01-28T19:25:10Z

Honestly, I don't think that it does not get much better than it is with the review by @wschenck. Together with the tests that @stinebuu carried out already earlier on Piz Daint and JUSUF, I think we should once the question by @terhorstd above is resolved.

@wschenck: how bad would the difference between old and new measurements be for the timers you mention above?

wschenck · 2022-01-30T22:24:41Z

@jougs: I expect slight differences only for the timer deliver_spike_data. From a max operation (before) to a mean operation (after code change in PR) over threads. The resulting difference depends on the variance between threads. I guess that this variance is very small so that the measurement differences would be neglible.

jarsi

Thanks for your work @stinebuu. I agree, it appears to be save to remove all these barriers.

jarsi · 2022-03-18T13:37:31Z

nestkernel/event_delivery_manager.cpp

@@ -736,7 +729,6 @@ EventDeliveryManager::gather_target_data( const thread tid )
        buffer_size_target_data_has_changed_ = kernel().mpi_manager.increase_buffer_size_target_data();
      }
    }
-#pragma omp barrier


I agree with removing it, but I think the reason is that we will hit the following implicit barrier when checking for the while condition.

removed some superfluous barriers

bd526e4

stinebuu added S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Jul 2, 2021

stinebuu added this to PRs in progress in Kernel via automation Jul 2, 2021

stinebuu added this to the NEST 3.1 milestone Jul 2, 2021

heplesser requested review from suku248 and wschenck July 2, 2021 12:47

heplesser modified the milestones: NEST 3.1, NEST 3.2 Sep 3, 2021

heplesser removed this from the NEST 3.2 milestone Dec 20, 2021

wschenck approved these changes Jan 19, 2022

View reviewed changes

stinebuu requested review from jarsi and removed request for wschenck March 1, 2022 11:13

heplesser removed the request for review from suku248 March 1, 2022 13:00

jarsi approved these changes Mar 18, 2022

View reviewed changes

jougs changed the title ~~Remove superfluous omp barriers~~ Remove superfluous OpenMP barriers Mar 18, 2022

jougs merged commit 66269a2 into nest:master Mar 18, 2022

Kernel automation moved this from PRs in progress to Done (PRs and issues) Mar 18, 2022

stinebuu deleted the remove_superfluous_barriers branch March 22, 2022 08:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove superfluous OpenMP barriers #2098

Remove superfluous OpenMP barriers #2098

stinebuu commented Jul 2, 2021

stinebuu commented Aug 30, 2021

heplesser commented Sep 2, 2021

jougs commented Sep 17, 2021

jougs commented Nov 11, 2021

terhorstd commented Jan 17, 2022

wschenck left a comment

wschenck Jan 19, 2022

wschenck Jan 19, 2022

wschenck Jan 19, 2022

wschenck Jan 19, 2022

jarsi Mar 18, 2022

wschenck Jan 19, 2022

heplesser Jan 20, 2022

terhorstd Jan 20, 2022

wschenck commented Jan 19, 2022 •

edited

heplesser commented Jan 20, 2022

jougs commented Jan 28, 2022

wschenck commented Jan 30, 2022

jarsi left a comment

jarsi Mar 18, 2022

Remove superfluous OpenMP barriers #2098

Remove superfluous OpenMP barriers #2098

Conversation

stinebuu commented Jul 2, 2021

stinebuu commented Aug 30, 2021

heplesser commented Sep 2, 2021

jougs commented Sep 17, 2021

jougs commented Nov 11, 2021

terhorstd commented Jan 17, 2022

wschenck 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wschenck commented Jan 19, 2022 • edited

heplesser commented Jan 20, 2022

jougs commented Jan 28, 2022

wschenck commented Jan 30, 2022

jarsi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wschenck commented Jan 19, 2022 •

edited