From 1c7b06c98895074ebc88557a1a632319a147b51d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arpad=20M=C3=BCller?= Date: Tue, 30 Jul 2024 09:59:15 +0200 Subject: [PATCH] Add metrics for input data considered and taken for compression (#8522) If compression is enabled, we currently try compressing each image larger than a specific size and if the compressed version is smaller, we write that one, otherwise we use the uncompressed image. However, this might sometimes be a wasteful process, if there is a substantial amount of images that don't compress well. The compression metrics added in #8420 `pageserver_compression_image_in_bytes_total` and `pageserver_compression_image_out_bytes_total` are well designed for answering the question how space efficient the total compression process is end-to-end, which helps one to decide whether to enable it or not. To answer the question of how much waste there is in terms of trial compression, so CPU time, we add two metrics: * one about the images that have been trial-compressed (considered), and * one about the images where the compressed image has actually been written (chosen). There is different ways of weighting them, like for example one could look at the count, or the compressed data. But the main contributor to compression CPU usage is amount of data processed, so we weight the images by their *uncompressed* size. In other words, the two metrics are: * `pageserver_compression_image_in_bytes_considered` * `pageserver_compression_image_in_bytes_chosen` Part of #5431 --- pageserver/src/metrics.rs | 18 +++++++++- pageserver/src/tenant/blob_io.rs | 36 +++++++++++++------ .../src/tenant/storage_layer/delta_layer.rs | 2 +- .../src/tenant/storage_layer/image_layer.rs | 26 ++++++++++++-- 4 files changed, 68 insertions(+), 14 deletions(-) diff --git a/pageserver/src/metrics.rs b/pageserver/src/metrics.rs index 9aff5220f59e..ede6b41a7509 100644 --- a/pageserver/src/metrics.rs +++ b/pageserver/src/metrics.rs @@ -613,7 +613,23 @@ pub(crate) static CIRCUIT_BREAKERS_UNBROKEN: Lazy = Lazy::new(|| { pub(crate) static COMPRESSION_IMAGE_INPUT_BYTES: Lazy = Lazy::new(|| { register_int_counter!( "pageserver_compression_image_in_bytes_total", - "Size of uncompressed data written into image layers" + "Size of data written into image layers before compression" + ) + .expect("failed to define a metric") +}); + +pub(crate) static COMPRESSION_IMAGE_INPUT_BYTES_CONSIDERED: Lazy = Lazy::new(|| { + register_int_counter!( + "pageserver_compression_image_in_bytes_considered", + "Size of potentially compressible data written into image layers before compression" + ) + .expect("failed to define a metric") +}); + +pub(crate) static COMPRESSION_IMAGE_INPUT_BYTES_CHOSEN: Lazy = Lazy::new(|| { + register_int_counter!( + "pageserver_compression_image_in_bytes_chosen", + "Size of data whose compressed form was written into image layers" ) .expect("failed to define a metric") }); diff --git a/pageserver/src/tenant/blob_io.rs b/pageserver/src/tenant/blob_io.rs index 791eefebe989..8e9d349ca88c 100644 --- a/pageserver/src/tenant/blob_io.rs +++ b/pageserver/src/tenant/blob_io.rs @@ -28,6 +28,12 @@ use crate::virtual_file::VirtualFile; use std::cmp::min; use std::io::{Error, ErrorKind}; +#[derive(Copy, Clone, Debug)] +pub struct CompressionInfo { + pub written_compressed: bool, + pub compressed_size: Option, +} + impl<'a> BlockCursor<'a> { /// Read a blob into a new buffer. pub async fn read_blob( @@ -273,8 +279,10 @@ impl BlobWriter { srcbuf: B, ctx: &RequestContext, ) -> (B::Buf, Result) { - self.write_blob_maybe_compressed(srcbuf, ctx, ImageCompressionAlgorithm::Disabled) - .await + let (buf, res) = self + .write_blob_maybe_compressed(srcbuf, ctx, ImageCompressionAlgorithm::Disabled) + .await; + (buf, res.map(|(off, _compression_info)| off)) } /// Write a blob of data. Returns the offset that it was written to, @@ -284,8 +292,12 @@ impl BlobWriter { srcbuf: B, ctx: &RequestContext, algorithm: ImageCompressionAlgorithm, - ) -> (B::Buf, Result) { + ) -> (B::Buf, Result<(u64, CompressionInfo), Error>) { let offset = self.offset; + let mut compression_info = CompressionInfo { + written_compressed: false, + compressed_size: None, + }; let len = srcbuf.bytes_init(); @@ -328,7 +340,9 @@ impl BlobWriter { encoder.write_all(&slice[..]).await.unwrap(); encoder.shutdown().await.unwrap(); let compressed = encoder.into_inner(); + compression_info.compressed_size = Some(compressed.len()); if compressed.len() < len { + compression_info.written_compressed = true; let compressed_len = compressed.len(); compressed_buf = Some(compressed); (BYTE_ZSTD, compressed_len, slice.into_inner()) @@ -359,7 +373,7 @@ impl BlobWriter { } else { self.write_all(srcbuf, ctx).await }; - (srcbuf, res.map(|_| offset)) + (srcbuf, res.map(|_| (offset, compression_info))) } } @@ -416,12 +430,14 @@ pub(crate) mod tests { let mut wtr = BlobWriter::::new(file, 0); for blob in blobs.iter() { let (_, res) = if compression { - wtr.write_blob_maybe_compressed( - blob.clone(), - ctx, - ImageCompressionAlgorithm::Zstd { level: Some(1) }, - ) - .await + let res = wtr + .write_blob_maybe_compressed( + blob.clone(), + ctx, + ImageCompressionAlgorithm::Zstd { level: Some(1) }, + ) + .await; + (res.0, res.1.map(|(off, _)| off)) } else { wtr.write_blob(blob.clone(), ctx).await }; diff --git a/pageserver/src/tenant/storage_layer/delta_layer.rs b/pageserver/src/tenant/storage_layer/delta_layer.rs index 229d1e36082b..f9becf53ff02 100644 --- a/pageserver/src/tenant/storage_layer/delta_layer.rs +++ b/pageserver/src/tenant/storage_layer/delta_layer.rs @@ -467,7 +467,7 @@ impl DeltaLayerWriterInner { .write_blob_maybe_compressed(val, ctx, compression) .await; let off = match res { - Ok(off) => off, + Ok((off, _)) => off, Err(e) => return (val, Err(anyhow::anyhow!(e))), }; diff --git a/pageserver/src/tenant/storage_layer/image_layer.rs b/pageserver/src/tenant/storage_layer/image_layer.rs index 44ba68549059..08db27514a21 100644 --- a/pageserver/src/tenant/storage_layer/image_layer.rs +++ b/pageserver/src/tenant/storage_layer/image_layer.rs @@ -734,6 +734,14 @@ struct ImageLayerWriterInner { // Total uncompressed bytes passed into put_image uncompressed_bytes: u64, + // Like `uncompressed_bytes`, + // but only of images we might consider for compression + uncompressed_bytes_eligible: u64, + + // Like `uncompressed_bytes`, but only of images + // where we have chosen their compressed form + uncompressed_bytes_chosen: u64, + blob_writer: BlobWriter, tree: DiskBtreeBuilder, } @@ -790,6 +798,8 @@ impl ImageLayerWriterInner { tree: tree_builder, blob_writer, uncompressed_bytes: 0, + uncompressed_bytes_eligible: 0, + uncompressed_bytes_chosen: 0, }; Ok(writer) @@ -808,13 +818,22 @@ impl ImageLayerWriterInner { ) -> anyhow::Result<()> { ensure!(self.key_range.contains(&key)); let compression = self.conf.image_compression; - self.uncompressed_bytes += img.len() as u64; + let uncompressed_len = img.len() as u64; + self.uncompressed_bytes += uncompressed_len; let (_img, res) = self .blob_writer .write_blob_maybe_compressed(img, ctx, compression) .await; // TODO: re-use the buffer for `img` further upstack - let off = res?; + let (off, compression_info) = res?; + if compression_info.compressed_size.is_some() { + // The image has been considered for compression at least + self.uncompressed_bytes_eligible += uncompressed_len; + } + if compression_info.written_compressed { + // The image has been compressed + self.uncompressed_bytes_chosen += uncompressed_len; + } let mut keybuf: [u8; KEY_SIZE] = [0u8; KEY_SIZE]; key.write_to_byte_slice(&mut keybuf); @@ -837,6 +856,9 @@ impl ImageLayerWriterInner { // Calculate compression ratio let compressed_size = self.blob_writer.size() - PAGE_SZ as u64; // Subtract PAGE_SZ for header crate::metrics::COMPRESSION_IMAGE_INPUT_BYTES.inc_by(self.uncompressed_bytes); + crate::metrics::COMPRESSION_IMAGE_INPUT_BYTES_CONSIDERED + .inc_by(self.uncompressed_bytes_eligible); + crate::metrics::COMPRESSION_IMAGE_INPUT_BYTES_CHOSEN.inc_by(self.uncompressed_bytes_chosen); crate::metrics::COMPRESSION_IMAGE_OUTPUT_BYTES.inc_by(compressed_size); let mut file = self.blob_writer.into_inner();