ac_log_reuse.patch

diff --git a/components/ac_log/src/lib.rs b/components/ac_log/src/lib.rs
index 9dc1669..a10c100 100644
--- a/components/ac_log/src/lib.rs
+++ b/components/ac_log/src/lib.rs
@@ -45,17 +45,20 @@
 //! Rust and the JVM in this manner.
 
 use std::{
-    ffi::CString,
     os::raw::c_char,
     sync::{
         atomic::{AtomicBool, Ordering},
-        Arc,
-        Mutex,
-        mpsc::{SyncSender, sync_channel},
+        Arc, Mutex,
     },
     thread,
 };
 
+use crossbeam_channel::{bounded, Receiver, Sender};
+
+use crate::record::LogRecord;
+
+pub mod record;
+
 #[derive(Clone, Copy)]
 #[repr(i32)]
 pub enum LogLevel {
@@ -67,8 +70,6 @@ pub enum LogLevel {
     ERROR = 6,
 }
 
-
-
 impl From<log::Level> for LogLevel {
     fn from(l: log::Level) -> Self {
         match l {
@@ -81,44 +82,6 @@ impl From<log::Level> for LogLevel {
     }
 }
 
-// TODO: use serde to send this to the other thread as bincode or something,
-// rather than allocating all these strings for every message.
-struct LogRecord {
-    level: LogLevel,
-    tag: Option<CString>,
-    message: CString,
-}
-
-fn string_to_cstring_lossy(s: String) -> CString {
-    let mut bytes = s.into_bytes();
-    for byte in bytes.iter_mut() {
-        if *byte == 0 {
-            *byte = b'?';
-        }
-    }
-    CString::new(bytes).expect("Bug in string_to_cstring_lossy!")
-}
-
-impl<'a, 'b> From<&'b log::Record<'a>> for LogRecord {
-    // XXX important! Don't log in this function!
-    fn from(r: &'b log::Record<'a>) -> Self {
-        let message = match (r.line(), r.file()) {
-            (Some(line), Some(file)) => format!("{}@{}: {}", file, line, r.args()),
-            (None, Some(file)) => format!("{}: {}", file, r.args()),
-            // The (Some(line), None) case is pointless
-            (_, None) => format!("{}", r.args()),
-        };
-
-        Self {
-            level: r.level().into(),
-            tag: r
-                .module_path()
-                .and_then(|mp| CString::new(mp.to_owned()).ok()),
-            message: string_to_cstring_lossy(message),
-        }
-    }
-}
-
 /// Type of the log callback provided to us by java.
 /// Takes the following arguments:
 ///
@@ -140,14 +103,18 @@ pub struct LogAdapterState {
     // prefix with _ to shut rust up about it being unused.
     handle: Option<std::thread::JoinHandle<()>>,
     stopped: Arc<Mutex<bool>>,
-    sender: SyncSender<LogMessage>,
+    sender: Sender<LogMessage>,
 }
 
 pub struct LogSink {
-    sender: SyncSender<LogMessage>,
+    sender: Sender<LogMessage>,
     // Used locally for preventing unnecessary work after the `sender`
-    // is closed. Not shared. Not required for correctness.
+    // is closed. Not shared. Not required for correctness, just lets us avoid
+    // serializing a bunch of records when we're certainly disabled.
     disabled: AtomicBool,
+    // Optimization: the callback thread sends us the records when it's done
+    // with them so that we can reuse their memory.
+    free_list: Receiver<LogRecord>,
 }
 
 impl log::Log for LogSink {
@@ -158,16 +125,22 @@ impl log::Log for LogSink {
 
     fn flush(&self) {}
     fn log(&self, record: &log::Record) {
-        // Important: we check stopped before writing, which means
-        // it must be set before
+        // SeqCst is stronger than we need but whatever.
         if self.disabled.load(Ordering::SeqCst) {
             // Note: `enabled` is not automatically called.
             return;
         }
+
+        let mut rec = self.free_list.try_recv().unwrap_or_default();
+        rec.set(record);
         // Either the queue is full, or the receiver is closed.
         // In either case, we want to stop all logging immediately.
-        if self.sender.try_send(LogMessage::Record(record.into())).is_err() {
+        if self.sender.try_send(LogMessage::Record(rec)).is_err() {
+            // SeqCst is stronger than we need here too, but whatever.
             self.disabled.store(true, Ordering::SeqCst);
+            // Clear anything remaining out of the free list so that it
+            // doesn't stick around foreve, ignoring errors.
+            while let Ok(_) = self.free_list.try_recv() {}
         }
     }
 }
@@ -178,7 +151,8 @@ impl LogAdapterState {
         // where `stopped` gets set by another thread between when we read it and
         // when we call the callback. This way, they'll block.
         let stopped = Arc::new(Mutex::new(false));
-        let (message_sender, message_recv) = sync_channel(4096);
+        let (message_sender, message_recv) = bounded(4096);
+        let (free_list_send, free_list_recv) = bounded(128);
         let handle = {
             let stopped = stopped.clone();
             thread::spawn(move || {
@@ -186,22 +160,27 @@ impl LogAdapterState {
                 // which probably can't happen since the sender owned by the
                 // logger will never get dropped), or if we get `LogMessage::Stop`,
                 // which means we should stop processing.
-                while let Ok(LogMessage::Record(record)) = message_recv.recv() {
-                    let LogRecord { tag, level, message } = record;
-                    let tag_ptr = tag.as_ref()
-                        .map(|s| s.as_ptr())
-                        .unwrap_or_else(std::ptr::null);
-                    let msg_ptr = message.as_ptr();
-
-                    let mut stop_guard = stopped.lock().unwrap();
-                    if *stop_guard {
-                        return;
-                    }
-                    let keep_going = callback(level, tag_ptr, msg_ptr);
-                    if keep_going == 0 {
-                        *stop_guard = true;
-                        return;
+                while let Ok(LogMessage::Record(mut record)) = message_recv.recv() {
+                    {
+                        let mut stop_guard = stopped.lock().unwrap();
+                        if *stop_guard {
+                            return;
+                        }
+                        let keep_going = {
+                            let ptrs = record.get_ptrs();
+                            callback(record.level, ptrs.tag, ptrs.message)
+                        };
+                        if keep_going == 0 {
+                            // Nobody observes this write at the moment, but
+                            // it seems like it would be confusing us to stop
+                            // without setting it.
+                            *stop_guard = true;
+                            return;
+                        }
                     }
+                    record.clear();
+                    // We don't care whether or not the free list is full.
+                    let _ = free_list_send.try_send(record);
                 }
             })
         };
@@ -209,6 +188,7 @@ impl LogAdapterState {
         let sink = LogSink {
             sender: message_sender.clone(),
             disabled: AtomicBool::new(false),
+            free_list: free_list_recv,
         };
 
         log::set_max_level(log::LevelFilter::Info);
diff --git a/components/ac_log/src/record.rs b/components/ac_log/src/record.rs
new file mode 100644
index 0000000..b6edf0b
--- /dev/null
+++ b/components/ac_log/src/record.rs
@@ -0,0 +1,101 @@
+use std::ffi::CStr;
+use std::io::Write;
+use std::os::raw::c_char;
+
+use crate::LogLevel;
+
+pub struct LogRecord {
+    pub level: LogLevel,
+    // Vecs and not CStrings so that we can reuse their memory.
+    tag: Vec<u8>,
+    message: Vec<u8>,
+}
+
+impl Default for LogRecord {
+    #[inline]
+    fn default() -> Self {
+        LogRecord {
+            level: LogLevel::VERBOSE,
+            tag: Vec::new(),
+            message: Vec::new(),
+        }
+    }
+}
+
+// Used to make it clear which logs are from native rust code.
+const GLOBAL_TAG_PREFIX: &'static str = "rust#";
+
+pub struct LogRecordPtrs<'a> {
+    pub tag: *const c_char,
+    pub message: *const c_char,
+    // Tied to the lifetime of LogRecord to help prevent use-after-free (actually preventing it seems impossible, though).
+    _marker: std::marker::PhantomData<&'a str>,
+}
+
+fn nul_terminate(bytes: &mut Vec<u8>) {
+    // Ensure there are no nuls present currently
+    for byte in bytes.iter_mut() {
+        if *byte == 0 {
+            *byte = b'?';
+        }
+    }
+    // Add a trailing one
+    bytes.push(0);
+}
+
+impl LogRecord {
+    #[inline]
+    pub fn clear(&mut self) {
+        self.level = LogLevel::VERBOSE;
+        self.tag.clear();
+        self.message.clear();
+    }
+
+    pub fn set(&mut self, record: &log::Record) {
+        // XXX important! Don't log in this function!
+        self.clear();
+        let tag = record.module_path().unwrap_or("<unknown>");
+        self.tag.reserve(GLOBAL_TAG_PREFIX.len() + tag.len() + 1);
+        self.tag.extend(GLOBAL_TAG_PREFIX.as_bytes());
+        self.tag.extend(tag.as_bytes());
+        nul_terminate(&mut self.tag);
+
+        // We really have no clue how long record.args could be, and this is
+        // a good upper bound that doesnt' use too much memory
+        self.message.reserve(128);
+        // writing to a vec is infallible, and honestly we don't care that much if it fails anyway.
+        let _ = match (record.line(), record.file()) {
+            (Some(line), Some(file)) => {
+                write!(self.message, "{}@{}: {}", file, line, record.args())
+            }
+            (None, Some(file)) => write!(self.message, "{}: {}", file, record.args()),
+            // The (Some(line), None) case is pointless
+            (_, None) => write!(self.message, "{}", record.args()),
+        };
+        nul_terminate(&mut self.message);
+    }
+
+    pub fn get_ptrs(&self) -> LogRecordPtrs {
+        // These unwraps should never trigger unless there's a bug in nul_terminate.
+        // We use CStr to verify this, however.
+        let tag_ptr = CStr::from_bytes_with_nul(&self.tag).unwrap().as_ptr();
+        let msg_ptr = CStr::from_bytes_with_nul(&self.message).unwrap().as_ptr();
+
+        LogRecordPtrs {
+            tag: tag_ptr,
+            message: msg_ptr,
+            _marker: std::marker::PhantomData,
+        }
+    }
+}
+
+impl<'a, 'b> From<&'b log::Record<'a>> for LogRecord {
+    // XXX important! Don't log in this function!
+    fn from(r: &'b log::Record<'a>) -> Self {
+        let mut res = LogRecord::default();
+        res.set(r);
+        res
+    }
+}