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

#941 added subphase support to loadComm #1081

Merged
merged 15 commits into from
Oct 15, 2020

Conversation

bradybray
Copy link

fixes #941 implementing subphases in loadComm

@bradybray bradybray changed the title #941: added subphase support to loadComm 941: added subphase support to loadComm Sep 23, 2020
@bradybray bradybray changed the title 941: added subphase support to loadComm #941 added subphase support to loadComm Sep 23, 2020
@codecov
Copy link

codecov bot commented Sep 23, 2020

Codecov Report

Merging #1081 into develop will increase coverage by 0.00%.
The diff coverage is 91.30%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1081   +/-   ##
========================================
  Coverage    75.60%   75.61%           
========================================
  Files          719      719           
  Lines        27239    27255   +16     
========================================
+ Hits         20595    20609   +14     
- Misses        6644     6646    +2     
Impacted Files Coverage Δ
src/vt/vrt/collection/balance/elm_stats.h 100.00% <ø> (ø)
src/vt/vrt/collection/balance/node_stats.h 100.00% <ø> (ø)
src/vt/vrt/collection/balance/node_stats.cc 74.72% <66.66%> (-0.28%) ⬇️
src/vt/vrt/collection/balance/elm_stats.cc 77.65% <100.00%> (+2.36%) ⬆️
src/vt/vrt/collection/balance/elm_stats.impl.h 100.00% <100.00%> (ø)

@PhilMiller
Copy link
Member

PhilMiller commented Sep 23, 2020

So, to carry this the rest of the way through, we'll need a per-subphase structure alongside node_comm_ in node_stats.h, and appropriate recording into it in NodeStats::addNodeStats. That means ElementStats will need to record it in ElementStats::recv* to provide the data later

@PhilMiller PhilMiller marked this pull request as draft September 23, 2020 14:39
@bradybray
Copy link
Author

@PhilMiller

So, to carry this the rest of the way through, we'll need a per-subphase structure alongside node_comm_ in node_stats.h, and appropriate recording into it in NodeStats::addNodeStats. That means ElementStats will need to record it in ElementStats::recv* to provide the data later

std::unordered_map<PhaseType, SubphaseLoadMapType> node_subphase_data_;
/// Local migration type-free lambdas for each object
std::unordered_map<ElementIDType,MigrateFnType> node_migrate_;
/// Map of temporary ID to permanent ID
std::unordered_map<ElementIDType,ElementIDType> node_temp_to_perm_;
/// Map of permanent ID to temporary ID
std::unordered_map<ElementIDType,ElementIDType> node_perm_to_temp_;
/// Map from element temporary ID to the collection's virtual proxy (untyped)
std::unordered_map<ElementIDType,VirtualProxyType> node_collection_lookup_;
/// Node communication graph for each local object
std::unordered_map<PhaseType, CommMapType> node_comm_;

So I see there is a node_subphase_data_ structure on line 257. Is this not a sufficient structure for this implementation?

Ah, as I wrote this I saw something on line 267. Would
std::unordered_map<SubphaseType, CommMapType> node_comm_s; be sufficient for the structure?
I'll start peeking into the other files to provide the additional implementation. I'm sure I'll have some more comments to follow ;)

@lifflander
Copy link
Collaborator

Ah, as I wrote this I saw something on line 267. Would
std::unordered_map<SubphaseType, CommMapType> node_comm_s; be sufficient for the structure?
I'll start peeking into the other files to provide the additional implementation. I'm sure I'll have some more comments to follow ;)

I think you will to refactor to be std::unordered_map<PhaseType, std::unordered_map<SubphaseType, CommMapType>> node_comm_;

@bradybray
Copy link
Author

@lifflander @PhilMiller /usr/bin/ld: final link failed: No space left on device This directory gets full?

@bradybray bradybray force-pushed the 941-record-communication-stats-per-subphase branch from aa73485 to 4919ca3 Compare October 13, 2020 04:41
@PhilMiller
Copy link
Member

It looks like the last commit, "Checkpoint before rebase", reverts all of the intended changes in the rest of the PR.

@bradybray
Copy link
Author

bradybray commented Oct 13, 2020

It looks like the last commit, "Checkpoint before rebase", reverts all of the intended changes in the rest of the PR.

Ok. Do I revert back to the previous commit then push?

@PhilMiller
Copy link
Member

I think you would want to git reset to the previous commit

@bradybray bradybray force-pushed the 941-record-communication-stats-per-subphase branch from 4919ca3 to 3326259 Compare October 13, 2020 17:12
@bradybray
Copy link
Author

I think you would want to git reset to the previous commit

Yep that is what I ended up doing. I force pushed the commit.

@bradybray
Copy link
Author

I think you would want to git reset to the previous commit

As for the conflicts; which side of the conflict do I take? I assume I should be taking the side from develop and not my own branch? The conflicts appear to be from the runtime library.

@PhilMiller
Copy link
Member

There shouldn't be conflicts at all - your changes don't touch those files.

@bradybray bradybray force-pushed the 941-record-communication-stats-per-subphase branch from 3326259 to 655358f Compare October 13, 2020 20:23
@bradybray bradybray force-pushed the 941-record-communication-stats-per-subphase branch 4 times, most recently from 9943029 to f2ad333 Compare October 15, 2020 15:50
@bradybray bradybray force-pushed the 941-record-communication-stats-per-subphase branch from f2ad333 to dcb917c Compare October 15, 2020 16:01
@PhilMiller PhilMiller marked this pull request as ready for review October 15, 2020 16:09
@PhilMiller PhilMiller merged commit 2d159ad into develop Oct 15, 2020
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.

Record communication stats per-subphase
3 participants