Improved calculation of CRC32

- made code a simple function - switched to taking a std::string_view to avoid unnecessary allocations - renamed function and file name to match
cms-sw · Dec 13, 2024 · d8046a1 · d8046a1
1 parent 99becea
commit d8046a1
Show file tree

Hide file tree

Showing 7 changed files with 45 additions and 59 deletions.
diff --git a/DataFormats/Provenance/src/BranchID.cc b/DataFormats/Provenance/src/BranchID.cc
@@ -1,13 +1,10 @@
 #include "DataFormats/Provenance/interface/BranchID.h"
-#include "FWCore/Utilities/interface/CRC32Calculator.h"
+#include "FWCore/Utilities/interface/calculateCRC32.h"
 #include <ostream>
 
 namespace edm {
 
-  BranchID::value_type BranchID::toID(std::string const& branchName) {
-    cms::CRC32Calculator crc32(branchName);
-    return crc32.checksum();
-  }
+  BranchID::value_type BranchID::toID(std::string const& branchName) { return cms::calculateCRC32(branchName); }
 
   std::ostream& operator<<(std::ostream& os, BranchID const& id) {
     os << id.id();

diff --git a/EventFilter/L1TRawToDigi/src/AMC13Spec.cc b/EventFilter/L1TRawToDigi/src/AMC13Spec.cc
@@ -3,11 +3,11 @@
 
 #include "FWCore/Framework/interface/Event.h"
 #include "FWCore/MessageLogger/interface/MessageLogger.h"
-#include "FWCore/Utilities/interface/CRC32Calculator.h"
+#include "FWCore/Utilities/interface/calculateCRC32.h"
 
 #include "EventFilter/L1TRawToDigi/interface/AMC13Spec.h"
 
-#define EDM_ML_DEBUG 1
+//#define EDM_ML_DEBUG 1
 
 namespace amc13 {
   Header::Header(unsigned int namc, unsigned int orbit) {
@@ -38,8 +38,8 @@ namespace amc13 {
   }
 
   void Trailer::writeCRC(const uint64_t* start, uint64_t* end) {
-    std::string dstring(reinterpret_cast<const char*>(start), reinterpret_cast<const char*>(end) + 4);
-    auto crc = cms::CRC32Calculator(dstring).checksum();
+    std::string_view dstring(reinterpret_cast<const char*>(start), reinterpret_cast<const char*>(end) + 4);
+    auto crc = cms::calculateCRC32(dstring);
 
     *end = ((*end) & ~(uint64_t(CRC_mask) << CRC_shift)) | (static_cast<uint64_t>(crc & CRC_mask) << CRC_shift);
   }
@@ -128,8 +128,8 @@ namespace amc13 {
 
     int crc = 0;
     if (check_crc) {
-      std::string check(reinterpret_cast<const char*>(start), reinterpret_cast<const char*>(data) - 4);
-      crc = cms::CRC32Calculator(check).checksum();
+      std::string_view check(reinterpret_cast<const char*>(start), reinterpret_cast<const char*>(data) - 4);
+      crc = cms::calculateCRC32(check);
 
       LogDebug("L1T") << "checking data checksum of " << std::hex << crc << std::dec;
     }
@@ -156,8 +156,8 @@ namespace amc13 {
       t = Trailer(data++);
 
       if (check_crc) {
-        std::string check(reinterpret_cast<const char*>(start), reinterpret_cast<const char*>(data) - 4);
-        crc = cms::CRC32Calculator(check).checksum();
+        std::string_view check(reinterpret_cast<const char*>(start), reinterpret_cast<const char*>(data) - 4);
+        crc = cms::calculateCRC32(check);
 
         LogDebug("L1T") << "checking data checksum of " << std::hex << crc << std::dec;
       } else {

diff --git a/EventFilter/L1TRawToDigi/src/AMCSpec.cc b/EventFilter/L1TRawToDigi/src/AMCSpec.cc
@@ -1,11 +1,11 @@
 #include <iomanip>
 
 #include "FWCore/MessageLogger/interface/MessageLogger.h"
-#include "FWCore/Utilities/interface/CRC32Calculator.h"
+#include "FWCore/Utilities/interface/calculateCRC32.h"
 
 #include "EventFilter/L1TRawToDigi/interface/AMCSpec.h"
 
-#define EDM_ML_DEBUG 1
+//#define EDM_ML_DEBUG 1
 
 namespace amc {
   BlockHeader::BlockHeader(unsigned int amc_no, unsigned int board_id, unsigned int size, unsigned int block) {
@@ -92,8 +92,8 @@ namespace amc {
   }
 
   void Trailer::writeCRC(const uint64_t *start, uint64_t *end) {
-    std::string dstring(reinterpret_cast<const char *>(start), reinterpret_cast<const char *>(end) + 4);
-    auto crc = cms::CRC32Calculator(dstring).checksum();
+    std::string_view dstring(reinterpret_cast<const char *>(start), reinterpret_cast<const char *>(end) + 4);
+    auto crc = cms::calculateCRC32(dstring);
 
     *end = ((*end) & ~(uint64_t(CRC_mask) << CRC_shift)) | (static_cast<uint64_t>(crc & CRC_mask) << CRC_shift);
   }
@@ -133,8 +133,8 @@ namespace amc {
       header_ = Header(payload_.data());
       trailer_ = Trailer(&payload_.back());
 
-      std::string check(reinterpret_cast<const char *>(payload_.data()), payload_.size() * 8 - 4);
-      auto crc = cms::CRC32Calculator(check).checksum();
+      std::string_view check(reinterpret_cast<const char *>(payload_.data()), payload_.size() * 8 - 4);
+      auto crc = cms::calculateCRC32(check);
 
       trailer_.check(crc, lv1, header_.getSize(), mtf7_mode);
     }

diff --git a/FWCore/Utilities/interface/CRC32Calculator.h → FWCore/Utilities/interface/calculateCRC32.h b/FWCore/Utilities/interface/CRC32Calculator.h → FWCore/Utilities/interface/calculateCRC32.h
@@ -1,5 +1,5 @@
-#ifndef FWCore_Utilities_CRC32Calculator_h
-#define FWCore_Utilities_CRC32Calculator_h
+#ifndef FWCore_Utilities_calculateCRC32_h
+#define FWCore_Utilities_calculateCRC32_h
 
 /*
 Code to calculate a CRC32 checksum on a string.  This code is based
@@ -55,18 +55,10 @@ from the original code follow below to attribute the source.
 
 #include <cstdint>
 
-#include <string>
+#include <string_view>
 
 namespace cms {
 
-  class CRC32Calculator {
-  public:
-    CRC32Calculator(std::string const& message);
-
-    std::uint32_t checksum() { return checksum_; }
-
-  private:
-    std::uint32_t checksum_;
-  };
+  std::uint32_t calculateCRC32(std::string_view message);
 }  // namespace cms
 #endif
diff --git a/FWCore/Utilities/src/CRC32Calculator.cc → FWCore/Utilities/src/calculateCRC32.cc b/FWCore/Utilities/src/CRC32Calculator.cc → FWCore/Utilities/src/calculateCRC32.cc
@@ -1,14 +1,14 @@
 
-#include "FWCore/Utilities/interface/CRC32Calculator.h"
+#include "FWCore/Utilities/interface/calculateCRC32.h"
 
 namespace cms {
 
   namespace {
 
-    const std::uint32_t CRC32_XINIT = 0xFFFFFFFFL;
-    const std::uint32_t CRC32_XOROT = 0xFFFFFFFFL;
+    constexpr std::uint32_t CRC32_XINIT = 0xFFFFFFFFL;
+    constexpr std::uint32_t CRC32_XOROT = 0xFFFFFFFFL;
 
-    const std::uint32_t crctable[256] = {
+    constexpr std::uint32_t crctable[256] = {
         0x00000000L, 0x77073096L, 0xEE0E612CL, 0x990951BAL, 0x076DC419L, 0x706AF48FL, 0xE963A535L, 0x9E6495A3L,
         0x0EDB8832L, 0x79DCB8A4L, 0xE0D5E91EL, 0x97D2D988L, 0x09B64C2BL, 0x7EB17CBDL, 0xE7B82D07L, 0x90BF1D91L,
         0x1DB71064L, 0x6AB020F2L, 0xF3B97148L, 0x84BE41DEL, 0x1ADAD47DL, 0x6DDDE4EBL, 0xF4D4B551L, 0x83D385C7L,
@@ -43,19 +43,16 @@ namespace cms {
         0xB3667A2EL, 0xC4614AB8L, 0x5D681B02L, 0x2A6F2B94L, 0xB40BBE37L, 0xC30C8EA1L, 0x5A05DF1BL, 0x2D02EF8DL};
   }  // namespace
 
-  CRC32Calculator::CRC32Calculator(std::string const& message) {
+  std::uint32_t calculateCRC32(std::string_view message) {
     /* initialize value */
-    checksum_ = CRC32_XINIT;
+    uint32_t checksum = CRC32_XINIT;
 
     /* process each byte prior to checksum field */
-    auto length = message.length();
-    char const* p = message.data();
-    for (size_t j = 0; j < length; j++) {
-      unsigned char uc = *p++;
-      checksum_ = cms::crctable[(checksum_ ^ uc) & 0xFFL] ^ (checksum_ >> 8);
+    for (auto uc : message) {
+      checksum = cms::crctable[(checksum ^ uc) & 0xFFL] ^ (checksum >> 8);
     }
 
     /* return XOR out value */
-    checksum_ = checksum_ ^ CRC32_XOROT;
+    return checksum ^ CRC32_XOROT;
   }
 }  // namespace cms
diff --git a/FWCore/Utilities/test/test_catch2_CRC32Calculator.cc b/FWCore/Utilities/test/test_catch2_CRC32Calculator.cc
diff --git a/FWCore/Utilities/test/test_catch2_calculateCRC32.cc b/FWCore/Utilities/test/test_catch2_calculateCRC32.cc
@@ -0,0 +1,16 @@
+#include "catch.hpp"
+#include "FWCore/Utilities/interface/calculateCRC32.h"
+
+TEST_CASE("test cms::calculateCRC32", "[calculateCRC32]") {
+  SECTION("known") {
+    auto crc32 = cms::calculateCRC32("type_label_instance_process");
+
+    // This known result was calculated using python as a cross check
+    unsigned int knownResult = 1215348599;
+    REQUIRE(crc32 == knownResult);
+  }
+  SECTION("empty") {
+    auto emptyString_crc32 = cms::calculateCRC32("");
+    REQUIRE(emptyString_crc32 == 0);
+  }
+}