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.

Sign up for GitHub

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

Only distribute biases over threads if they don't need I/O #638

Merged
merged 5 commits into from
Dec 4, 2023
Merged

Only distribute biases over threads if they don't need I/O #638

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
57e01e0
Appease someone's comments about the name of a function
giacomofiorin Dec 1, 2023
8e6586d
Add function to report when biases need to communicate with replicas
giacomofiorin Dec 1, 2023
bc03dda
Only distribute biases over threads if they don't need I/O
giacomofiorin Dec 1, 2023
095ecec
Lift error conditions with shared ABF and SMP
giacomofiorin Dec 1, 2023
789f22a
More explicit message about SMP at startup
jhenin Dec 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions namd/src/colvarproxy_namd.C
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1466,6 +1466,15 @@ int colvarproxy_namd::compute_volmap(int flags,

#if CMK_SMP && USE_CKLOOP // SMP only

int colvarproxy_namd::check_smp_enabled()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I agree with "someone" about this function name. What was the comment?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Did I misinterpret this comment?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reminder! Back then you suggested replacing the function with two boolean functions. I did just that, except that one of them is not used currently (the one that would indicate if SMP is built into the binary), so I did not add it yet (it would be dead code): we can add it as soon as a use case appears.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please do both cases, so that we can use the same runtime to test SMP and non-SMP code path?

Also, your commit will break NAMD compilation for multicore builds (the GH tests pass only because they are building NAMD without SMP)

{
if (b_smp_active) {
return COLVARS_OK;
}
return COLVARS_ERROR;
}


void calc_colvars_items_smp(int first, int last, void *result, int paramNum, void *param)
{
colvarproxy_namd *proxy = (colvarproxy_namd *) param;
Expand Down
8 changes: 1 addition & 7 deletions namd/src/colvarproxy_namd.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,13 +113,7 @@ class colvarproxy_namd : public colvarproxy, public GlobalMaster {
}

#if CMK_SMP && USE_CKLOOP
int smp_enabled() override
{
if (b_smp_active) {
return COLVARS_OK;
}
return COLVARS_ERROR;
}
int check_smp_enabled() override;

int smp_colvars_loop() override;

Expand Down
6 changes: 6 additions & 0 deletions src/colvarbias.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,12 +447,18 @@ int colvarbias::bin_count(int /* bin_index */)
cvm::error("Error: bin_count() not implemented.\n");
return COLVARS_NOT_IMPLEMENTED;
}

int colvarbias::replica_share()
{
cvm::error("Error: replica_share() not implemented.\n");
return COLVARS_NOT_IMPLEMENTED;
}

size_t colvarbias::replica_share_freq() const
{
return 0;
}


std::string const colvarbias::get_state_params() const
{
Expand Down
4 changes: 4 additions & 0 deletions src/colvarbias.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,13 @@ class colvarbias
//// Give the count at a given bin index.
// FIXME this is currently 1D only
virtual int bin_count(int bin_index);

//// Share information between replicas, whatever it may be.
virtual int replica_share();

/// Report the frequency at which this bias needs to communicate with replicas
virtual size_t replica_share_freq() const;

/// Perform analysis tasks
virtual void analyze() {}

Expand Down
12 changes: 6 additions & 6 deletions src/colvarbias_abf.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,12 +106,6 @@ int colvarbias_abf::init(std::string const &conf)
}
cvm::log("shared ABF will be applied among "+
cvm::to_str(proxy->num_replicas()) + " replicas.\n");
if (cvm::proxy->smp_enabled() == COLVARS_OK) {
cvm::error("Error: shared ABF is currently not available with SMP parallelism; "
"please set \"SMP off\" at the top of the Colvars configuration file.\n",
COLVARS_NOT_IMPLEMENTED);
return COLVARS_NOT_IMPLEMENTED;
}

// If shared_freq is not set, we default to output_freq
get_keyval(conf, "sharedFreq", shared_freq, output_freq);
Expand Down Expand Up @@ -599,6 +593,12 @@ int colvarbias_abf::replica_share() {
}


size_t colvarbias_abf::replica_share_freq() const
{
return shared_freq;
}


template <class T> int colvarbias_abf::write_grid_to_file(T const *grid,
std::string const &filename,
bool close) {
Expand Down
3 changes: 3 additions & 0 deletions src/colvarbias_abf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,12 @@ class colvarbias_abf : public colvarbias {
bool shared_on;
size_t shared_freq;
cvm::step_number shared_last_step;

// Share between replicas -- may be called independently of update
virtual int replica_share();

virtual size_t replica_share_freq() const;

// Store the last set for shared ABF
colvar_grid_gradient *last_gradients;
colvar_grid_count *last_samples;
Expand Down
9 changes: 7 additions & 2 deletions src/colvarbias_meta.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ colvarbias_meta::colvarbias_meta(char const *key)

ebmeta_equil_steps = 0L;

replica_update_freq = 0;
replica_id.clear();
}

Expand Down Expand Up @@ -985,9 +984,9 @@ void colvarbias_meta::recount_hills_off_grid(colvarbias_meta::hill_iter h_first
int colvarbias_meta::replica_share()
{
int error_code = COLVARS_OK;
colvarproxy *proxy = cvm::proxy;
// sync with the other replicas (if needed)
if (comm == multiple_replicas) {
colvarproxy *proxy = cvm::main()->proxy;
// reread the replicas registry
error_code |= update_replicas_registry();
// empty the output buffer
Expand All @@ -998,6 +997,12 @@ int colvarbias_meta::replica_share()
}


size_t colvarbias_meta::replica_share_freq() const
{
return replica_update_freq;
}


int colvarbias_meta::update_replicas_registry()
{
int error_code = COLVARS_OK;
Expand Down
3 changes: 2 additions & 1 deletion src/colvarbias_meta.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ class colvarbias_meta
virtual int update_bias();
virtual int update_grid_data();
virtual int replica_share();
virtual size_t replica_share_freq() const;

virtual int calc_energy(std::vector<colvarvalue> const *values);
virtual int calc_forces(std::vector<colvarvalue> const *values);
Expand Down Expand Up @@ -261,7 +262,7 @@ class colvarbias_meta
std::vector<colvarbias_meta *> replicas;

/// \brief Frequency at which data the "mirror" biases are updated
size_t replica_update_freq;
size_t replica_update_freq = 0;

/// List of replicas (and their output list files): contents are
/// copied into replicas_registry for convenience
Expand Down
28 changes: 21 additions & 7 deletions src/colvarmodule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,14 @@ colvarmodule::colvarmodule(colvarproxy *proxy_in)
" https://doi.org/10.1080/00268976.2013.813594\n"
"as well as all other papers listed below for individual features used.\n");

if (proxy->smp_enabled() == COLVARS_OK) {
cvm::log("SMP parallelism is enabled (num threads = " + to_str(proxy->smp_num_threads()) +
"); use \"smp off\" to disable if needed.\n");
if (proxy->check_smp_enabled() == COLVARS_NOT_IMPLEMENTED) {
cvm::log("SMP parallelism is not available in this build.\n");
} else {
if (proxy->check_smp_enabled() == COLVARS_OK) {
cvm::log("SMP parallelism is enabled (num threads = " + to_str(proxy->smp_num_threads()) + ").\n");
} else {
cvm::log("SMP parallelism is available in this build but not enabled.\n");
}
}

#if (__cplusplus >= 201103L)
Expand Down Expand Up @@ -901,7 +906,7 @@ int colvarmodule::calc_colvars()
}

// if SMP support is available, split up the work
if (proxy->smp_enabled() == COLVARS_OK) {
if (proxy->check_smp_enabled() == COLVARS_OK) {

// first, calculate how much work (currently, how many active CVCs) each colvar has

Expand Down Expand Up @@ -983,8 +988,15 @@ int colvarmodule::calc_biases()
}
}

// if SMP support is available, split up the work
if (proxy->smp_enabled() == COLVARS_OK) {
bool biases_need_io = false;
for (bi = biases_active()->begin(); bi != biases_active()->end(); bi++) {
if (((*bi)->replica_share_freq() > 0) && (step_absolute() % (*bi)->replica_share_freq() == 0)) {
biases_need_io = true;
}
Comment on lines +993 to +995
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is insufficient for metadynamics at least. When newHillFrequency is not the same as replicaUpdateFrequency, biases_need_io would be false. However, in the multi-replica case, metadynamics would still need to open the output stream when projecting a new hill in update_bias() and ((cvm::step_absolute() % new_hill_freq) == 0 (

colvars/src/colvarbias_meta.cpp

Lines 620 to 631 in 632c0db

case multiple_replicas:
add_hill(hill(cvm::step_absolute(), hill_weight*hills_scale,
colvar_values, colvar_sigmas, replica_id));
std::ostream &replica_hills_os =
cvm::proxy->output_stream(replica_hills_file, "replica hills file");
if (replica_hills_os) {
write_hill(replica_hills_os, hills.back());
} else {
return cvm::error("Error: in metadynamics bias \""+this->name+"\""+
((comm != single_replica) ? ", replica \""+replica_id+"\"" : "")+
" while writing hills for the other replicas.\n", COLVARS_FILE_ERROR);
}
), and if metadynamics is not on rank 0 then there would be an error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this!

}

// If SMP support is available, split up the work (unless biases need to use main thread's memory)
if (proxy->check_smp_enabled() == COLVARS_OK && !biases_need_io) {

if (use_scripted_forces && !scripting_after_biases) {
// calculate biases and scripted forces in parallel
Expand All @@ -1000,10 +1012,12 @@ int colvarmodule::calc_biases()
error_code |= calc_scripted_forces();
}

// Straight loop over biases on a single thread
cvm::increase_depth();
for (bi = biases_active()->begin(); bi != biases_active()->end(); bi++) {
error_code |= (*bi)->update();
if (cvm::get_error()) {
cvm::decrease_depth();
return error_code;
}
}
Expand Down Expand Up @@ -1955,7 +1969,7 @@ size_t & colvarmodule::depth()
{
// NOTE: do not call log() or error() here, to avoid recursion
colvarmodule *cv = cvm::main();
if (proxy->smp_enabled() == COLVARS_OK) {
if (proxy->check_smp_enabled() == COLVARS_OK) {
int const nt = proxy->smp_num_threads();
if (int(cv->depth_v.size()) != nt) {
proxy->smp_lock();
Expand Down
6 changes: 3 additions & 3 deletions src/colvarproxy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ colvarproxy_smp::~colvarproxy_smp()
}


int colvarproxy_smp::smp_enabled()
int colvarproxy_smp::check_smp_enabled()
{
#if defined(_OPENMP)
if (b_smp_active) {
Expand Down Expand Up @@ -492,8 +492,8 @@ colvarproxy::~colvarproxy()

bool colvarproxy::io_available()
{
return (smp_enabled() == COLVARS_OK && smp_thread_id() == 0) ||
(smp_enabled() != COLVARS_OK);
return (check_smp_enabled() == COLVARS_OK && smp_thread_id() == 0) ||
(check_smp_enabled() != COLVARS_OK);
}


Expand Down
2 changes: 1 addition & 1 deletion src/colvarproxy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,7 @@ class colvarproxy_smp {
bool b_smp_active;

/// Whether threaded parallelization is available (TODO: make this a cvm::deps feature)
virtual int smp_enabled();
virtual int check_smp_enabled();

/// Distribute calculation of colvars (and their components) across threads
virtual int smp_colvars_loop();
Expand Down