robur-coop / albatross

Albatross: orchestrate and manage MirageOS unikernels with Solo5
ISC License
141 stars 17 forks source link

Refactor albatross stats stubs #116

Closed reynir closed 1 year ago

reynir commented 2 years ago

In #115 an exception from List.combine is raised. With git grep I found two sites that call List.combine. One place it's called with taps and vm.config.bridges. This should be fine since the former is constructed from the latter. The other place is in albatross_stats_pure.ml in order to associate a statistic / counter value with a string description. Previously, the statistics description strings was fetched once when opening the first VM. From what I could gather the number of VM statistic counters depends on the number of CPUs assigned, so my hypothesis is the number of counters for the first VM may have differed from a later VM.

This patch makes vmmanage_vmmapi_stats return (string * int64) list instead of a int64 list where the string is the description fetched with vm_get_stat_desc at each vmmanage_vmmapi_stats call. This may be less efficient.

Ideally, we would somehow verify my hypothesis, and maybe benchmark the performance impact of this patch.

hannesm commented 2 years ago

Thanks, I thought maybe instead of let descr = ref [] we could use let descr = lazy (wrap vmmapi_statnames ctx), and apply Lazy.force. This way, ti would be evaluated only once, and the Lazy module would take care that it is good.

hannesm commented 2 years ago

This looks fine, I pushed a commit that guard the statistics gathering of bhyve debug counters optional (via a flag passed to albatross-stats). WDYT?

reynir commented 2 years ago

Nice, it looks good to me