refactor/dynamic environment part thirteen by zzcgumn · Pull Request #39 · zzcgumn/dds

zzcgumn · 2025-10-13T15:56:53Z

feat(system,util): introduce Arena and adopt for tiny log buffers; add unit test and testable default
perf(system): eliminate per-construction arena cost; remove default arena define in testable targets
perf(system): make Arena per-thread via registry and keep lazy access
feat(moves): add optional arena-backed scratch for MergeSort via TLS allocator
feat(arena): per-thread Arena reuse test + TLS scratch usage docs

…d unit test and testable default - Add dds::Arena (bump-pointer allocator) and integrate into SolverContext with optional capacity - Reset Arena on ResetForSolve to enable per-solve reuse - First adoption: use arena-backed small snprintf buffers for DDS_UTILITIES_LOG in SolverContext with stack fallback - Enable conservative default arena size (64KB) only for testable targets - Add arena_test to validate allocation/reset/fallback behavior Docs: perf artifact added under copilot/perf/artifacts with neutral timing notes

…rena define in testable targets - Lazy-initialize Arena on first use instead of in SolverContext constructor - Remove DDS_DEFAULT_ARENA_BYTES define from testable_dds variants to avoid implicit Arena activation in dtest Expected effect: restore parity with mn_main on dtest list1000 by preventing repeated Arena allocations per SolverContext construction.

- Allocate Arena once per ThreadData and reuse via a central registry - Keep ResetForSolve() semantics to reset the arena between solves - Remove per-instance arena member; provide out-of-line accessors This removes per-construction costs while preserving behavior neutrality.

…allocator - Introduce a tiny TLS scratch allocator (utility/ScratchAllocTLS) and wire it in SolverContext move generation entry points. - Moves::MergeSort now optionally uses a temporary copy buffer allocated via TLS for stable insertion, preserving behavior. - Add focused unit test merge_scratch_test to assert ordering parity. - Update inventory doc to record the first hot-path arena adoption. This is behavior-neutral by default; TLS allocator is only installed by SolverContext, and falls back seamlessly when absent.

- Add system test asserting per-thread Arena reuse and reset semantics - Keep behavior-neutral fallback for TLS scratch; MergeSort coverage in place - Document Arena/TLS usage contract, safety, and adoption checklist in inventory report

Copilot

Pull Request Overview

This PR introduces Arena-based memory allocation and thread-local scratch buffers to optimize memory management in the DDS library. The changes include a new Arena allocator system with per-thread reuse capabilities and integration into existing move sorting and logging functionality.

Introduces dds::Arena bump-pointer allocator with fallback support
Adds thread-local scratch allocation system (ScratchAllocTLS) for temporary buffers
Integrates Arena into SolverContext with per-thread registry and automatic reset

Reviewed Changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
library/src/system/util/Arena.h	Core Arena allocator implementation with bump-pointer and fallback allocation
library/src/system/SolverContext.h/.cpp	Integration of Arena into SolverContext with per-thread registry
library/src/utility/ScratchAllocTLS.h/.cpp	Thread-local scratch allocator API for temporary buffers
library/src/moves/Moves.cpp	Optional Arena-backed scratch buffer for MergeSort operations
library/tests/utility/arena_test.cpp	Unit tests for Arena basic functionality
library/tests/system/arena_reuse_test.cpp	Tests for per-thread Arena reuse behavior
library/tests/heuristic_sorting/merge_scratch_test.cpp	Validation test for MergeSort ordering with scratch buffers

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-13T15:57:54Z

library/src/utility/ArenaTLS.h

+// Lightweight TLS hook to make an Arena available in hot paths without
+// threading a context parameter through legacy code.
+#pragma once
+
+namespace dds {
+class Arena; // forward decl
+
+namespace tls {
+// Set by call sites that know the current thread's Arena (if any).
+// Consumers can treat this as optional: nullptr means "no arena".
+extern thread_local Arena* arena;
+}
+
+} // namespace dds


This file appears to be duplicated with library/src/system/util/ArenaTLS.h. Having identical files in different locations can lead to maintenance issues and confusion about which one is authoritative.

Suggested change

// Lightweight TLS hook to make an Arena available in hot paths without

// threading a context parameter through legacy code.

#pragma once

namespace dds {

class Arena; // forward decl

namespace tls {

// Set by call sites that know the current thread's Arena (if any).

// Consumers can treat this as optional: nullptr means "no arena".

extern thread_local Arena* arena;

}

} // namespace dds

Copilot · 2025-10-13T15:57:54Z

library/src/utility/ArenaTLS.cpp

+#include "utility/ArenaTLS.h"
+#include "system/util/Arena.h"
+
+namespace dds {
+namespace tls {
+
+thread_local Arena* arena = nullptr;
+
+} // namespace tls
+} // namespace dds


This file is duplicated with library/src/system/util/ArenaTLS.cpp. The duplication creates maintenance burden and potential inconsistencies.

