Conversation
…eutrality - Lazily initialize Utilities RNG to avoid per-context std::mt19937 construction when unused - Confirms baseline timings restored; deterministic seeding preserved
There was a problem hiding this comment.
Pull Request Overview
Performance follow-up focusing on reducing unnecessary initialization overhead in Utilities and adding a regression test for logging behavior.
- Lazy-initialize std::mt19937 in Utilities to avoid default construction cost when RNG is unused.
- Add a system test verifying that log snapshots are independent of subsequent mutations.
- Update ChangeLog to document the RNG initialization change.
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| library/src/system/util/Utilities.h | Switch rng_ to lazy-initialized storage and update accessors/seed to instantiate on demand. |
| library/tests/system/utilities_log_snapshot_test.cpp | New test ensuring log_snapshot returns an independent copy. |
| ChangeLog | Note about lazy initialization of RNG and preserved deterministic seeding. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
library/src/system/util/Utilities.h
Outdated
|
|
||
| private: | ||
| std::mt19937 rng_{}; // default-constructed; seed via seed() | ||
| mutable std::unique_ptr<std::mt19937> rng_; // created on demand |
There was a problem hiding this comment.
Switching from std::mt19937 to std::unique_ptrstd::mt19937 makes Utilities non-copyable (copy ctor/assignment are implicitly deleted), which is a breaking change if callers relied on copying Utilities. Consider preserving copy semantics by either implementing an explicit deep-copy copy constructor and copy assignment operator (cloning the engine when present) or using a copyable optional-like storage (e.g., std::optionalstd::mt19937) to keep lazy initialization without removing copyability; alternatively, document and enforce non-copyable semantics if that’s intentional.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| std::mt19937& rng() { | ||
| if (!rng_) rng_ = std::make_unique<std::mt19937>(); | ||
| return *rng_; | ||
| } | ||
| const std::mt19937& rng() const { | ||
| if (!rng_) rng_ = std::make_unique<std::mt19937>(); | ||
| return *rng_; | ||
| } |
There was a problem hiding this comment.
Switching from a value member to std::unique_ptr makes Utilities non-copyable (copy ctor/assignment are implicitly deleted). If Utilities was previously copyable, this is a breaking change. To preserve copy semantics and still be lazy, consider std::optionalstd::mt19937 rng_; and emplace on first use, or explicitly define copy ctor/assignment to deep-copy the engine (copy mt19937 state) and default the move operations.
| std::mt19937& rng() { | ||
| if (!rng_) rng_ = std::make_unique<std::mt19937>(); | ||
| return *rng_; | ||
| } | ||
| const std::mt19937& rng() const { | ||
| if (!rng_) rng_ = std::make_unique<std::mt19937>(); | ||
| return *rng_; | ||
| } |
There was a problem hiding this comment.
[nitpick] The lazy-init logic is duplicated in both overloads. Factor into a small helper (e.g., ensure_rng()) to avoid divergence and ease future changes.
| const std::mt19937& rng() const { | ||
| if (!rng_) rng_ = std::make_unique<std::mt19937>(); | ||
| return *rng_; | ||
| } |
There was a problem hiding this comment.
[nitpick] The const accessor performs lazy initialization (via mutable) and can allocate; this is a side effect in a const method. Please document this behavior on the accessor to set expectations for callers.
No description provided.