Re: [PATCH v1 2/2] oxenstored: make Quota.t pure

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 23/02/2024 11:35 am, Edwin Torok wrote:
>> On 31 Jan 2024, at 16:27, Edwin Torok <edwin.torok@xxxxxxxxx> wrote:
>>> On 31 Jan 2024, at 11:17, Christian Lindig
>>> <christian.lindig@xxxxxxxxx> wrote:
>>>> On 31 Jan 2024, at 10:52, Edwin Török <edwin.torok@xxxxxxxxx> 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.lindig@xxxxxxxxx>
>>>
>>> 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