[BLOCKING] Per-node sync slows down distributed training with 'hist'

[CodingCat]

looks like latest branch has per-node sync again

xgboost/src/tree/updater_quantile_hist.cc

Lines 615 to 617 in 4ef6d21

	for (auto elem : nodes) {
	this->histred_.Allreduce(hist_[elem.nid].data(), hist_builder_.GetNumBins());
	}

[CodingCat]

and the grads is also synced per node

xgboost/src/tree/updater_quantile_hist.cc

Line 643 in 4ef6d21

this->histred_.Allreduce(&grad_stat, 1);

(only needed for root actually , check #4140)

[trivialfis]

Seems the optimization PR didn't make a clean rebase.

[CodingCat]

more discussion at #4716

[hcho3]

Re-opening this, since #4716 fixed correctness but did not address performance. @thvasilo points out that syncing for every nodes leads to significant slow-down due to extra communication.

[thvasilo]

Copying from the other issue: While the second per-node sync seems straightforward to fix (see this WIP) getting rid of the first loop could entail changes to how we build and use the hist_ container (of HistCollection type) which would require a lot of effort to untangle.

[hcho3]

@thvasilo Note that this issue is not present in release 0.90. I believe #4529 introduced it. A proper fix would need to first analyze #4529 in detail and selectively revert the parts that cause per-node sync. In 0.90, hist performs per-level sync (unless grow_policy is specifically set to lossguide).