Fix threading issue when connecting #1276

stinebuu · 2019-08-27T12:09:00Z

A little while ago @heplesser found that when trying to reproduce the figures from Ippen et al (2017), the strong scaling by threads variant behaved badly with master. Luckily, he found the problem as well, which is that a lot of threads are trying to write to have_connections_changed_ at once. This PR use CompletedChecker for have_connections_changed_ to make sure that all threads do not write to the same variable.

A benchmark can be found here: Fig_5_results_one_node_noinst.pdf The benchmark is from July, 0ac2385 was master at the time, and 8fa2e37 contains the fix. With the fix we see that strong scaling by threads and strong scaling by mpi again are equal when we connect.

This PR also cleans up some of our thread-safe connection routines. Instead of passing around one static empty dummy dictionary when connecting, we use a list of empty dictionaries, one dict per thread.

…_issues

threading_issues

jakobj

looks already quite good, thanks for fixing this! i've added a few comments that should be addressed

nestkernel/conn_builder.cpp

jakobj · 2019-09-03T16:00:56Z

nestkernel/connection_manager.cpp

@@ -427,7 +426,10 @@ nest::ConnectionManager::connect( const index sgid,
 {
  kernel().model_manager.assert_valid_syn_id( syn_id );

-  have_connections_changed_ = true;
+  if ( not have_connections_changed_[ target_thread ] )


do we need this check here? isn't it cheaper to just write in all cases? the logic wouldn't change, would it?

@jakobj I tested on Blaustein, and if we do not check before setting the parameter, then the performance gets a lot worse. You can see it as 4c388f4 here. It must have something to do with all the threads trying to access at once and maybe something with the omp automatic write in the set function. I wrote a comment before every if to try to explain and make sure nobody removes the if without testing the performance.

ok,thanks for clarifying

jakobj · 2019-09-03T16:01:37Z

nestkernel/connection_manager.cpp

  const thread tid = kernel().vp_manager.get_thread_id();

+  if ( not have_connections_changed_[ tid ] )


(see above)

nestkernel/connection_manager.cpp

jakobj · 2019-09-03T16:03:47Z

nestkernel/connection_manager.cpp

@@ -694,7 +693,10 @@ nest::ConnectionManager::find_connection( const thread tid, const synindex syn_i
 void
 nest::ConnectionManager::disconnect( const thread tid, const synindex syn_id, const index sgid, const index tgid )
 {
-  have_connections_changed_ = true;
+  if ( not have_connections_changed_[ tid ] )


(see above)

nestkernel/connector_base.h

nestkernel/connector_model_impl.h

nestkernel/sp_manager.cpp

topology/connection_creator.cpp

jakobj · 2019-09-03T16:09:41Z

topology/connection_creator.cpp

 ConnectionCreator::ConnectionCreator( DictionaryDatum dict )
  : allow_autapses_( true )
  , allow_multapses_( true )
+  , allow_oversized_( false )


what is this used for?

I think I just added it because I noticed it wasn't initialized. I can remove it again.

@jakobj If allow_oversized_ is not initialized here, the test test_oversized_mask.sli fails. I don't understand why the test suddenly fails here and not in master though.

uninitialized is always bad, thanks for fixing this.

This reverts commit cacb5de.

but just for mpi, as this is easier to read.

stinebuu · 2019-11-05T09:46:47Z

@jakobj First of all, sorry for taking way too long to address your comments! I have made some changes based on your comments. Concerning your comments about why I don't set have_connections_changed_ directly, I want to do some benchmarking again. I feel like there was a reason for this, but as I can't remember it I will test it out, and then either add a comment or fix it.

So no need to do another review yet.

heplesser · 2019-11-26T10:33:25Z

@jakobj @jarsi Could you (re-)review this PR in the near future? It is one of two PRs for 2.18.1 still open.

stinebuu · 2019-11-28T09:21:13Z

@heplesser It is hanging on me I'm afraid.

…ssues_test

stinebuu · 2019-12-17T12:03:25Z

@jakobj @jarsi The PR is again ready for the review, sorry for the delay.

Somewhere along the way the benchmarks showed worse performance again, and it took some time to detect the problem, but that is fixed now: benchmark_results.pdf

nestkernel/conn_builder.h

jakobj

@stinebuu thanks! everything looks good, just one minor comment where we could use std function. otherwise 👍

jakobj · 2020-01-09T10:15:19Z

nestkernel/completed_checker.cpp

@@ -64,6 +64,34 @@ CompletedChecker::all_true() const
  return true;
 }

+bool
+CompletedChecker::any_false() const


why not use std::any_of since we use c++11 now? we should probably also replace the other functions, maybe in a separate PR though

jakobj · 2020-01-09T10:15:32Z

nestkernel/completed_checker.cpp

+}
+
+bool
+CompletedChecker::any_true() const


jakobj · 2020-01-09T10:16:44Z

nestkernel/connection_manager.cpp

@@ -427,7 +426,10 @@ nest::ConnectionManager::connect( const index sgid,
 {
  kernel().model_manager.assert_valid_syn_id( syn_id );

-  have_connections_changed_ = true;
+  if ( not have_connections_changed_[ target_thread ] )


ok,thanks for clarifying

jakobj · 2020-01-09T10:19:22Z

topology/connection_creator.cpp

 ConnectionCreator::ConnectionCreator( DictionaryDatum dict )
  : allow_autapses_( true )
  , allow_multapses_( true )
+  , allow_oversized_( false )


uninitialized is always bad, thanks for fixing this.

jarsi

Thanks for the work, the pr looks fine to me.

heplesser · 2020-01-09T21:34:24Z

@jakobj Thanks for you comments! We cannot use std::any_of for two reasons: The underlying data type of a_ is a C-style array to allow OpenMP atomic updates and we need and OpenMP barrier before the "any" check.

As all reviewers have approved now, I will merge.

heplesser and others added 12 commits July 7, 2019 12:12

Removed spurious thread_local_connectors update.

cacb5de

Replaced global dummy_param dict with thread-specific ones.

07c3898

Added any_false and any_true to completed_checker

d8166a2

have_connections_changed -> set on each thread.

f760dc9

check if already changed

4861470

check if connection already changed on thread

03b128e

update on primary and secondary connections checks.

eac58cb

Merge remote-tracking branch 'origin/threading_issues' into threading…

8fa2e37

…_issues

Use vector of dummy param dicts in topology Connect

af688ac

allow_oversized_ must be initialized.

c31aeb4

Wait for all threads to finish

fd96ac1

Merge remote-tracking branch 'nest-simulator/master' into

59acedf

threading_issues

heplesser requested review from jakobj and jarsi August 27, 2019 12:11

stinebuu added 2 commits August 27, 2019 14:25

clang

6e77648

Updated barriers

c19006f

terhorstd added this to the NEST 2.18.1 milestone Sep 2, 2019

jakobj suggested changes Sep 3, 2019

View reviewed changes

stinebuu added 3 commits November 5, 2019 09:19

Use const, brackets instead of at and revert back to if/else

735727e

revert formatting changes not relevant for PR

2cfa778

Revert "Removed spurious thread_local_connectors update."

6b83ff7

This reverts commit cacb5de.

stinebuu force-pushed the threading_issues branch from 7de30bd to b16db1f Compare November 5, 2019 09:35

Go back to not syncing primary and secondary connections over threads,

63c0862

but just for mpi, as this is easier to read.

stinebuu force-pushed the threading_issues branch from b16db1f to 63c0862 Compare November 5, 2019 09:37

stinebuu mentioned this pull request Nov 5, 2019

Remove spurious thread_local_connectors update. #1333

Merged

stinebuu added 3 commits December 10, 2019 09:29

Merge remote-tracking branch 'nest-simulator/master' into threading_i…

fdb040d

…ssues_test

Wait for threads

a0ac05d

Add comments about why we check the parameter before we set it.

6ec5807

heplesser reviewed Dec 17, 2019

View reviewed changes

nestkernel/conn_builder.h Outdated Show resolved Hide resolved

Added comment about why we create one dictionary per thread

8562217

heplesser approved these changes Jan 7, 2020

View reviewed changes

jakobj approved these changes Jan 9, 2020

View reviewed changes

jarsi approved these changes Jan 9, 2020

View reviewed changes

heplesser merged commit 02a44ba into nest:master Jan 9, 2020

jakobj mentioned this pull request Jan 13, 2020

Add warning about invalid connection descriptors when new connections are created #1250

Merged

stinebuu deleted the threading_issues branch April 7, 2020 06:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix threading issue when connecting #1276

Fix threading issue when connecting #1276

stinebuu commented Aug 27, 2019 •

edited

Loading

jakobj left a comment

jakobj Sep 3, 2019

stinebuu Dec 17, 2019 •

edited

Loading

jakobj Jan 9, 2020

jakobj Sep 3, 2019

jakobj Sep 3, 2019

jakobj Sep 3, 2019

stinebuu Nov 5, 2019

stinebuu Dec 17, 2019

jakobj Jan 9, 2020

stinebuu commented Nov 5, 2019

heplesser commented Nov 26, 2019

stinebuu commented Nov 28, 2019

stinebuu commented Dec 17, 2019

jakobj left a comment

jakobj Jan 9, 2020

jakobj Jan 9, 2020

jakobj Jan 9, 2020

jakobj Jan 9, 2020

jarsi left a comment

heplesser commented Jan 9, 2020

		const thread tid = kernel().vp_manager.get_thread_id();

		if ( not have_connections_changed_[ tid ] )

Fix threading issue when connecting #1276

Fix threading issue when connecting #1276

Conversation

stinebuu commented Aug 27, 2019 • edited Loading

jakobj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stinebuu Dec 17, 2019 • edited Loading

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

stinebuu commented Nov 5, 2019

heplesser commented Nov 26, 2019

stinebuu commented Nov 28, 2019

stinebuu commented Dec 17, 2019

jakobj 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

jarsi left a comment

Choose a reason for hiding this comment

heplesser commented Jan 9, 2020

stinebuu commented Aug 27, 2019 •

edited

Loading

stinebuu Dec 17, 2019 •

edited

Loading