[Bug] Fixed a string buffer growing strategy

For some reason the growing strategy of asmjit::String was too aggressive, basically reaching the maximum doubling capacity too fast (after the first reallocation). This code adapts the current vector growing strategy to be used also by asmjit::String, which doubles the capacity until a threshold is reached and then grows linearly.
asmjit · Jun 22, 2024 · 062e69c · 062e69c
1 parent f5df7a2
commit 062e69c
Show file tree

Hide file tree

Showing 2 changed files with 86 additions and 24 deletions.
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
@@ -3,6 +3,10 @@ on:
   push:
   pull_request:
 
+concurrency:
+  group: ${{github.ref}}
+  cancel-in-progress: ${{github.ref != 'refs/heads/master'}}
+
 defaults:
   run:
     shell: bash
@@ -133,14 +137,14 @@ jobs:
           - { title: "windows"         , host: "windows-2022"  , arch: "x64"    , cc: "vs2022"  , conf: "Debug"  , defs: "ASMJIT_TEST=1" }
           - { title: "windows"         , host: "windows-2022"  , arch: "x64"    , cc: "vs2022"  , conf: "Release", defs: "ASMJIT_TEST=1" }
 
-          - { title: "freebsd"         , host: "macos-12"      , arch: "x86-64" , cc: "clang"   , conf: "Release", vm: "freebsd", vm_ver: "13.2", defs: "ASMJIT_TEST=1" }
-          - { title: "netbsd"          , host: "macos-12"      , arch: "x86-64" , cc: "clang"   , conf: "Release", vm: "netbsd" , vm_ver: "9.3" , defs: "ASMJIT_TEST=1" }
-          - { title: "openbsd"         , host: "macos-12"      , arch: "x86-64" , cc: "clang"   , conf: "Release", vm: "openbsd", vm_ver: "7.4" , defs: "ASMJIT_TEST=1" }
+          - { title: "freebsd"         , host: "ubuntu-latest" , arch: "x64"    , cc: "clang"   , conf: "Release", vm: "freebsd", vm_ver: "14.1", defs: "ASMJIT_TEST=1" }
+          - { title: "freebsd"         , host: "ubuntu-latest" , arch: "arm64"  , cc: "clang"   , conf: "Release", vm: "freebsd", vm_ver: "14.1", defs: "ASMJIT_TEST=1" }
+          - { title: "netbsd"          , host: "ubuntu-latest" , arch: "x64"    , cc: "clang"   , conf: "Release", vm: "netbsd" , vm_ver: "10.0", defs: "ASMJIT_TEST=1" }
+          - { title: "netbsd"          , host: "ubuntu-latest" , arch: "arm64"  , cc: "clang"   , conf: "Release", vm: "netbsd" , vm_ver: "10.0", defs: "ASMJIT_TEST=1" }
+          - { title: "openbsd"         , host: "ubuntu-latest" , arch: "x64"    , cc: "clang"   , conf: "Release", vm: "openbsd", vm_ver: "7.4" , defs: "ASMJIT_TEST=1" }
           - { title: "openbsd"         , host: "ubuntu-latest" , arch: "arm64"  , cc: "clang"   , conf: "Release", vm: "openbsd", vm_ver: "7.4" , defs: "ASMJIT_TEST=1" }
 
-          # arm/v7 VM image doesn't work on CI environment at the moment
-          # - { title: "debian"          , host: "ubuntu-latest" , arch: "arm/v7" , cc: "clang"   , conf: "Release", vm: "debian:unstable", defs: "ASMJIT_TEST=1" }
-
+          - { title: "debian"          , host: "ubuntu-latest" , arch: "arm/v7" , cc: "clang"   , conf: "Release", vm: "debian:unstable", defs: "ASMJIT_TEST=1" }
           - { title: "debian"          , host: "ubuntu-latest" , arch: "arm64"  , cc: "clang"   , conf: "Release", vm: "debian:unstable", defs: "ASMJIT_TEST=1" }
           - { title: "debian"          , host: "ubuntu-latest" , arch: "riscv64", cc: "clang"   , conf: "Release", vm: "debian:unstable", defs: "ASMJIT_TEST=1" }
           - { title: "debian"          , host: "ubuntu-latest" , arch: "ppc64le", cc: "clang"   , conf: "Release", vm: "debian:unstable", defs: "ASMJIT_TEST=1" }

diff --git a/src/asmjit/core/string.cpp b/src/asmjit/core/string.cpp
@@ -14,9 +14,51 @@ ASMJIT_BEGIN_NAMESPACE
 
 static const char String_baseN[] = "0123456789ABCDEF";
 
-constexpr size_t kMinAllocSize = 64;
+constexpr size_t kMinAllocSize = 128;
 constexpr size_t kMaxAllocSize = SIZE_MAX - Globals::kGrowThreshold;
 
