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

[giacomofiorin]

Prevents a possible race condition when running multiple-walkers metadynamics with threads. This didn't use to be an issue until recently, when objects do all I/O operations by modifying members of the colvarmodule class.

This fix also removes an earlier restriction on multiple-walkers ABF. No doc changes are needed, since that was not documented anyway.

Possible TODO: check if SMP can be enabled again on NAMD CI jobs, which could have been failing because it's the only engine currently having a CI test for multiple-walkers metadynamics.

[giacomofiorin]

@jhenin Take a look if this may interfere with you ABF refactoring? Otherwise it should be just a quick merge IMO.

[jhenin]

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

[giacomofiorin]

Ok. Did I misinterpret this comment?

[jhenin]

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.

[giacomofiorin]

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)

[jhenin]

All good: no negative interactions with the ABF changeset.

[HanatoK]

I think it would be better to ask biases themselves if they need I/O instead of checking replica_share_freq() explicitly in colvarmodule.cpp. You could implement virtual bool need_io(step_number step); for each colvarbias class, and then call them in colvarmodule.cpp.

[HanatoK]

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.

[giacomofiorin]

Thanks for catching this!