Low hanging fruit perf changes (#1004)

* Some minor changes for some easy perf wins based on trace info

* Manually track buffer offsets in File writer

* Add metrics tests

* Call members from appropriate shared object

Source/santad/BUILD

@@ -503,6 +503,7 @@ objc_library(
     hdrs = ["Logs/EndpointSecurity/Writers/File.h"],
     deps = [
         ":EndpointSecurityWriter",
+        "//Source/common:BranchPrediction",
     ],
 )
 

Source/santad/EventProviders/EndpointSecurity/Message.h

@@ -47,9 +47,6 @@ class Message {
   std::shared_ptr<EndpointSecurityAPI> esapi_;
   const es_message_t* es_msg_;
 
-  mutable std::string pname_;
-  mutable std::string parent_pname_;
-
   std::string GetProcessName(pid_t pid) const;
 };
 

Source/santad/EventProviders/EndpointSecurity/Message.mm

@@ -22,7 +22,7 @@
 namespace santa::santad::event_providers::endpoint_security {
 
 Message::Message(std::shared_ptr<EndpointSecurityAPI> esapi, const es_message_t *es_msg)
-    : esapi_(esapi), es_msg_(es_msg) {
+    : esapi_(std::move(esapi)), es_msg_(es_msg) {
   esapi_->RetainMessage(es_msg);
 }
 
@@ -45,10 +45,7 @@
 }
 
 std::string Message::ParentProcessName() const {
-  if (parent_pname_.length() == 0) {
-    parent_pname_ = GetProcessName(es_msg_->process->ppid);
-  }
-  return parent_pname_;
+  return GetProcessName(es_msg_->process->ppid);
 }
 
 std::string Message::GetProcessName(pid_t pid) const {

Source/santad/EventProviders/SNTEndpointSecurityClient.mm

@@ -239,8 +239,9 @@ - (bool)respondToMessage:(const Message &)msg
 
 - (void)processEnrichedMessage:(std::shared_ptr<EnrichedMessage>)msg
                        handler:(void (^)(std::shared_ptr<EnrichedMessage>))messageHandler {
+  __block std::shared_ptr<EnrichedMessage> msgTmp = std::move(msg);
   dispatch_async(_notifyQueue, ^{
-    messageHandler(std::move(msg));
+    messageHandler(std::move(msgTmp));
   });
 }
 

Source/santad/Logs/EndpointSecurity/Writers/File.h

@@ -49,6 +49,10 @@ class File : public Writer, public std::enable_shared_from_this<File> {
   void OpenFileHandle();
   void WatchLogFile();
   void FlushBuffer();
+  bool ShouldFlush();
+
+  void EnsureCapacity(size_t additional_bytes);
+  void CopyData(const std::vector<uint8_t> &bytes);
 
   std::vector<uint8_t> buffer_;
   size_t batch_size_bytes_;
@@ -57,6 +61,12 @@ class File : public Writer, public std::enable_shared_from_this<File> {
   dispatch_source_t watch_source_;
   NSString *path_;
   NSFileHandle *file_handle_;
+
+  // Used to manually track the size of valid data in the `buffer_`.
+  // Benchmarking showed a large amount of time clearing the buffer after
+  // flushes, but that isn't very necessary. Instead we can manually track the
+  // `end` of the buffer and skip clearing the data.
+  size_t buffer_offset_ = 0;
 };
 
 }  // namespace santa::santad::logs::endpoint_security::writers

Source/santad/Logs/EndpointSecurity/Writers/File.mm

@@ -12,10 +12,12 @@
 ///    See the License for the specific language governing permissions and
 ///    limitations under the License.
 
-#import "Source/santad/Logs/EndpointSecurity/Writers/File.h"
+#include "Source/santad/Logs/EndpointSecurity/Writers/File.h"
 
 #include <memory>
 
+#include "Source/common/BranchPrediction.h"
+
 namespace santa::santad::logs::endpoint_security::writers {
 
 std::shared_ptr<File> File::Create(NSString *path, uint64_t flush_timeout_ms,
@@ -47,12 +49,12 @@
 
 File::File(NSString *path, size_t batch_size_bytes, size_t max_expected_write_size_bytes,
            dispatch_queue_t q, dispatch_source_t timer_source)
-    : batch_size_bytes_(batch_size_bytes),
+    : buffer_(batch_size_bytes + max_expected_write_size_bytes),
+      batch_size_bytes_(batch_size_bytes),
       q_(q),
       timer_source_(timer_source),
       watch_source_(nullptr) {
   path_ = path;
-  buffer_.reserve(batch_size_bytes + max_expected_write_size_bytes);
   OpenFileHandle();
 }
 
@@ -99,17 +101,40 @@
   dispatch_async(q_, ^{
     std::vector<uint8_t> moved_bytes = std::move(temp_bytes);
 
-    shared_this->buffer_.insert(shared_this->buffer_.end(), moved_bytes.begin(), moved_bytes.end());
-    if (shared_this->buffer_.size() >= batch_size_bytes_) {
+    shared_this->CopyData(moved_bytes);
+
+    if (shared_this->ShouldFlush()) {
       shared_this->FlushBuffer();
     }
   });
 }
 
+bool File::ShouldFlush() {
+  return buffer_offset_ >= batch_size_bytes_;
+}
+
+// IMPORTANT: Not thread safe.
+void File::EnsureCapacity(size_t additional_bytes) {
+  if ((buffer_offset_ + additional_bytes) > buffer_.capacity()) {
+    buffer_.resize(buffer_.capacity() * 2);
+  }
+}
+
+// IMPORTANT: Not thread safe.
+void File::CopyData(const std::vector<uint8_t> &bytes) {
+  EnsureCapacity(bytes.size());
+  std::copy(bytes.begin(), bytes.end(), buffer_.begin() + buffer_offset_);
+  buffer_offset_ += bytes.size();
+}
+
 // IMPORTANT: Not thread safe.
 void File::FlushBuffer() {
-  write(file_handle_.fileDescriptor, buffer_.data(), buffer_.size());
-  buffer_.clear();
+  if (likely(buffer_offset_ > 0)) {
+    write(file_handle_.fileDescriptor, buffer_.data(), buffer_offset_);
+
+    // After flushing, reset the offset back to 0
+    buffer_offset_ = 0;
+  }
 }
 
 }  // namespace santa::santad::logs::endpoint_security::writers

Source/santad/Logs/EndpointSecurity/Writers/FileTest.mm

@@ -29,11 +29,15 @@
   // Make constructors visible
   using File::File;
 
-  NSFileHandle *FileHandle() { return file_handle_; }
+  using File::CopyData;
+  using File::EnsureCapacity;
+  using File::ShouldFlush;
+  using File::WatchLogFile;
 
-  void BeginWatchingLogFile() { WatchLogFile(); }
+  NSFileHandle *FileHandle() { return file_handle_; }
 
-  size_t InternalBufferSize() { return buffer_.size(); }
+  size_t InternalBufferSize() { return buffer_offset_; }
+  size_t InternalBufferCapacity() { return buffer_.capacity(); }
 };
 
 }  // namespace santa::santad::logs::endpoint_security::writers
@@ -109,7 +113,7 @@ - (void)tearDown {
 
 - (void)testWatchLogFile {
   auto file = std::make_shared<FilePeer>(self.logPath, 100, 500, self.q, self.timer);
-  file->BeginWatchingLogFile();
+  file->WatchLogFile();
 
   // Constructing a File object will open the file at the given path
   struct stat wantSBOrig;
@@ -176,4 +180,89 @@ - (void)testWrite {
   XCTAssertEqual(0, file->InternalBufferSize());
 }
 
+- (void)testEnsureCapacity {
+  const size_t batchSize = 100;
+  auto file =
+    std::make_shared<FilePeer>(self.logPath, batchSize, batchSize * 2, self.q, self.timer);
+
+  // Initial capacity == (batch_size + max_expected_write_size)
+  const size_t initialCapacity = batchSize + (batchSize * 2);
+
+  // Buffer size should initially be 0 and capacity match initial expectations
+  XCTAssertEqual(file->InternalBufferSize(), 0);
+  XCTAssertEqual(file->InternalBufferCapacity(), initialCapacity);
+
+  file->EnsureCapacity(batchSize);
+
+  // No data was written, so size is still 0
+  XCTAssertEqual(file->InternalBufferSize(), 0);
+
+  // Capacity should be unchanged because the amount ensured didn't exceed
+  // the initial amount
+  XCTAssertEqual(file->InternalBufferCapacity(), initialCapacity);
+
+  file->EnsureCapacity(initialCapacity + 100);
+
+  // No data was written, so size is still 0
+  XCTAssertEqual(file->InternalBufferSize(), 0);
+
+  // Capacity should be doubled since the amount ensured was greater than
+  // the previous capacity
+  XCTAssertEqual(file->InternalBufferCapacity(), initialCapacity * 2);
+}
+
+- (void)testCopyData {
+  const size_t batchSize = 100;
+  // Use a buffer to copy that's slightly larger than the batch size
+  std::vector<uint8_t> bytes(batchSize + 2, 'A');
+  auto file =
+    std::make_shared<FilePeer>(self.logPath, batchSize, batchSize * 2, self.q, self.timer);
+
+  // Initial capacity == (batch_size + max_expected_write_size)
+  const size_t initialCapacity = batchSize + (batchSize * 2);
+
+  // Buffer size should initially be 0 and capacity match initial expectations
+  XCTAssertEqual(file->InternalBufferSize(), 0);
+  XCTAssertEqual(file->InternalBufferCapacity(), initialCapacity);
+
+  file->CopyData(bytes);
+
+  // After a copy, buffer size should match copied data size
+  XCTAssertEqual(file->InternalBufferSize(), bytes.size());
+
+  // Do a couple more copies that should require the buffer to grow and then
+  // confirm the size/capacity still matches expectations
+  file->CopyData(bytes);
+  file->CopyData(bytes);
+  XCTAssertEqual(file->InternalBufferSize(), bytes.size() * 3);
+  XCTAssertEqual(file->InternalBufferCapacity(), initialCapacity * 2);
+}
+
+- (void)testShouldFlush {
+  const size_t batchSize = 100;
+  const size_t halfBatch = batchSize / 2;
+  std::vector<uint8_t> bytes(halfBatch);
+  auto file =
+    std::make_shared<FilePeer>(self.logPath, batchSize, batchSize * 2, self.q, self.timer);
+
+  // Should never want to flush with no data in the buffer
+  XCTAssertFalse(file->ShouldFlush());
+
+  // Copy some data into the buffer
+  file->CopyData(bytes);
+
+  // Buffer size should be updated
+  XCTAssertEqual(file->InternalBufferSize(), bytes.size());
+
+  // Still shouldn't flush below the batch size
+  XCTAssertFalse(file->ShouldFlush());
+
+  // Exceed the batch size
+  file->CopyData(bytes);
+  file->CopyData(bytes);
+
+  // Should want to flush now that the batch size is exceeded
+  XCTAssertTrue(file->ShouldFlush());
+}
+
 @end

Source/santad/Metrics.h

@@ -20,6 +20,7 @@
 #import <MOLXPCConnection/MOLXPCConnection.h>
 #include <dispatch/dispatch.h>
 
+#include <map>
 #include <memory>
 
 #import "Source/common/SNTMetricSet.h"
@@ -43,6 +44,9 @@ enum class Processor {
   kFileAccessAuthorizer,
 };
 
+using EventCountTuple = std::tuple<Processor, es_event_type_t, EventDisposition>;
+using EventTimesTuple = std::tuple<Processor, es_event_type_t>;
+
 class Metrics : public std::enable_shared_from_this<Metrics> {
  public:
   static std::shared_ptr<Metrics> Create(SNTMetricSet *metricSet, uint64_t interval);
@@ -58,6 +62,8 @@ class Metrics : public std::enable_shared_from_this<Metrics> {
   void StopPoll();
   void SetInterval(uint64_t interval);
 
+  void FlushMetrics();
+
   void SetEventMetrics(Processor processor, es_event_type_t event_type,
                        EventDisposition disposition, int64_t nanos);
 
@@ -79,6 +85,10 @@ class Metrics : public std::enable_shared_from_this<Metrics> {
   // Separate queue used for setting event metrics
   // Mitigate issues where capturing metrics could be blocked on exporting
   dispatch_queue_t events_q_;
+
+  // Small caches for storing event metrics between metrics export operations
+  std::map<EventCountTuple, int64_t> event_counts_cache_;
+  std::map<EventTimesTuple, int64_t> event_times_cache_;
 };
 
 }  // namespace santa::santad

Source/santad/Metrics.mm

@@ -55,7 +55,7 @@
 
 namespace santa::santad {
 
-const NSString *ProcessorToString(Processor processor) {
+NSString *const ProcessorToString(Processor processor) {
   switch (processor) {
     case Processor::kAuthorizer: return kProcessorAuthorizer;
     case Processor::kDeviceManager: return kProcessorDeviceManager;
@@ -68,7 +68,7 @@
   }
 }
 
-const NSString *EventTypeToString(es_event_type_t eventType) {
+NSString *const EventTypeToString(es_event_type_t eventType) {
   switch (eventType) {
     case ES_EVENT_TYPE_AUTH_CLONE: return kEventTypeAuthClone;
     case ES_EVENT_TYPE_AUTH_COPYFILE: return kEventTypeAuthCopyfile;
@@ -98,7 +98,7 @@
   }
 }
 
-const NSString *EventDispositionToString(EventDisposition d) {
+NSString *const EventDispositionToString(EventDisposition d) {
   switch (d) {
     case EventDisposition::kDropped: return kEventDispositionDropped;
     case EventDisposition::kProcessed: return kEventDispositionProcessed;
@@ -137,6 +137,8 @@
       return;
     }
 
+    shared_metrics->FlushMetrics();
+
     [[shared_metrics->metrics_connection_ remoteObjectProxy]
       exportForMonitoring:[metricSet export]];
   });
@@ -180,6 +182,30 @@
   metrics_connection_ = metrics_connection;
 }
 
+void Metrics::FlushMetrics() {
+  dispatch_sync(events_q_, ^{
+    for (const auto &kv : event_counts_cache_) {
+      NSString *processorName = ProcessorToString(std::get<Processor>(kv.first));
+      NSString *eventName = EventTypeToString(std::get<es_event_type_t>(kv.first));
+      NSString *dispositionName = EventDispositionToString(std::get<EventDisposition>(kv.first));
+
+      [event_counts_ incrementBy:kv.second
+                  forFieldValues:@[ processorName, eventName, dispositionName ]];
+    }
+
+    for (const auto &kv : event_times_cache_) {
+      NSString *processorName = ProcessorToString(std::get<Processor>(kv.first));
+      NSString *eventName = EventTypeToString(std::get<es_event_type_t>(kv.first));
+
+      [event_processing_times_ set:kv.second forFieldValues:@[ processorName, eventName ]];
+    }
+
+    // Reset the maps so the next cycle begins with a clean state
+    event_counts_cache_ = {};
+    event_times_cache_ = {};
+  });
+}
+
 void Metrics::SetInterval(uint64_t interval) {
   dispatch_sync(q_, ^{
     LOGI(@"Setting metrics interval to %llu (exporting? %s)", interval, running_ ? "YES" : "NO");
@@ -220,13 +246,9 @@
 
 void Metrics::SetEventMetrics(Processor processor, es_event_type_t event_type,
                               EventDisposition event_disposition, int64_t nanos) {
-  dispatch_async(events_q_, ^{
-    NSString *processorName = (NSString *)ProcessorToString(processor);
-    NSString *eventName = (NSString *)EventTypeToString(event_type);
-    NSString *disposition = (NSString *)EventDispositionToString(event_disposition);
-
-    [event_counts_ incrementForFieldValues:@[ processorName, eventName, disposition ]];
-    [event_processing_times_ set:nanos forFieldValues:@[ processorName, eventName ]];
+  dispatch_sync(events_q_, ^{
+    event_counts_cache_[EventCountTuple{processor, event_type, event_disposition}]++;
+    event_times_cache_[EventTimesTuple{processor, event_type}] = nanos;
   });
 }