Refactor ghosts.cpp #3216

hirschsn · 2019-09-26T14:03:36Z

More refactoring that builds upon #3212

Description of major changes:

remove GhostCommunication::mpi_comm as it is not used in ghosts.cpp,
make GhostCommunication::part_lists a std::vector,
remove static variables s_buffer and r_buffer,
factor out memory handling,
change loops to range based for,
use boost::mpi.
Replace the manual poststore and prefetch loops by find_ifs

Mainly, ghosts.cpp now defines CommBuf, which is a container for the data to be sent or received as well as two classes (Archiver and BondArchiver), that insert and extract the memory from CommBuf.

Some of these changes, like the removal of static variables, is necessary for my implementation of asynchronous ghost communication.

PR Checklist

Tests?
- Interface
- Core
Docs?

GhostCommunicator::mpi_comm does not appear in ghosts.cpp. Function ghosts_communicator() uses only global comm_cart, so not even the comment of this field is correct anymore.

- Remove static buffers - Group data and bond buffers together - Factor out memory handling

This is now handled by sending the bond buffer as std::vector and boost::mpi.

Removes one level on indentation from ghost_communicator()

codecov · 2019-09-27T09:29:12Z

Codecov Report

Merging #3216 into python will increase coverage by <1%.
The diff coverage is 98%.

@@           Coverage Diff           @@
##           python   #3216    +/-   ##
=======================================
+ Coverage      85%     85%   +<1%     
=======================================
  Files         528     528            
  Lines       25790   25733    -57     
=======================================
- Hits        22148   22116    -32     
+ Misses       3642    3617    -25

