On 23/02/2024 11:35 am, Edwin Torok wrote: >> On 31 Jan 2024, at 16:27, Edwin Torok <edwin.to...@cloud.com> wrote: >>> On 31 Jan 2024, at 11:17, Christian Lindig >>> <christian.lin...@cloud.com> wrote: >>>> On 31 Jan 2024, at 10:52, Edwin Török <edwin.to...@cloud.com> wrote: >>>> >>>> Now that we no longer have a hashtable inside we can make Quota.t pure, >>>> and push the mutable update to its callers. >>>> Store.t already had a mutable Quota.t field. >>>> >>>> No functional change. >>> >>> Acked-by: Christian Lindig <christian.lin...@cloud.com> >>> >>> This is shifting copying working to GC work, at least potentially. I >>> would agree that this is a good trade-off and the code looks correct >>> to me. But I think we should see more testing and benchmarking >>> before merging this unless we are fine merging speculative improvements. >>> >>> — C >>> >>> >> >> >> I’ve written this [1] microbenchmark which creates ~300_000 entries >> in xenstore, assigns quota to 1000 domains and then measure how long >> it takes to list xenstore >> (It doesn’t matter whether these domains exist or not). >> It shows a measurable improvement with this patch, once I’ve run a >> more realistic test on the original machine with 1000 real VMs I’ll >> post those results too: > > The machine that can run this test is offline now due to a lab move, > but I managed to get this data before it went away, and I think this > patch series is ready to be committed. > > Flamegraph without my changes, where Hashtbl.copy takes up a > significant amount of oxenstored > time: > https://cdn.jsdelivr.net/gh/edwintorok/xen@oxenstored-coverletter/docs/original.svg?x=153.0&y=1269 > <https://cdn.jsdelivr.net/gh/edwintorok/xen@oxenstored-coverletter/docs/original.svg?x=153.0&y=1269> > Flamegraph with this patch series, where Hashtbl.copy does not show up > at > all: > https://cdn.jsdelivr.net/gh/edwintorok/xen@oxenstored-coverletter/docs/oxenstored_no_hashtbl_copy.svg?x=294.3&y=1301 > <https://cdn.jsdelivr.net/gh/edwintorok/xen@oxenstored-coverletter/docs/oxenstored_no_hashtbl_copy.svg?x=294.3&y=1301> > (There are of course still hashtbl in the flame graph, due to the > well-known inefficient poll_select implementation, and we see hashtbl > iteration as a parent caller, which is fine) > > IMHO this means the patch series is a worthwhile improvement: it > removes a codepath that was previously a hotspot completely from > oxenstored.
Agreed. I'll queue this series in due course. ~Andrew
