From bde2b637812c5018be0aaeef3c86fb2a5ea3adb5 Mon Sep 17 00:00:00 2001 From: Marcin Radomski Date: Tue, 4 Mar 2025 13:36:14 +0000 Subject: [PATCH 1/3] Avoid rewriting the preconfigured tag In case we already have a CString tag in Config, do not attempt to rewrite it to a temporary buffer, but use it directly. --- src/lib.rs | 45 ++++++++++++++++++++++----------------------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index a05d23d..b287a8d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -140,7 +140,13 @@ impl AndroidLogger { static ANDROID_LOGGER: OnceLock = OnceLock::new(); -// Maximum length of a tag that does not require allocation. +/// Maximum length of a tag that does not require allocation. +/// +/// Tags configured explicitly in [Config] will not cause an extra allocation. When the tag is +/// derived from the module path, paths longer than this limit will trigger an allocation for each +/// log statement. +/// +/// The terminating nullbyte does not count towards this limit. const LOGGING_TAG_MAX_LEN: usize = 127; const LOGGING_MSG_MAX_LEN: usize = 4000; @@ -162,32 +168,25 @@ impl Log for AndroidLogger { return; } - // tag longer than LOGGING_TAG_MAX_LEN causes allocation + // Temporary storage for null-terminating record.module_path() if it's needed. + // Tags too long to fit here cause allocation. let mut tag_bytes: [MaybeUninit; LOGGING_TAG_MAX_LEN + 1] = uninit_array(); + // In case we end up allocating, keep the CString alive. + let _owned_tag; let module_path = record.module_path().unwrap_or_default(); - // If no tag was specified, use module name - let custom_tag = &config.tag; - let tag = custom_tag - .as_ref() - .map(|s| s.as_bytes()) - .unwrap_or_else(|| module_path.as_bytes()); - - // In case we end up allocating, keep the CString alive. - let _owned_tag; - let tag = if tag.len() < tag_bytes.len() { - // truncate the tag here to fit into LOGGING_TAG_MAX_LEN - fill_tag_bytes(&mut tag_bytes, tag) + let tag: &CStr = if let Some(ref tag) = config.tag { + tag } else { - // Tag longer than available stack buffer; allocate. - // We're using either - // - CString::as_bytes on config.tag, or - // - str::as_bytes on record.module_path() - // Neither of those include the terminating nullbyte. - _owned_tag = CString::new(tag) - .expect("config.tag or record.module_path() should never contain nullbytes"); - _owned_tag.as_ref() + if module_path.len() < tag_bytes.len() { + fill_tag_bytes(&mut tag_bytes, module_path.as_bytes()) + } else { + // Tag longer than available stack buffer; allocate. + _owned_tag = CString::new(module_path.as_bytes()) + .expect("record.module_path() shouldn't contain nullbytes"); + _owned_tag.as_ref() + } }; // message must not exceed LOGGING_MSG_MAX_LEN @@ -196,7 +195,7 @@ impl Log for AndroidLogger { // If a custom tag is used, add the module path to the message. // Use PlatformLogWriter to output chunks if they exceed max size. - let _ = match (custom_tag, &config.custom_format) { + let _ = match (&config.tag, &config.custom_format) { (_, Some(format)) => format(&mut writer, record), (Some(_), _) => fmt::write( &mut writer, From 52885612df1912c3fada086f177ca3a72d4bc6eb Mon Sep 17 00:00:00 2001 From: Marcin Radomski Date: Wed, 5 Mar 2025 11:00:48 +0000 Subject: [PATCH 2/3] cargo clippy --fix --- src/lib.rs | 14 ++++++-------- src/platform_log_writer.rs | 4 ++-- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index b287a8d..07089b1 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -178,15 +178,13 @@ impl Log for AndroidLogger { let tag: &CStr = if let Some(ref tag) = config.tag { tag + } else if module_path.len() < tag_bytes.len() { + fill_tag_bytes(&mut tag_bytes, module_path.as_bytes()) } else { - if module_path.len() < tag_bytes.len() { - fill_tag_bytes(&mut tag_bytes, module_path.as_bytes()) - } else { - // Tag longer than available stack buffer; allocate. - _owned_tag = CString::new(module_path.as_bytes()) - .expect("record.module_path() shouldn't contain nullbytes"); - _owned_tag.as_ref() - } + // Tag longer than available stack buffer; allocate. + _owned_tag = CString::new(module_path.as_bytes()) + .expect("record.module_path() shouldn't contain nullbytes"); + _owned_tag.as_ref() }; // message must not exceed LOGGING_MSG_MAX_LEN diff --git a/src/platform_log_writer.rs b/src/platform_log_writer.rs index 85826ca..1b75877 100644 --- a/src/platform_log_writer.rs +++ b/src/platform_log_writer.rs @@ -191,7 +191,7 @@ pub mod tests { #[test] fn platform_log_writer_init_values() { - let tag = CStr::from_bytes_with_nul(b"tag\0").unwrap(); + let tag = c"tag"; let writer = PlatformLogWriter::new(None, Level::Warn, tag); @@ -301,7 +301,7 @@ pub mod tests { PlatformLogWriter::new( None, Level::Warn, - CStr::from_bytes_with_nul(b"tag\0").unwrap(), + c"tag", ) } } From 47488c95e345fe49b9c3c93e360397d16c68ca6a Mon Sep 17 00:00:00 2001 From: Marcin Radomski Date: Wed, 5 Mar 2025 12:33:19 +0000 Subject: [PATCH 3/3] Apply review suggestions - Remove unnecessary type annotation - Replace c"" with CStr::from_bytes_with_nul for compatibility with older Rust versions --- src/lib.rs | 2 +- src/platform_log_writer.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 07089b1..2d6b82b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -176,7 +176,7 @@ impl Log for AndroidLogger { let module_path = record.module_path().unwrap_or_default(); - let tag: &CStr = if let Some(ref tag) = config.tag { + let tag = if let Some(tag) = &config.tag { tag } else if module_path.len() < tag_bytes.len() { fill_tag_bytes(&mut tag_bytes, module_path.as_bytes()) diff --git a/src/platform_log_writer.rs b/src/platform_log_writer.rs index 1b75877..85826ca 100644 --- a/src/platform_log_writer.rs +++ b/src/platform_log_writer.rs @@ -191,7 +191,7 @@ pub mod tests { #[test] fn platform_log_writer_init_values() { - let tag = c"tag"; + let tag = CStr::from_bytes_with_nul(b"tag\0").unwrap(); let writer = PlatformLogWriter::new(None, Level::Warn, tag); @@ -301,7 +301,7 @@ pub mod tests { PlatformLogWriter::new( None, Level::Warn, - c"tag", + CStr::from_bytes_with_nul(b"tag\0").unwrap(), ) } }