+// Based on ZoneVector_growCapacity().
+//
+// NOTE: The sizes here include null terminators - that way we can have aligned allocations that are power of 2s
+// initially.
+static ASMJIT_FORCE_INLINE size_t String_growCapacity(size_t byteSize, size_t minimumByteSize) noexcept {
+  static constexpr size_t kGrowThreshold = Globals::kGrowThreshold;
+
+  ASMJIT_ASSERT(minimumByteSize < kMaxAllocSize);
+
+  // This is more than exponential growth at the beginning.
+  if (byteSize < kMinAllocSize) {
+    byteSize = kMinAllocSize;
+  }
+  else if (byteSize < 512) {
+    byteSize = 512;
+  }
+
+  if (byteSize < minimumByteSize) {
+    // Exponential growth before we reach `kGrowThreshold`.
+    byteSize = Support::alignUpPowerOf2(minimumByteSize);
+
+    // Bail to `minimumByteSize` in case of overflow - most likely whatever that is happening afterwards would just fail.
+    if (byteSize < minimumByteSize) {
+      return minimumByteSize;
+    }
+
+    // Pretty much chunked growth advancing by `kGrowThreshold` after we exceed it.
+    if (byteSize > kGrowThreshold) {
+      // Align to kGrowThreshold.
+      size_t remainder = minimumByteSize % kGrowThreshold;
+
+      byteSize = minimumByteSize + remainder;
+
+      // Bail to `minimumByteSize` in case of overflow.
+      if (byteSize < minimumByteSize)
+        return minimumByteSize;
+    }
+  }
+
+  return Support::min<size_t>(byteSize, kMaxAllocSize);
+}
+
 // String - Clear & Reset
 // ======================
 
@@ -49,13 +91,13 @@ char* String::prepare(ModifyOp op, size_t size) noexcept {
   size_t curCapacity;
 
   if (isLargeOrExternal()) {
-    curData = this->_large.data;
-    curSize = this->_large.size;
-    curCapacity = this->_large.capacity;
+    curData = _large.data;
+    curSize = _large.size;
+    curCapacity = _large.capacity;
   }
   else {
-    curData = this->_small.data;
-    curSize = this->_small.type;
+    curData = _small.data;
+    curSize = _small.type;
     curCapacity = kSSOCapacity;
   }
 
@@ -90,25 +132,20 @@ char* String::prepare(ModifyOp op, size_t size) noexcept {
   }
   else {
     // Prevent arithmetic overflow.
-    if (ASMJIT_UNLIKELY(size >= kMaxAllocSize - curSize))
+    if (ASMJIT_UNLIKELY(size >= kMaxAllocSize - curSize - 1))
       return nullptr;
 
     size_t newSize = size + curSize;
     size_t newSizePlusOne = newSize + 1;
 
-    if (newSizePlusOne > curCapacity) {
-      size_t newCapacity = Support::max<size_t>(curCapacity + 1, kMinAllocSize);
-
-      if (newCapacity < newSizePlusOne && newCapacity < Globals::kGrowThreshold)
-        newCapacity = Support::alignUpPowerOf2(newCapacity);
-
-      if (newCapacity < newSizePlusOne)
-        newCapacity = Support::alignUp(newSizePlusOne, Globals::kGrowThreshold);
+    if (newSize > curCapacity) {
+      size_t newCapacityPlusOne = String_growCapacity(size + 1u, newSizePlusOne);
+      ASMJIT_ASSERT(newCapacityPlusOne >= newSizePlusOne);
 
-      if (ASMJIT_UNLIKELY(newCapacity < newSizePlusOne))
+      if (ASMJIT_UNLIKELY(newCapacityPlusOne < newSizePlusOne))
         return nullptr;
 
-      char* newData = static_cast<char*>(::malloc(newCapacity));
+      char* newData = static_cast<char*>(::malloc(newCapacityPlusOne));
       if (ASMJIT_UNLIKELY(!newData))
         return nullptr;
 
@@ -119,7 +156,7 @@ char* String::prepare(ModifyOp op, size_t size) noexcept {
 
       _large.type = kTypeLarge;
       _large.size = newSize;
-      _large.capacity = newCapacity - 1;
+      _large.capacity = newCapacityPlusOne - 1;
       _large.data = newData;
 
       newData[newSize] = '\0';
@@ -488,9 +525,28 @@ bool String::equals(const char* other, size_t size) const noexcept {
 // ==============
 
 #if defined(ASMJIT_TEST)
+static void test_string_grow() noexcept {
+  String s;
+  size_t c = s.capacity();
+
+  INFO("Testing string grow strategy (SSO capacity: %zu)", c);
+  for (size_t i = 0; i < 1000000; i++) {
+    s.append('x');
+    if (s.capacity() != c) {
+      c = s.capacity();
+      INFO("  String reallocated to new capacity: %zu", c);
+    }
+  }
+
+  // We don't expect a 1 million character string to occupy 4MiB, for example. So verify that!
+  EXPECT_LT(c, size_t(4 * 1024 * 1024));
+}
+
 UNIT(core_string) {
   String s;
 
+  INFO("Testing string functionality");
+
   EXPECT_FALSE(s.isLargeOrExternal());
   EXPECT_FALSE(s.isExternal());
 
@@ -553,6 +609,8 @@ UNIT(core_string) {
   EXPECT_TRUE(sTmp.isExternal());
   EXPECT_EQ(sTmp.appendChars(' ', 1000), kErrorOk);
   EXPECT_FALSE(sTmp.isExternal());
+
+  test_string_grow();
 }
 #endif