Impacted Files	Coverage Δ
src/utils/tests/Span_test.cpp	`100% <100%> (ø)`	⬆️
src/core/nsquare.cpp	`100% <100%> (ø)`	⬆️
src/core/domain_decomposition.cpp	`94% <100%> (-1%)`	⬇️
src/utils/include/utils/Span.hpp	`100% <100%> (ø)`	⬆️
src/core/ghosts.hpp	`100% <100%> (ø)`	⬆️
src/core/layered.cpp	`78% <50%> (ø)`	⬆️
src/core/ghosts.cpp	`99% <99%> (+8%)`	⬆️
src/core/particle_data.cpp	`96% <0%> (-1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b7b357...f21d717. Read the comment docs.

hirschsn · 2019-09-28T19:21:00Z

@RudolfWeeber Sure! Just propose a time and date.

fweik · 2019-10-01T08:16:48Z

I'll have a look at the changes today, and then we can decide how to proceed with this.

…ghost-cleanup

fweik · 2019-10-01T14:53:49Z

This looks good to me. I'd plan to do something similar in the future but did not get around to it yet, so thank you :-). I've made some cosmetic changes and made a clearer separation between the two archiver classes so that they can be tested better in the future. Please let me now if you are d'accord with this, I did not mean to hijack your PR.

RudolfWeeber · 2019-10-01T16:39:38Z

@hirschsn, @fweik, either during the summer school or the week after. Letś organize it offline, once Iḿ back on the 7th.

hirschsn · 2019-10-01T17:22:14Z

@fweik Feel free to hijack any PR you like. :) I'm totally fine with your changes, they definitely improve this PR. Thanks!
@RudolfWeeber just ping me when you're back.

KaiSzuttor · 2019-10-02T09:28:50Z

src/core/ghosts.cpp

-    max_s_buffer = n_s_buffer;
-    s_buffer = Utils::realloc(s_buffer, max_s_buffer);
-  }
+  s_buffer.resize(calc_transmit_size(gc, data_parts));


a lot of the code would be easier to read if the variable names would be just a little more descriptive: e.g.:

send_buffer instead of s_buffer,

rec_buffer instead of r_buffer,

ghost_comm instead of gc,

archiver instead of ar,

bond_archiver instead of bar,

bond_list instead of bl,

part instead of pt,

part_list instead of cur_list

KaiSzuttor · 2019-10-02T09:29:26Z

src/core/ghosts.cpp

-static int max_s_buffer = 0;
-/** send buffer. Just grows, which should be ok */
-static char *s_buffer = nullptr;
+struct CommBuf {


please add a docstring for the struct

also think about using class instead of struct here. core guideline

KaiSzuttor · 2019-10-02T09:47:15Z

src/core/ghosts.cpp

+      /* send op; write back delayed data from last recv, when this was a
+       * prefetch send. */
+      /* find previous action where we recv and which has PSTSTORE set */
+      auto postst_gcn = std::find_if(


please find a readable name for this variable

hirschsn · 2019-10-02T13:12:22Z

@KaiSzuttor Thanks for the suggestions. Done.

I now implemented the following convention in both, ghosts.hpp and ghosts.cpp:

"GhostCommunication" variables are always "ghost_comm".
"GhostCommunicator" variables are always named "gcr".

The latter one is not very descriptive but that is by choice. Otherwise there would be too much similarity to ghost_communication/ghost_comm/... which is also irritating. If anyone can think of a better combination of names, please tell me.

Also, I sneaked in two more changes: Another pointer->reference conversion and the removal of two superfluous pointer variables in cell_cell_transfer. Could you please take another look at these?

jngrad · 2019-10-18T09:04:38Z

bors r=KaiSzuttor

3216: Refactor ghosts.cpp r=KaiSzuttor a=hirschsn More refactoring that builds upon #3212 Description of *major* changes: - remove GhostCommunication::mpi_comm as it is not used in ghosts.cpp, - make GhostCommunication::part_lists a std::vector, - remove static variables s_buffer and r_buffer, - factor out memory handling, - change loops to range based for, - use boost::mpi. - Replace the manual poststore and prefetch loops by find_ifs Mainly, ghosts.cpp now defines CommBuf, which is a container for the data to be sent or received as well as two classes (Archiver and BondArchiver), that insert and extract the memory from CommBuf. Some of these changes, like the removal of static variables, is necessary for my implementation of asynchronous ghost communication. PR Checklist ------------ - [ ] Tests? - [ ] Interface - [ ] Core - [ ] Docs? 3239: Added test criteria for the charged_system-2 tutorial r=RudolfWeeber,jngrad a=reinaual 3253: Refactor NpT public interface r=fweik a=jngrad Description of changes: - remove the silent conversion of the incorrect input parameter `dimension=[0,0,0]` to `[1,1,1]` in the core (bypassing sanity checks), now the checks will throw an exception for fixed-volume NpT; the original behavior was counter-intuitive and undocumented until 2 days ago - remove the automatic decay of NpT to NVT upon initialization of NpT with incorrect parameters - remove unused `p_inst_av` variable (average instantaneous pressure) - cleanup integrator documentation 3258: CMake minor fixes r=fweik a=jngrad Description of changes: - change next milestone to 4.2 - load `GNUInstallDirs` to make standard GNU paths accessible from CMake variables - simplify CMake logic and install in `python3.X` folder instead of the deprecated `python3` folder - add extra check to make sure install paths are correctly configured (all python and shared object files must be inside the package `espressomd`) Co-authored-by: Steffen Hirschmann <steffen.hirschmann@ipvs.uni-stuttgart.de> Co-authored-by: Florian Weik <fweik@icp.uni-stuttgart.de> Co-authored-by: Alexander Reinauer <st144434@stud.uni-stuttgart.de> Co-authored-by: Jean-Noël Grad <jgrad@icp.uni-stuttgart.de>

bors · 2019-10-18T10:03:41Z

Build succeeded

ICP GitLab CI

hirschsn added 14 commits September 26, 2019 12:40

ghosts.cpp: Make predicates return bool

85f235d

ghosts.cpp: Remove unused MPI_Status variable

b2444d6

ghosts.cpp: Use clear() instead of resize(0)

3bb4449

ghosts: Remove unused mpi_comm field

aaab228

GhostCommunicator::mpi_comm does not appear in ghosts.cpp. Function ghosts_communicator() uses only global comm_cart, so not even the comment of this field is correct anymore.

Remove left-over typedefs from C days

ffe7113

ghosts: Make part_lists a std::vector

09f5140

ghosts: Make s_buffer and r_buffer std::vectors

eab1470

ghosts: Factor out memory handling

03708a2

- Remove static buffers - Group data and bond buffers together - Factor out memory handling

ghosts: Use boost::mpi for recv, send and bcst

5d77262

Explicitly cast length to int

a5b811d

ghost_rdce: Use boost::mpi

56e4572

ghosts: Remove n_bonds from payload

2b910e8

This is now handled by sending the bond buffer as std::vector and boost::mpi.

ghosts: Use range-based for loops

c507697

ghosts: Reformat

646d468

jngrad added this to the Espresso 5 milestone Sep 26, 2019

hirschsn added 5 commits September 27, 2019 11:00

More reformats

a847b1c

Update comment

8e8c581

ghosts: Change two more instances of n_X_buffer

b20fe3d

ghosts: Remove else. Check must always hold.

b872b68

ghosts: Early exit on GHOST_LOCL

0408706

Removes one level on indentation from ghost_communicator()

hirschsn added 4 commits September 27, 2019 14:49

ghosts: Remove prefetch loop

70f5a60

ghosts: else+if -> else if

8097ec3

ghosts: More else if

8a6ca56

ghosts: Change friend decl to struct

f60d634

hirschsn changed the title ~~More ghost cleanup~~ Refactor ghosts.cpp Sep 27, 2019

hirschsn added 4 commits September 27, 2019 15:19

ghosts: Remove poststore loop

71de951

ghosts: Make helper functions take references

551699d

ghosts: Make helper functions static

0d7aa14

add_forces: range based for loop

84bea92

fweik self-assigned this Oct 1, 2019

fweik added 5 commits October 1, 2019 11:13

Merge branch 'python' of https://github.com/espressomd/espresso into …

50f789a

…ghost-cleanup

Merge branch 'python' of https://github.com/espressomd/espresso into …

b5bd6e8

…ghost-cleanup

ghosts: cleanup

f060558

utils: make_span overload from container

d021d80

ghosts: Decouple Archiver and BondArchiver

1ab35ff

fweik requested a review from KaiSzuttor October 1, 2019 14:53

fweik added Core Improvement labels Oct 1, 2019

KaiSzuttor suggested changes Oct 2, 2019

View reviewed changes

hirschsn added 6 commits October 2, 2019 14:32

Make CommBuf a class and add documentation

513a8d5

cell_cell_transfer: Remove pointers

8df0132

ghosts: Rename variables

23007cb

Make GhostCommunicator variables always "gcr".

0b50cb2

ghosts: Convert "additional checks" into asserts

aef12a5

ghosts: Another pointer->reference conversion

c62e278

KaiSzuttor approved these changes Oct 4, 2019

View reviewed changes

Merge branch 'python' into ghost-cleanup

f21d717

jngrad modified the milestones: Espresso 5, Espresso 4.2 Oct 18, 2019

bors bot merged commit f21d717 into espressomd:python Oct 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ghosts.cpp #3216

Refactor ghosts.cpp #3216

hirschsn commented Sep 26, 2019 •

edited

Loading

codecov bot commented Sep 27, 2019 •

edited

Loading

hirschsn commented Sep 28, 2019

fweik commented Oct 1, 2019

fweik commented Oct 1, 2019 •

edited

Loading

RudolfWeeber commented Oct 1, 2019

hirschsn commented Oct 1, 2019

KaiSzuttor Oct 2, 2019

KaiSzuttor Oct 2, 2019

KaiSzuttor Oct 2, 2019

KaiSzuttor Oct 2, 2019

hirschsn commented Oct 2, 2019 •

edited

Loading

jngrad commented Oct 18, 2019

bors bot commented Oct 18, 2019

Refactor ghosts.cpp #3216

Refactor ghosts.cpp #3216

Conversation

hirschsn commented Sep 26, 2019 • edited Loading

PR Checklist

codecov bot commented Sep 27, 2019 • edited Loading

Codecov Report

hirschsn commented Sep 28, 2019

fweik commented Oct 1, 2019

fweik commented Oct 1, 2019 • edited Loading

RudolfWeeber commented Oct 1, 2019

hirschsn commented Oct 1, 2019

KaiSzuttor Oct 2, 2019

Choose a reason for hiding this comment

KaiSzuttor Oct 2, 2019

Choose a reason for hiding this comment

KaiSzuttor Oct 2, 2019

Choose a reason for hiding this comment

KaiSzuttor Oct 2, 2019

Choose a reason for hiding this comment

hirschsn commented Oct 2, 2019 • edited Loading

jngrad commented Oct 18, 2019

bors bot commented Oct 18, 2019

Build succeeded

hirschsn commented Sep 26, 2019 •

edited

Loading

codecov bot commented Sep 27, 2019 •

edited

Loading

fweik commented Oct 1, 2019 •

edited

Loading

hirschsn commented Oct 2, 2019 •

edited

Loading