[pbs-devel] [PATCH v2 proxmox-backup 3/4] garbage collection: allow to keep track of already touched chunks

[Christian Ebner]

On 3/17/25 16:39, Christian Ebner wrote:
> On 3/17/25 15:55, Fabian Grünbichler wrote:
>>
>> this could avoid the memory allocation (and for bigger
>> indices/snapshots, probably multiple reallocations via the inserts) by
>> switching to `retain` (which gets a `&mut V` and can thus flip the value
>> of touched while filtering in a single pass).
>>
>> despite the performance warning about it visiting empty buckets as well,
>> this seems to be (slightly) faster for my test datastore (would be
>> interesting if your test cases agree?) when benchmarking with a warmed
>> up cache.
>>
>> I used the following on-top of your series (the shrink_to is to reduce
>> memory usage in case of outliers after they've been processed):
>>
>> ```
>> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/ 
>> datastore.rs
>> index a80343d9b..d3c3f831f 100644
>> --- a/pbs-datastore/src/datastore.rs
>> +++ b/pbs-datastore/src/datastore.rs
>> @@ -1650,13 +1650,12 @@ impl TouchedChunks {
>>       // Clear untouched chunks and reset the touched marker for others.
>>       fn reset(&mut self) {
>> -        let mut new_list = HashMap::new();
>> -        for (digest, touched) in self.list.drain() {
>> -            if touched {
>> -                new_list.insert(digest, false);
>> -            }
>> -        }
>> -        self.list = new_list;
>> +        self.list.retain(|_digest, touched| {
>> +            *touched = !*touched;
>> +            !*touched
>> +        });
>> +        let max_capacity = 
>> self.list.len().saturating_add(self.list.len() / 3);
>> +        self.list.shrink_to(max_capacity);
>>       }
>>       // Insert the digest in the list of touched chunks.
>> ```
>>
>> if the above is slower for your test inputs, then at least initializing
>> the second HashMap with some initial capacity (the len of the previous
>> list?) is probably sensible..
> 
> Okay, will check and incorporate these suggestions, thanks!

Did some testing on this, above changes do result in a slight improvement:

Without above I do get about 26s with 1147680 utimensat calls on a 
datastore located on spinnig disk, with it drops to about 24s (checked 
the same number of utimensat calls).

On the datastore located on SSD I get about 15s with 566341 utimensat 
calls without the modifications and about 14s with.

Note: These values are not comparable to the tests results listed in the 
coverletter of v2, as the datastore contents changed since.

So this helps also for my testcase. However, see below...

>>
>> alternatively, we could also explore (maybe as a follow-up to
>> immediately realize the performance gains we already know we get from
>> the current aproach?) some sort of LRU-based approach?
> 
> Yeah, had something like that in mind as well because of the recent work 
> on that. This would allow to better control the overall memory 
> requirements for the garbage collection task as well, as it then does 
> not depend on the index files. Keeping the cache capacity large is of 
> course a pre-condition for that.

I did test this as well storing up to 1024 * 1024 * 32 bytes in digests 
as keys in the LRU cache, and while there is no significant change in 
runtime (?, this needs some double checking) as compared to the patched 
version as shown above, the utimensat call count dropped to 1078946 for 
my particular test case on the spinning disk and 560833 on the SSD. 
Therefore I will further pursue this approach given the already 
mentioned advantages.

The question then remains, do we want to keep the changes to the image 
list iteration, or even drop that as well in favor of reduced complexity 
and slight increase in performance?

I do not see much gain from that anymore...