don't Copy() self on Merge()

[rade]

Merge() is always returning a copy, so there is no need to Copy() struct fields first before calling Merge() on them.

This reduces GC pressure (#1010) and helps overall app performance (#1457).

[rade]

@2opremio I think my interpretation of the intended (and actual) Merge() semantics is correct, but I defer to your superior knowledge of scope internals.

I have tested this by scope launching the app on my desktop (~300 processes, though few with connections), while having ~20 connected containers, pointing my browser at the containers view, and monitoring the app's CPU usage with pidstat. This showed a reduction of about 20%.

[rade]

Nodes, Metrics, EdgeMetadata, MetadataTemplates could potentially also benefit from the same treatment, but are less straightforward than Report and Node.

[rade]

Is Merge() required to create deep copies or is it simply required not to modify self or other?

My code in this PR, in common with a bunch of other the Merge() code I have come across, meets the former criteria. But there is also code which doesn't do that. For example Nodes.Merge() only deep-copies self but not other (in particular elements from other that are not present in self are only shallow-copied), and so only meets the latter criteria.

If only the latter criteria is required, a whole bunch of easy, high-value optimisations are possible.

[rade]

Is Merge() required to create deep copies or is it simply required not to modify self or other?

@paulbellamy says it's the second. Which is just as well since otherwise some of the existing code would be wrong :)

[2opremio]

Thanks for this @rade! Calling Copy() inside Merge() at each level of the data structure means that objects were being unnecessarily copied depth - 1 times. (Not completely accurate since some levels like Topology didn't Copy()).

Is Merge() required to create deep copies or is it simply required not to modify self or other?

Unless I am missing something important, I believe Merge() is simply required not to modify self or other. In fact, as I mentioned above, Topology already does this.

Also, I believe that the benefit of this would be lower if all the data-structures where immutable internally (e.g. Metrics is still mutable) making copies essentially for free.

Nodes, Metrics, EdgeMetadata, MetadataTemplates could potentially also benefit from the same treatment, but are less straightforward than Report and Node.

I don't immediately see how you can avoid copies with things like Metrics (based on a mutable map).

[2opremio]

@paulbellamy says it's the second. Which is just as well since otherwise some of the existing code would be wrong :)

Great, then we agree.

[rade]

I don't immediately see how you can avoid copies with things like Metrics (based on a mutable map).

It's the deep copy that is avoidable.

[2opremio]

It's the deep copy that is avoidable.

But Metrics is a leaf of the data structure. The only optimization I see is using make to preallocate objects.

[rade]

But Metrics is a leaf of the data structure.

Sorry, I meant MetricTemplates.

[2opremio]

Sorry, I meant MetricTemplates.

Oh, I see. All good.

[2opremio]

Note: (discussed offline with @rade ) See if we can get rid of all the Copy() functions.

[2opremio]

LGTM