Task 08: Utilities RNG + logging + stats#37
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a utilities infrastructure for the DDS solver with RNG, logging, and statistics capabilities. The implementation uses compile-time flags to optionally enable logging and statistics tracking without affecting default builds.
- Adds a header-only Utilities class providing RNG, simple logging buffer, and basic statistics
- Integrates utilities into SolverContext with deterministic RNG seeding via configuration
- Implements optional compile-time logging and statistics tracking for TransTable lifecycle events
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| library/src/system/util/Utilities.h | Header-only utilities class with RNG, logging, and stats |
| library/src/system/SolverContext.h | Integration of utilities with facade pattern and RNG seeding |
| library/src/system/SolverContext.cpp | TransTable lifecycle logging and stats tracking with compile flags |
| library/src/system/BUILD.bazel | Build variants with optional DDS_UTILITIES_LOG and DDS_UTILITIES_STATS |
| library/src/BUILD.bazel | Testable library variants with utilities features enabled |
| library/tests/system/*.cpp | Test files validating logging and stats behavior with/without defines |
| library/tests/utility/rng_determinism_test.cpp | RNG determinism validation test |
| library/tests/system/BUILD.bazel | Test targets for utilities features |
| library/tests/utility/BUILD.bazel | Test target for RNG determinism |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| #ifdef DDS_UTILITIES_LOG | ||
| // Append a tiny debug entry indicating TT creation and chosen kind/sizes. | ||
| { | ||
| char buf[96]; |
There was a problem hiding this comment.
Magic number 96 for buffer size should be defined as a named constant to improve maintainability and make the buffer size choice explicit.
library/src/system/SolverContext.h
Outdated
| inline UtilitiesContext utilities() { return UtilitiesContext(&utils_); } | ||
| inline UtilitiesContext utilities() const { return UtilitiesContext(const_cast<::dds::Utilities*>(&utils_)); } |
There was a problem hiding this comment.
Using const_cast in a const method violates const-correctness. Consider providing separate const and non-const versions of UtilitiesContext or redesigning the interface to avoid const_cast.
| inline UtilitiesContext utilities() { return UtilitiesContext(&utils_); } | |
| inline UtilitiesContext utilities() const { return UtilitiesContext(const_cast<::dds::Utilities*>(&utils_)); } | |
| class ConstUtilitiesContext { | |
| public: | |
| explicit ConstUtilitiesContext(const ::dds::Utilities* util) : util_(util) {} | |
| const ::dds::Utilities& util() const { return *util_; } | |
| const std::mt19937& rng() const { return util_->rng(); } | |
| const std::vector<std::string>& logBuffer() const { return util_->log_buffer(); } | |
| private: | |
| const ::dds::Utilities* util_ = nullptr; | |
| }; | |
| inline UtilitiesContext utilities() { return UtilitiesContext(&utils_); } | |
| inline ConstUtilitiesContext utilities() const { return ConstUtilitiesContext(&utils_); } |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 13 out of 13 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.
library/src/system/SolverContext.cpp
Outdated
| const_cast<SolverContext*>(this)->utilities().logAppend(std::string(buf)); | ||
| } | ||
| #endif | ||
|
|
||
| #ifdef DDS_UTILITIES_STATS | ||
| const_cast<SolverContext*>(this)->utilities().util().stats().tt_creates++; |
There was a problem hiding this comment.
The const_cast pattern is repeated multiple times throughout this file. Consider making the utilities() method and related logging/stats operations non-const, or provide a mutable utilities accessor to avoid the repeated const_cast which makes the code harder to maintain and understand.
| const_cast<SolverContext*>(this)->utilities().logAppend(std::string(buf)); | |
| } | |
| #endif | |
| #ifdef DDS_UTILITIES_STATS | |
| const_cast<SolverContext*>(this)->utilities().util().stats().tt_creates++; | |
| utilities().logAppend(std::string(buf)); | |
| } | |
| #endif | |
| #ifdef DDS_UTILITIES_STATS | |
| utilities().util().stats().tt_creates++; |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 19 out of 19 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.
| char buf[96]; | ||
| const char kch = (kind == TTKind::Small ? 'S' : 'L'); | ||
| std::snprintf(buf, sizeof(buf), "tt:create|%c|%d|%d", kch, defMB, maxMB); |
There was a problem hiding this comment.
Magic number 96 for buffer size should be documented or replaced with a named constant to explain why this size was chosen.
| char buf[64]; | ||
| std::snprintf(buf, sizeof(buf), "tt:resize|%d|%d", defMB, maxMB); |
There was a problem hiding this comment.
Magic number 64 for buffer size should be documented or replaced with a named constant. Consider using the same buffer size as the tt:create logging for consistency.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 20 out of 20 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.
| { | ||
| #ifdef DDS_UTILITIES_LOG | ||
| { | ||
| char buf[64]; |
There was a problem hiding this comment.
Magic number 64 should be defined as a named constant to clarify the buffer size requirement and improve maintainability.
… buffer; seed via SolverConfig - Introduce header-only dds::Utilities (mt19937 RNG + simple log buffer) under system/util/. - Extend SolverConfig with rngSeed and wire seeding in SolverContext constructors. - Expose Utilities through SolverContext::utilities() facade with rng() and log helpers. - Export Utilities headers via //library/src/system build target. - Add deterministic RNG unit test and BUILD target in library/tests/utility; depend on testable_dds. This is Task 08, slice 1 toward moving utilities (RNG/logging/stats) into instance-scoped context. No behavior change for solver logic.
… tests - Introduce header-only Utilities (rng + log buffer) already wired in SolverContext. - Under DDS_UTILITIES_LOG, append small TT lifecycle entries (create/dispose) to Utilities log buffer. - Add two tests: * utilities_log_test: asserts no logging without define * utilities_log_test_with_define: asserts presence of create/dispose entries when enabled - Provide system_util_log and testable_dds_util_log build variants to enable logging in tests without affecting default builds. Part of Task 08: Utilities migration (slice: logging).
…tests - Extend Utilities with a tiny Stats struct (tt_creates, tt_disposes) + reset accessor. - Under DDS_UTILITIES_STATS, increment counters on TT create/dispose in SolverContext. - Add tests verifying counters are zero by default and increment when enabled. - Provide system/testable_dds variants compiled with DDS_UTILITIES_STATS for targeted tests. Continues Task 08: Utilities migration (slice: stats).
- Add concise log entries for ResetForSolve, ClearTT, ResizeTT, and ResetBestMovesLite - Keeps logging behind DDS_UTILITIES_LOG; default builds unaffected
- Add utilities_log_ctx_ops_test_with_define to verify ResetForSolve, ResetBestMovesLite, ResizeTT, ClearTT log entries when logging is enabled - Uses logging-enabled testable_dds variant; default builds unaffected
- utilities_log_ctx_ops_test: exercises ResetForSolve, ResetBestMovesLite, ResizeTT, ClearTT and expects empty log buffer without DDS_UTILITIES_LOG
…entry - Add README section for Utilities RNG seeding and compile-time flags (DDS_UTILITIES_LOG/DDS_UTILITIES_STATS) - Prepend Unreleased ChangeLog notes summarizing Task 06/07 and Task 08 Utilities slices - Add Utilities::log_enabled()/stats_enabled() helpers and tests (enabled/disabled variants) - Wire new tests in system BUILD; verify via Bazel test targets
- Provide Utilities::log_size() for quick assertions on log entries - Provide Utilities::stats_snapshot() to copy current counters - Both helpers are compile-guard safe and no-op-friendly
- Adds Utilities::log_contains() for simple prefix checks in tests - Adds utilities_log_contains_test to validate behavior - Keeps logging optional; helper works regardless of compile-time flags
- Mark SolverContext::utils_ as mutable and return a non-const UtilitiesContext from utilities() const - Replace repeated const_cast usages in SolverContext.cpp with direct utilities() calls - No behavior change; simplifies logging/stats call sites and maintainability
6e0b6b8 to
12e851a
Compare
This PR delivers the first Utilities slices for Task 08:\n\n- Utilities sub-context in SolverContext with per-instance RNG seeded via SolverConfig.rngSeed.\n- Header-only Utilities class (rng + simple log buffer), exported under system/util.\n- Compile-time optional logging routed through Utilities (DDS_UTILITIES_LOG):\n - Emits compact entries for TransTable lifecycle (create/dispose) from SolverContext.\n - Tests: utilities_log_test (disabled: empty) and utilities_log_test_with_define (enabled: entries present) using system/testable_dds variants.\n- Compile-time optional stats (DDS_UTILITIES_STATS):\n - Utilities::Stats tracks tt_creates/tt_disposes; incremented from SolverContext lifecycle.\n - Tests: utilities_stats_test (disabled: zeros) and utilities_stats_test_with_define (enabled: increments) using system/testable_dds variants.\n\nBuild & tests\n- Bazel builds and all tests pass locally on macOS (dbg).\n- New tests are isolated behind build variants so default builds are unaffected.\n\nNotes\n- Behavior unchanged by default; flags only enabled in dedicated test targets.\n- Sets up groundwork for migrating more utilities in future slices (e.g., wider logging, additional stats).