From 18ee6a75b5c668f3f0d3380d26e1f8816b2f8bd2 Mon Sep 17 00:00:00 2001
From: James M Snell <jasnell@gmail.com>
Date: Wed, 9 Oct 2024 11:05:20 -0700
Subject: [PATCH 1/3] Deduplicate multiple isHexDigit impls and optimize

---
 src/workerd/api/util.c++    |  6 +++---
 src/workerd/api/util.h      |  5 -----
 src/workerd/jsg/BUILD.bazel |  1 +
 src/workerd/jsg/url.c++     |  7 ++-----
 src/workerd/util/strings.h  | 17 +++++++++++++++++
 5 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/src/workerd/api/util.c++ b/src/workerd/api/util.c++
index 03e1b16092a..276f2916da2 100644
--- a/src/workerd/api/util.c++
+++ b/src/workerd/api/util.c++
@@ -7,6 +7,7 @@
 #include "simdutf.h"
 
 #include <workerd/util/mimetype.h>
+#include <workerd/util/strings.h>
 
 #include <kj/encoding.h>
 
@@ -216,7 +217,6 @@ kj::String redactUrl(kj::StringPtr url) {
     bool isUpper = ('A' <= c && c <= 'Z');
     bool isLower = ('a' <= c && c <= 'z');
     bool isDigit = ('0' <= c && c <= '9');
-    bool isHexDigit = (isDigit || ('A' <= c && c <= 'F') || ('a' <= c && c <= 'f'));
     bool isSep = (c == '+' || c == '-' || c == '_');
     // These extra characters are used in the regular and url-safe versions of
     // base64, but might also be used for GUID-style separators in hex ids.
@@ -225,10 +225,10 @@ kj::String redactUrl(kj::StringPtr url) {
     // character.
 
     if (isUpper || isLower || isDigit || isSep) {
-      if (isHexDigit) {
+      if (isHexDigit(c)) {
         hexDigitCount++;
       }
-      if (!isHexDigit && !isSep) {
+      if (!isHexDigit(c) && !isSep) {
         sawNonHexChar = true;
       }
       if (isUpper) {
diff --git a/src/workerd/api/util.h b/src/workerd/api/util.h
index 09edc77812e..5333c807939 100644
--- a/src/workerd/api/util.h
+++ b/src/workerd/api/util.h
@@ -35,11 +35,6 @@ kj::String toLower(kj::String&& str);
 // Mutate `str` with all alphabetic ASCII characters uppercased. Returns `str`.
 kj::String toUpper(kj::String&& str);
 
-inline bool isHexDigit(uint32_t c) {
-  // Check if `c` is the ASCII code of a hexadecimal digit.
-  return ('0' <= c && c <= '9') || ('a' <= c && c <= 'f') || ('A' <= c && c <= 'F');
-}
-
 // Parse `rawText` as application/x-www-form-urlencoded name/value pairs and store in `query`. If
 // `skipLeadingQuestionMark` is true, any initial '?' will be ignored. Otherwise, it will be
 // interpreted as part of the first URL-encoded field.
diff --git a/src/workerd/jsg/BUILD.bazel b/src/workerd/jsg/BUILD.bazel
index b84b00b48b7..01de37e92d5 100644
--- a/src/workerd/jsg/BUILD.bazel
+++ b/src/workerd/jsg/BUILD.bazel
@@ -92,6 +92,7 @@ wd_cc_library(
     visibility = ["//visibility:public"],
     deps = [
         ":memory-tracker",
+        "//src/workerd/util:strings",
         "@capnp-cpp//src/kj",
     ],
 )
diff --git a/src/workerd/jsg/url.c++ b/src/workerd/jsg/url.c++
index 515a43aa4a9..77e258212b7 100644
--- a/src/workerd/jsg/url.c++
+++ b/src/workerd/jsg/url.c++
@@ -1,5 +1,7 @@
 #include "url.h"
 
+#include <workerd/util/strings.h>
+
 #include <kj/hash.h>
 
 extern "C" {
@@ -488,11 +490,6 @@ inline bool isAsciiDigit(char c) {
   return c >= '0' && c <= '9';
 };
 
-inline bool isHexDigit(char c) {
-  // Check if `c` is the ASCII code of a hexadecimal digit.
-  return isAsciiDigit(c) || ('a' <= c && c <= 'f') || ('A' <= c && c <= 'F');
-}
-
 inline bool isAscii(char codepoint) {
   return codepoint >= 0x00 && codepoint <= 0x7f;
 };
diff --git a/src/workerd/util/strings.h b/src/workerd/util/strings.h
index c036128db2e..35c80e4c4d6 100644
--- a/src/workerd/util/strings.h
+++ b/src/workerd/util/strings.h
@@ -23,4 +23,21 @@ inline kj::String toLowerCopy(kj::ArrayPtr<const char> ptr) {
   return kj::mv(str);
 }
 
+constexpr kj::FixedArray<uint8_t, 256> kHexDigitTable = []() consteval {
+  kj::FixedArray<uint8_t, 256> result{};
+  for (uint8_t c: {'1', '2', '3', '4', '5', '6', '7', '8', '9', '0'}) {
+    result[c] = true;
+  }
+  for (uint8_t c: {'A', 'B', 'C', 'D', 'E', 'F'}) {
+    result[c] = true;         // Uppercase
+    result[c + 0x20] = true;  // Lowercase
+  }
+  return result;
+}();
+
+// Check if `c` is the ASCII code of a hexadecimal digit.
+constexpr bool isHexDigit(char c) {
+  return kHexDigitTable[static_cast<int>(c)] == 1;
+}
+
 }  // namespace workerd

From 0a4ed822b18431b2131c65f5a34ea9c98bce778a Mon Sep 17 00:00:00 2001
From: James M Snell <jasnell@gmail.com>
Date: Wed, 9 Oct 2024 12:52:05 -0700
Subject: [PATCH 2/3] Consolidate and optimize more ascii character checks

---
 src/workerd/api/data-url.c++        |  3 +-
 src/workerd/api/encoding.c++        |  1 +
 src/workerd/api/encoding.h          | 12 ----
 src/workerd/api/pyodide/pyodide.c++ |  5 +-
 src/workerd/api/util.c++            | 21 ++++---
 src/workerd/util/strings.h          | 89 +++++++++++++++++++++++------
 6 files changed, 88 insertions(+), 43 deletions(-)

diff --git a/src/workerd/api/data-url.c++ b/src/workerd/api/data-url.c++
index 61808802b0a..42d55b9509a 100644
--- a/src/workerd/api/data-url.c++
+++ b/src/workerd/api/data-url.c++
@@ -1,6 +1,7 @@
 #include "data-url.h"
 
 #include <workerd/api/encoding.h>
+#include <workerd/util/strings.h>
 
 #include <kj/encoding.h>
 
@@ -39,7 +40,7 @@ kj::Maybe<DataUrl> DataUrl::from(const jsg::Url& url) {
   static const auto strip = [](auto label) {
     auto result = kj::heapArray<kj::byte>(label.size());
     size_t len = 0;
-    for (auto c: label) {
+    for (const char c: label) {
       if (!isAsciiWhitespace(c)) {
         result[len++] = c;
       }
diff --git a/src/workerd/api/encoding.c++ b/src/workerd/api/encoding.c++
index 7186ad837a1..2ef849c7abb 100644
--- a/src/workerd/api/encoding.c++
+++ b/src/workerd/api/encoding.c++
@@ -7,6 +7,7 @@
 #include "util.h"
 
 #include <workerd/jsg/jsg.h>
+#include <workerd/util/strings.h>
 
 #include <unicode/ucnv.h>
 #include <unicode/utf8.h>
diff --git a/src/workerd/api/encoding.h b/src/workerd/api/encoding.h
index 420ec1c7b53..1dc24be2257 100644
--- a/src/workerd/api/encoding.h
+++ b/src/workerd/api/encoding.h
@@ -11,18 +11,6 @@
 
 namespace workerd::api {
 
-constexpr kj::FixedArray<uint8_t, 256> ascii_whitespace_table = []() consteval {
-  kj::FixedArray<uint8_t, 256> result{};
-  for (uint8_t c: {0x09, 0x0a, 0x0c, 0x0d, 0x20}) {
-    result[c] = true;
-  }
-  return result;
-}();
-
-constexpr bool isAsciiWhitespace(uint8_t c) noexcept {
-  return ascii_whitespace_table[c];
-}
-
 // The encodings listed here are defined as required by the Encoding spec.
 // The first label is enum we use to identify the encoding in code, while
 // the second label is the public identifier.
diff --git a/src/workerd/api/pyodide/pyodide.c++ b/src/workerd/api/pyodide/pyodide.c++
index 9c8a5884aab..b09b1b2aecc 100644
--- a/src/workerd/api/pyodide/pyodide.c++
+++ b/src/workerd/api/pyodide/pyodide.c++
@@ -4,6 +4,7 @@
 #include "pyodide.h"
 
 #include <workerd/util/string-buffer.h>
+#include <workerd/util/strings.h>
 
 #include <kj/array.h>
 #include <kj/common.h>
@@ -190,13 +191,13 @@ kj::Array<kj::String> ArtifactBundler::parsePythonScriptImports(kj::Array<kj::St
       // We also accept `.` because import idents can contain it.
       // TODO: We don't currently support unicode, but if we see packages that utilise it we will
       // implement that support.
-      if (std::isdigit(str[start])) {
+      if (isDigit(str[start])) {
         return 0;
       }
       int i = 0;
       for (; start + i < str.size(); i++) {
         char c = str[start + i];
-        bool validIdentChar = std::isalpha(c) || std::isdigit(c) || c == '_' || c == '.';
+        bool validIdentChar = isAlpha(c) || isDigit(c) || c == '_' || c == '.';
         if (!validIdentChar) {
           return i;
         }
diff --git a/src/workerd/api/util.c++ b/src/workerd/api/util.c++
index 276f2916da2..b98c876ba6b 100644
--- a/src/workerd/api/util.c++
+++ b/src/workerd/api/util.c++
@@ -214,27 +214,30 @@ kj::String redactUrl(kj::StringPtr url) {
   };
 
   for (const char& c: url) {
-    bool isUpper = ('A' <= c && c <= 'Z');
-    bool isLower = ('a' <= c && c <= 'z');
-    bool isDigit = ('0' <= c && c <= '9');
-    bool isSep = (c == '+' || c == '-' || c == '_');
+    uint8_t lookup = kCharLookupTable[c];
+    bool isSep = lookup & CharAttributeFlag::SEPARATOR;
+    bool isAlphaUpper = lookup & CharAttributeFlag::UPPER_CASE;
+    bool isAlphaLower = lookup & CharAttributeFlag::LOWER_CASE;
+    bool isDigit = lookup & CharAttributeFlag::DIGIT;
+    bool isHex = lookup & CharAttributeFlag::HEX;
+
     // These extra characters are used in the regular and url-safe versions of
     // base64, but might also be used for GUID-style separators in hex ids.
     // Regular base64 also includes '/', which we don't try to match here due
     // to its prevalence in URLs.  Likewise, we ignore the base64 "=" padding
     // character.
 
-    if (isUpper || isLower || isDigit || isSep) {
-      if (isHexDigit(c)) {
+    if (isAlphaUpper || isAlphaLower || isDigit || isSep) {
+      if (isHex) {
         hexDigitCount++;
       }
-      if (!isHexDigit(c) && !isSep) {
+      if (!isHex && !isSep) {
         sawNonHexChar = true;
       }
-      if (isUpper) {
+      if (isAlphaUpper) {
         upperCount++;
       }
-      if (isLower) {
+      if (isAlphaLower) {
         lowerCount++;
       }
       if (isDigit) {
diff --git a/src/workerd/util/strings.h b/src/workerd/util/strings.h
index 35c80e4c4d6..67cdeeddcfc 100644
--- a/src/workerd/util/strings.h
+++ b/src/workerd/util/strings.h
@@ -3,14 +3,82 @@
 //     https://opensource.org/licenses/Apache-2.0
 #pragma once
 
+#include <kj/debug.h>
 #include <kj/string.h>
 
 namespace workerd {
 
+enum CharAttributeFlag : uint8_t {
+  NONE = 0,
+  ALPHA = 1 << 0,
+  DIGIT = 1 << 1,
+  HEX = 1 << 2,
+  ASCII = 1 << 3,
+  ASCII_WHITESPACE = 1 << 4,
+  UPPER_CASE = 1 << 5,
+  LOWER_CASE = 1 << 6,
+  SEPARATOR = 1 << 7,
+};
+
+// Construct a lookup table for various interesting character properties.
+constexpr kj::FixedArray<uint8_t, 256> kCharLookupTable = []() consteval {
+  kj::FixedArray<uint8_t, 256> result{};
+  for (uint8_t c = 'A'; c <= 'Z'; c++) {
+    if (c <= 'F') {
+      result[c] |= CharAttributeFlag::HEX;
+      result[c + 0x20] |= CharAttributeFlag::HEX;
+    }
+    result[c] |= CharAttributeFlag::ALPHA | CharAttributeFlag::UPPER_CASE;
+    result[c + 0x20] |= CharAttributeFlag::ALPHA | CharAttributeFlag::LOWER_CASE;
+  }
+  for (uint8_t c = '0'; c <= '9'; c++) {
+    result[c] |= CharAttributeFlag::DIGIT | CharAttributeFlag::HEX;
+  }
+  for (uint8_t c = 0; c <= 0x7f; c++) {
+    result[c] |= CharAttributeFlag::ASCII;
+  }
+  for (uint8_t c: {0x09, 0x0a, 0x0c, 0x0d, 0x20}) {
+    result[c] |= CharAttributeFlag::ASCII_WHITESPACE;
+  }
+  result['+'] |= CharAttributeFlag::SEPARATOR;
+  result['-'] |= CharAttributeFlag::SEPARATOR;
+  result['_'] |= CharAttributeFlag::SEPARATOR;
+  return result;
+}();
+
+constexpr bool isAlpha(const kj::byte c) noexcept {
+  return kCharLookupTable[c] & CharAttributeFlag::ALPHA;
+}
+
+constexpr bool isDigit(const kj::byte c) noexcept {
+  return kCharLookupTable[c] & CharAttributeFlag::DIGIT;
+}
+
+// Check if `c` is the ASCII code of a hexadecimal digit.
+constexpr bool isHexDigit(const kj::byte c) noexcept {
+  return kCharLookupTable[c] & CharAttributeFlag::HEX;
+}
+
+constexpr bool isAscii(const kj::byte c) noexcept {
+  return kCharLookupTable[c] & CharAttributeFlag::ASCII;
+}
+
+constexpr bool isAsciiWhitespace(const kj::byte c) noexcept {
+  return kCharLookupTable[c] & CharAttributeFlag::ASCII_WHITESPACE;
+}
+
+constexpr bool isAlphaUpper(const kj::byte c) noexcept {
+  return kCharLookupTable[c] & CharAttributeFlag::UPPER_CASE;
+}
+
+constexpr bool isAlphaLower(const kj::byte c) noexcept {
+  return kCharLookupTable[c] & CharAttributeFlag::LOWER_CASE;
+}
+
 inline kj::String toLowerCopy(kj::StringPtr ptr) {
   auto str = kj::str(ptr);
   for (char& c: str) {
-    if ('A' <= c && c <= 'Z') c += 'a' - 'A';
+    if (isAlphaUpper(c)) c += 0x20;
   }
   return kj::mv(str);
 }
@@ -18,26 +86,9 @@ inline kj::String toLowerCopy(kj::StringPtr ptr) {
 inline kj::String toLowerCopy(kj::ArrayPtr<const char> ptr) {
   auto str = kj::str(ptr);
   for (char& c: str) {
-    if ('A' <= c && c <= 'Z') c += 'a' - 'A';
+    if (isAlphaUpper(c)) c += 0x20;
   }
   return kj::mv(str);
 }
 
-constexpr kj::FixedArray<uint8_t, 256> kHexDigitTable = []() consteval {
-  kj::FixedArray<uint8_t, 256> result{};
-  for (uint8_t c: {'1', '2', '3', '4', '5', '6', '7', '8', '9', '0'}) {
-    result[c] = true;
-  }
-  for (uint8_t c: {'A', 'B', 'C', 'D', 'E', 'F'}) {
-    result[c] = true;         // Uppercase
-    result[c + 0x20] = true;  // Lowercase
-  }
-  return result;
-}();
-
-// Check if `c` is the ASCII code of a hexadecimal digit.
-constexpr bool isHexDigit(char c) {
-  return kHexDigitTable[static_cast<int>(c)] == 1;
-}
-
 }  // namespace workerd

From dc975ed61642593e2f4f50454905425176ed42d2 Mon Sep 17 00:00:00 2001
From: James M Snell <jsnell@cloudflare.com>
Date: Wed, 9 Oct 2024 18:13:04 -0700
Subject: [PATCH 3/3] Apply suggestions from code review

Co-authored-by: Yagiz Nizipli <yagiz@cloudflare.com>
---
 src/workerd/api/data-url.c++ | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/workerd/api/data-url.c++ b/src/workerd/api/data-url.c++
index 42d55b9509a..d3d9c6819e2 100644
--- a/src/workerd/api/data-url.c++
+++ b/src/workerd/api/data-url.c++
@@ -40,7 +40,7 @@ kj::Maybe<DataUrl> DataUrl::from(const jsg::Url& url) {
   static const auto strip = [](auto label) {
     auto result = kj::heapArray<kj::byte>(label.size());
     size_t len = 0;
-    for (const char c: label) {
+    for (const kj::byte c: label) {
       if (!isAsciiWhitespace(c)) {
         result[len++] = c;
       }