Suggested change

#include "utility/ArenaTLS.h"

#include "system/util/Arena.h"

namespace dds {

namespace tls {

thread_local Arena* arena = nullptr;

} // namespace tls

} // namespace dds

// This file is deprecated. The implementation has been moved to:

// library/src/system/util/ArenaTLS.cpp

//

// Please update your includes to use the canonical implementation.

#include "system/util/ArenaTLS.cpp"

library/src/system/SolverContext.cpp

Copilot · 2025-10-13T15:57:55Z

library/src/moves/Moves.cpp

+        copyBuf = static_cast<moveType*>(p);
      }
+      if (copyBuf) {
+        std::memcpy(copyBuf, mply, static_cast<std::size_t>(numMoves) * sizeof(moveType));


The multiplication numMoves * sizeof(moveType) could potentially overflow if numMoves is very large. Consider checking for overflow or using a safer multiplication method.

Copilot · 2025-10-13T15:57:55Z

library/src/moves/Moves.cpp

+      // Try an arena-backed temporary copy buffer if a TLS allocator is set.
+      moveType* copyBuf = nullptr;
+      if (numMoves > 0) {
+        void* p = dds::tls::TryAlloc(static_cast<std::size_t>(numMoves) * sizeof(moveType), alignof(moveType));


Similar to the memcpy issue, this multiplication could overflow if numMoves is extremely large. The cast to std::size_t doesn't prevent overflow from the multiplication itself.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull Request Overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-13T16:06:55Z

library/src/utility/ArenaTLS.cpp

+#include "utility/ArenaTLS.h"
+#include "system/util/Arena.h"
+
+namespace dds {
+namespace tls {
+
+thread_local Arena* arena = nullptr;
+
+} // namespace tls
+} // namespace dds


This file appears to be a duplicate of library/src/system/util/ArenaTLS.cpp with identical content. Having duplicate implementation files can lead to linker errors or maintenance confusion.

library/src/system/SolverContext.cpp

Copilot · 2025-10-13T16:06:56Z

library/src/system/SolverContext.cpp

  const pos& tpos)
 {
-  return thr_->moves.MoveGen123(tricks, relHand, tpos);
+  SolverContext ctx(thr_);


Creating a new SolverContext instance inside MoveGenContext::MoveGen123 introduces unnecessary overhead since the parent context already exists. Consider passing the parent context or accessing its arena directly.

Copilot

Pull Request Overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-13T16:15:41Z

library/src/moves/Moves.cpp

+        copyBuf = static_cast<moveType*>(p);
      }
+      if (copyBuf) {
+        std::memcpy(copyBuf, mply, static_cast<std::size_t>(numMoves) * sizeof(moveType));


Same potential integer overflow issue as line 1000. The multiplication should be validated for overflow before use in memcpy.

…/123 TLS shim - Pass ThreadData* directly to the TLS allocator shim to access per-thread Arena - Keeps behavior identical while reducing redundant object construction

Copilot

Pull Request Overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-13T16:29:53Z

library/src/system/SolverContext.cpp

  const moveType& bestMoveTT,
  const relRanksType thrp_rel[])


Creating a new SolverContext instance for each MoveGen call is inefficient. Consider passing the existing context or using a lighter-weight approach to access the arena.

Copilot · 2025-10-13T16:29:53Z

library/src/system/SolverContext.cpp

  const pos& tpos)
 {


Duplicate SolverContext construction in MoveGen123. This pattern repeats the same inefficiency as in MoveGen0.

zzcgumn added 5 commits October 13, 2025 14:31

Copilot AI review requested due to automatic review settings October 13, 2025 15:56

Copilot AI reviewed Oct 13, 2025

View reviewed changes

Update library/src/system/SolverContext.cpp

f1457b9

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings October 13, 2025 16:06

Copilot AI reviewed Oct 13, 2025

View reviewed changes

Deletes dead code.

f9ba853

Copilot AI review requested due to automatic review settings October 13, 2025 16:14

Copilot AI reviewed Oct 13, 2025

View reviewed changes

zzcgumn force-pushed the refactor/dynamic_environment_part_thirteen branch from 1a18029 to f9ba853 Compare October 13, 2025 16:25

perf(movegen): avoid temporary SolverContext construction in MoveGen0…

f415813

…/123 TLS shim - Pass ThreadData* directly to the TLS allocator shim to access per-thread Arena - Keeps behavior identical while reducing redundant object construction

Copilot AI review requested due to automatic review settings October 13, 2025 16:28

Copilot AI reviewed Oct 13, 2025

View reviewed changes

zzcgumn self-assigned this Oct 13, 2025

zzcgumn merged commit fcae0c6 into mn_main Oct 13, 2025
3 checks passed

zzcgumn deleted the refactor/dynamic_environment_part_thirteen branch October 13, 2025 16:36

Conversation

zzcgumn commented Oct 13, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants