From 8d8f637ee5a09ea37290120942ae902f8e68773d Mon Sep 17 00:00:00 2001
From: Yurii Zubrytskyi <zyy@google.com>
Date: Mon, 6 Apr 2020 19:35:33 -0700
Subject: [PATCH] [zip] Stop calculating crc if it's not checked

Crc calculation shows up in the profiler in 2-5% range, and is
never currently validated. Let's disable it for good.
For a well-compressible test data the difference is even nicer:

Benchmark                       Time          CPU        Iteration
------------------------------------------------------------------
ziparchive-benchmarks:
before:
  #ExtractEntry/2            1943244 ns    1926758 ns          375
  #ExtractEntry/16           1877295 ns    1867049 ns          375
  #ExtractEntry/1024         1888772 ns    1879976 ns          373
after:
  #ExtractEntry/2             817003 ns     812870 ns          874
  #ExtractEntry/16            814029 ns     809813 ns          875
  #ExtractEntry/1024          804904 ns     800972 ns          879

Bug: 153392568
Test: atest, manual
Change-Id: I917abecab01301f1d09a5bf3b542d24b3875e359
---
 libziparchive/zip_archive.cc            | 16 +++++++----
 libziparchive/zip_archive_benchmark.cpp | 35 +++++++++++++++++++++----
 2 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/libziparchive/zip_archive.cc b/libziparchive/zip_archive.cc
index 7658d5c2d..19f95d4a0 100644
--- a/libziparchive/zip_archive.cc
+++ b/libziparchive/zip_archive.cc
@@ -59,7 +59,7 @@
 
 // Used to turn on crc checks - verify that the content CRC matches the values
 // specified in the local file header and the central directory.
-static const bool kCrcChecksEnabled = false;
+static constexpr bool kCrcChecksEnabled = false;
 
 // The maximum number of bytes to scan backwards for the EOCD start.
 static const uint32_t kMaxEOCDSearch = kMaxCommentLen + sizeof(EocdRecord);
@@ -1314,11 +1314,15 @@ static int32_t CopyEntryToWriter(MappedZipFile& mapped_zip, const ZipEntry64* en
     if (!writer->Append(&buf[0], block_size)) {
       return kIoError;
     }
-    crc = crc32(crc, &buf[0], block_size);
+    if (crc_out) {
+      crc = crc32(crc, &buf[0], block_size);
+    }
     count += block_size;
   }
 
-  *crc_out = crc;
+  if (crc_out) {
+    *crc_out = crc;
+  }
 
   return 0;
 }
@@ -1331,9 +1335,11 @@ int32_t ExtractToWriter(ZipArchiveHandle handle, const ZipEntry64* entry,
   int32_t return_value = -1;
   uint64_t crc = 0;
   if (method == kCompressStored) {
-    return_value = CopyEntryToWriter(handle->mapped_zip, entry, writer, &crc);
+    return_value =
+        CopyEntryToWriter(handle->mapped_zip, entry, writer, kCrcChecksEnabled ? &crc : nullptr);
   } else if (method == kCompressDeflated) {
-    return_value = InflateEntryToWriter(handle->mapped_zip, entry, writer, &crc);
+    return_value =
+        InflateEntryToWriter(handle->mapped_zip, entry, writer, kCrcChecksEnabled ? &crc : nullptr);
   }
 
   if (!return_value && entry->has_data_descriptor) {
diff --git a/libziparchive/zip_archive_benchmark.cpp b/libziparchive/zip_archive_benchmark.cpp
index 09d3b8ad8..cfa5912a2 100644
--- a/libziparchive/zip_archive_benchmark.cpp
+++ b/libziparchive/zip_archive_benchmark.cpp
@@ -17,7 +17,6 @@
 #include <cstdio>
 #include <cstdlib>
 #include <cstring>
-#include <iostream>
 #include <string>
 #include <tuple>
 #include <vector>
@@ -28,17 +27,20 @@
 #include <ziparchive/zip_archive_stream_entry.h>
 #include <ziparchive/zip_writer.h>
 
-static TemporaryFile* CreateZip() {
-  TemporaryFile* result = new TemporaryFile;
+static std::unique_ptr<TemporaryFile> CreateZip(int size = 4, int count = 1000) {
+  auto result = std::make_unique<TemporaryFile>();
   FILE* fp = fdopen(result->fd, "w");
 
   ZipWriter writer(fp);
   std::string lastName = "file";
-  for (size_t i = 0; i < 1000; i++) {
+  for (size_t i = 0; i < count; i++) {
     // Make file names longer and longer.
     lastName = lastName + std::to_string(i);
     writer.StartEntry(lastName.c_str(), ZipWriter::kCompress);
-    writer.WriteBytes("helo", 4);
+    while (size > 0) {
+      writer.WriteBytes("helo", 4);
+      size -= 4;
+    }
     writer.FinishEntry();
   }
   writer.Finish();
@@ -106,5 +108,28 @@ static void StartAlignedEntry(benchmark::State& state) {
 }
 BENCHMARK(StartAlignedEntry)->Arg(2)->Arg(16)->Arg(1024)->Arg(4096);
 
+static void ExtractEntry(benchmark::State& state) {
+  std::unique_ptr<TemporaryFile> temp_file(CreateZip(1024 * 1024, 1));
+
+  ZipArchiveHandle handle;
+  ZipEntry data;
+  if (OpenArchive(temp_file->path, &handle)) {
+    state.SkipWithError("Failed to open archive");
+  }
+  if (FindEntry(handle, "file0", &data)) {
+    state.SkipWithError("Failed to find archive entry");
+  }
+
+  std::vector<uint8_t> buffer(1024 * 1024);
+  for (auto _ : state) {
+    if (ExtractToMemory(handle, &data, buffer.data(), uint32_t(buffer.size()))) {
+      state.SkipWithError("Failed to extract archive entry");
+      break;
+    }
+  }
+  CloseArchive(handle);
+}
+
+BENCHMARK(ExtractEntry)->Arg(2)->Arg(16)->Arg(1024);
 
 BENCHMARK_MAIN();