CA-372059: refactor the type of host in squeeze.ml

[duobei]

use Set instead of List because set_difference implemented using List is quadratic cost

The flame graph before refactoring:

The flame graph after refactoring:

Special thanks to @psafont and @edwintorok for guides.

[minglumlu]

Could you please give me a pointer on how to read out the improvements from the flame graph?
Both of them were collected in booting up 1000 VMs?
I think maybe another test case is good to test against set_difference a b. E.g. both a and b are of almost same and big size. Only booting up 1000 VMs may not be enough to stress this function.

[duobei]

Could you please give me a pointer on how to read out the improvements from the flame graph? Both of them were collected in booting up 1000 VMs? I think maybe another test case is good to test against set_difference a b. E.g. both a and b are of almost same and big size. Only booting up 1000 VMs may not be enough to stress this function.

I ran the load testing by booting up 2048 VMs:

#! /bin/bash
 
set -e
D=/boot/guest
 
mkdir -p $D
cd $D
wget https://github.com/lindig/xen-test-vm/releases/download/0.1.17/test-vm.xen.gz
UUID=$(xe vm-create name-label=minion)
xe vm-param-set PV-kernel=$D/test-vm.xen.gz uuid=$UUID
xe vm-memory-limits-set uuid=$UUID dynamic-max=16777216 dynamic-min=16777216 static-max=16777216 static-min=16777216
seq 1 2048 | xargs -P16 -n1 -I% xe vm-clone new-name-description=% new-name-label=% vm=minion
xe vm-list power-state=halted --minimal | tr , '\n' | xargs -P16 -n1 -I% sh -c "echo pausing % && xe vm-start paused=true uuid=%"

then generate the flame graph in 10 minutes :
perf record -F 97 -g --call-graph dwarf -p $SQUEEZED_PID -- sleep 600 && perf script > perf.script

I have sent files of raw flame graph svg to you by slack. Opening the flame graph in web browser could get more information.

[psafont]

I have sent files of raw flame graph svg to you by slack. Opening the flame graph in web browser could get more information.

The file in github can be downloaded and inspected. I don't know why opening the link directly doesn't allow for inspection.

[duobei]

Could you please give me a pointer on how to read out the improvements from the flame graph? Both of them were collected in booting up 1000 VMs? I think maybe another test case is good to test against set_difference a b. E.g. both a and b are of almost same and big size. Only booting up 1000 VMs may not be enough to stress this function.

For more inspection, I generated two differential flame graphs:

1. the refactored differ from before:
   
2. the before differ from refactored:
   

In a differential flame graph, the red and blue areas represent the differences between two flame graphs. The color scheme is used to indicate the direction and relative magnitude of the change between the two datasets.

Red: The red areas in the differential flame graph represent an increase or growth in the performance metric being visualized. The darker the red, the larger the increase in the metric for the specific frame in the stack. This indicates that the performance metric for those functions or code paths has increased in the second dataset compared to the first.

Blue: Conversely, the blue areas in the differential flame graph represent a decrease or reduction in the performance metric. The darker the blue, the larger the decrease in the metric for the specific frame in the stack. This indicates that the performance metric for those functions or code paths has decreased in the second dataset compared to the first.

The saturation of the colors is relative to the delta, so dark red or dark blue indicates a significant change, while very light frames can be ignored as they represent minimal changes.

See more in https://www.brendangregg.com/blog/2014-10-31/cpi-flame-graphs.html.

[psafont]

The high towers in the map are a result of not having tail-recursive maps. The stacks can be squashed by piping the perf data through scripts in FlameGraph like so:

curl sftp://xenserver_HOST/root/perf.script | FlameGraph/stackcollapse-perf.pl --addrs | FlameGraph/stackcollapse-recursive.pl | FlameGraph/flamegraph.pl > perf-new.svg && open perf-new.svg

[psafont]

Some of this changes in the interface will need deletion from the module, as it's unused.

[psafont]

Could you squash the last 3 commits into one?

[duobei]

Could you squash the last 3 commits into one?

Yes, of course. Done.

[psafont]

The changes themselves look generally good to me.
The only doubt I have are the 2 folds that revert the order of some remaining list.
Have these changes passed specific ballooning tests?

[duobei]

The changes themselves look generally good to me. The only doubt I have are the 2 folds that revert the order of some remaining list. Have these changes passed specific ballooning tests?

All the tests in squeeze_test.ml has passed. There may indeed be an issue with the order. I'll check that tomorrow.

[psafont]

I was referring to whole-product tests using the internal framework ;)

[psafont]

Code looks good to me, the last commit passed the DMC e2e tests in job 194022

[minglumlu]

Thanks. Now I understand the 2nd flamegraph the before differ from refactored shows the key difference in the test: the List.mem invoked in update_cooperative_table contributes to the majority of delta (dark red). This should be a result of the elimination of set_difference in this PR (List.mem -> Set.diff).
It would be nice to separate the tests to show the improvements frommap|iter -> fold and List.mem -> Set.diff separately. It's clear for List.mem -> Set.diff in the flamegraph. But it's not clear to me for the improvement resulted by map|iter -> fold yet. If there is no quadratic cost in iteration, both of them should be linear. Or I missed something again?

[minglumlu]

But anyway, the review and test have shown the correctness of the changes. Especially I believe these changes would result in much better performance when the a and b in set_difference a b are both of same order of size .

[github-actions]

pytype_reporter extracted 49 problem reports from pytype output.

You can check the results of the job here

[duobei]

It would be nice to separate the tests to show the improvements frommap|iter -> fold and List.mem -> Set.diff separately. It's clear for List.mem -> Set.diff in the flamegraph. But it's not clear to me for the improvement resulted by map|iter -> fold yet. If there is no quadratic cost in iteration, both of them should be linear. Or I missed something again?

There may be no iter -> fold. There are indeed many map -> fold.

In fact, we can't use Set.map as List.map. If we want to use Set.map, we only can use DomainSet.map that Set has specifically type.

The type of List.map is: ('a -> 'b) -> 'a list -> 'b list.
The type of DomainSet.map is: (elt -> elt) -> DomainSet.t -> DomainSet.t.

So we can't use DomainSet.map to get other type except DomainSet.t.
For example, we can't use DomainSet.map to get IntSet.

Without redundant transformation, there is no alternative to use DomainSet.fold.
If we really want to use map, we can firstly transform set to list use Set.elements or Set.to_seq |> List.of_seq, Set.to_list occurs only since 5.1.
In addition, the returned list of Set.elements is sorted in increasing order with respect to the ordering Ord.compare, where Ord is the argument given to Set.Make. Set.of_seq iterates on the whole set, in ascending order.