From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 969031FF163 for ; Thu, 10 Oct 2024 16:49:11 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 36C901CA0F; Thu, 10 Oct 2024 16:49:40 +0200 (CEST) Date: Thu, 10 Oct 2024 16:49:00 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20240912143322.548839-1-c.ebner@proxmox.com> <20240912143322.548839-6-c.ebner@proxmox.com> In-Reply-To: <20240912143322.548839-6-c.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid) Message-Id: <1728553641.f5p3um0lbk.astroid@yuna.none> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.049 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH v3 proxmox-backup 05/33] client: backup writer: bundle upload stats counters X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" high level nit: I wonder whether it would make sense to refactor/merge BackupStats, UploadStats and UploadStatsCounter and add some `fn`s (e.g., always-inline fns to add to certain counters) and move the lot to their own module? e.g., you could then do something like counters.to_stats(csum, duration) to get the UploadStats, abstract away all the Atomics, get rid of a lot of Arc cloning boilerplate, .. On September 12, 2024 4:32 pm, Christian Ebner wrote: > In preparation for push support in sync jobs. > > Introduce `UploadStatsCounters` struct to hold the Arc clones of the > chunk upload statistics counters. By bundling them into the struct, > they can be passed as single function parameter when factoring out > the common stream future implementation in the subsequent > implementation of the chunk upload for push support in sync jobs. > > Signed-off-by: Christian Ebner > --- > changes since version 2: > - no changes > > pbs-client/src/backup_writer.rs | 52 ++++++++++++++++++--------------- > 1 file changed, 28 insertions(+), 24 deletions(-) > > diff --git a/pbs-client/src/backup_writer.rs b/pbs-client/src/backup_writer.rs > index d63c09b5a..34ac47beb 100644 > --- a/pbs-client/src/backup_writer.rs > +++ b/pbs-client/src/backup_writer.rs > @@ -65,6 +65,16 @@ struct UploadStats { > csum: [u8; 32], > } > > +struct UploadStatsCounters { > + injected_chunk_count: Arc, > + known_chunk_count: Arc, > + total_chunks: Arc, > + compressed_stream_len: Arc, > + injected_len: Arc, > + reused_len: Arc, > + stream_len: Arc, > +} > + > type UploadQueueSender = mpsc::Sender<(MergedChunkInfo, Option)>; > type UploadResultReceiver = oneshot::Receiver>; > > @@ -638,20 +648,23 @@ impl BackupWriter { > injections: Option>, > ) -> impl Future> { > let total_chunks = Arc::new(AtomicUsize::new(0)); > - let total_chunks2 = total_chunks.clone(); > let known_chunk_count = Arc::new(AtomicUsize::new(0)); > - let known_chunk_count2 = known_chunk_count.clone(); > let injected_chunk_count = Arc::new(AtomicUsize::new(0)); > - let injected_chunk_count2 = injected_chunk_count.clone(); > > let stream_len = Arc::new(AtomicUsize::new(0)); > - let stream_len2 = stream_len.clone(); > let compressed_stream_len = Arc::new(AtomicU64::new(0)); > - let compressed_stream_len2 = compressed_stream_len.clone(); > let reused_len = Arc::new(AtomicUsize::new(0)); > - let reused_len2 = reused_len.clone(); > let injected_len = Arc::new(AtomicUsize::new(0)); > - let injected_len2 = injected_len.clone(); > + > + let counters = UploadStatsCounters { > + injected_chunk_count: injected_chunk_count.clone(), > + known_chunk_count: known_chunk_count.clone(), > + total_chunks: total_chunks.clone(), > + compressed_stream_len: compressed_stream_len.clone(), > + injected_len: injected_len.clone(), > + reused_len: reused_len.clone(), > + stream_len: stream_len.clone(), > + }; > > let append_chunk_path = format!("{}_index", prefix); > let upload_chunk_path = format!("{}_chunk", prefix); > @@ -794,27 +807,18 @@ impl BackupWriter { > }) > .then(move |result| async move { upload_result.await?.and(result) }.boxed()) > .and_then(move |_| { > - let duration = start_time.elapsed(); > - let chunk_count = total_chunks2.load(Ordering::SeqCst); > - let chunk_reused = known_chunk_count2.load(Ordering::SeqCst); > - let chunk_injected = injected_chunk_count2.load(Ordering::SeqCst); > - let size = stream_len2.load(Ordering::SeqCst); > - let size_reused = reused_len2.load(Ordering::SeqCst); > - let size_injected = injected_len2.load(Ordering::SeqCst); > - let size_compressed = compressed_stream_len2.load(Ordering::SeqCst) as usize; > - > let mut guard = index_csum_2.lock().unwrap(); > let csum = guard.take().unwrap().finish(); > > futures::future::ok(UploadStats { > - chunk_count, > - chunk_reused, > - chunk_injected, > - size, > - size_reused, > - size_injected, > - size_compressed, > - duration, > + chunk_count: counters.total_chunks.load(Ordering::SeqCst), > + chunk_reused: counters.known_chunk_count.load(Ordering::SeqCst), > + chunk_injected: counters.injected_chunk_count.load(Ordering::SeqCst), > + size: counters.stream_len.load(Ordering::SeqCst), > + size_reused: counters.reused_len.load(Ordering::SeqCst), > + size_injected: counters.injected_len.load(Ordering::SeqCst), > + size_compressed: counters.compressed_stream_len.load(Ordering::SeqCst) as usize, > + duration: start_time.elapsed(), > csum, > }) > }) > -- > 2.39.2 > > > > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel > > > _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel