stuff #156
stuff #156
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughIntroduces fast-path allocator APIs and thread-local pool access, switches frame/trampoline storage of Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Frame as Frame::operator new / delete
participant MemoryResource as memory_resource
participant Footer as FrameFooter (memcpy)
Caller->>Frame: allocate(size)
Frame->>MemoryResource: allocate(size + footer)
MemoryResource-->>Frame: block (footer reserved)
Frame->>Footer: store memory_resource* via std::memcpy
Caller->>Frame: use frame...
Caller->>Frame: delete / deallocate
Frame->>Footer: retrieve memory_resource* via std::memcpy
Frame->>MemoryResource: deallocate(total size) / deallocate_fast
MemoryResource-->>Frame: freed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
An automated preview of the documentation is available at https://156.capy.prtest3.cppalliance.org/index.html If more commits are pushed to the pull request, the docs will rebuild at the same URL. 2026-02-12 22:55:46 UTC |
|
GCOVR code coverage report https://156.capy.prtest3.cppalliance.org/gcovr/index.html Build time: 2026-02-12 23:04:41 UTC |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@example/allocation/allocation.cpp`:
- Around line 10-17: The header comment incorrectly says "20 million" while the
iterations variable (iterations) is set to 2000000 (2 million); update the
header text to "2 million" to match the actual value used (or alternatively
change the iterations variable to 20000000 if 20 million was intended) — locate
the descriptive comment at the top of allocation.cpp mentioning "20 million" and
either edit it to "2 million" or set the iterations variable named iterations to
20000000 so the comment and code agree.
🧹 Nitpick comments (3)
example/allocation/allocation.cpp (2)
30-30:counteris incremented but never read.The global
counteratomic is stored-to and fetch_add'd but its value is never printed or asserted. If it's meant to prevent the coroutine chain from being optimized away, abenchmark::DoNotOptimize-style sink or a final print would make the intent clearer. Otherwise it's dead code.Also applies to: 37-41
67-114: Benchmark runs may be biased by ordering.The recycling allocator always runs first, potentially paying cold-cache and JIT-like warm-up costs. Consider adding a short warm-up iteration (or running each benchmark twice and discarding the first) to reduce noise. This is an optional improvement for a benchmark example.
include/boost/capy/ex/recycling_memory_resource.hpp (1)
126-166: Fast-path allocation/deallocation logic is correct.The size-class computation, bucket pop/push, and fallback to
::operator new/deletefor oversized allocations are all sound. The unnamed alignment parameter is intentionally unused since power-of-2 size classes from::operator neware naturally aligned.One minor note: both methods (and
allocate_slow/deallocate_slow) only access static/thread-local state, neverthis. They could bestaticmember functions, which would make the API contract clearer and avoid the need for callers to downcast to call them. That said, keeping them as non-static is fine if you anticipate per-instance state in the future.
vinniefalco
force-pushed
the
develop
branch
3 times, most recently
from
e806140 to
d133786
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/ex/recycling_memory_resource.cpp`:
- Around line 55-65: do_allocate currently forwards the alignment to
allocate_fast which ignores it, meaning over-aligned requests can get only
default alignment; update the fallback/slow allocation path to honor requested
alignment by calling ::operator new(rounded, std::align_val_t{alignment}) when
alignment > __STDCPP_DEFAULT_NEW_ALIGNMENT__, and ensure allocate_fast is
changed to accept and propagate the alignment parameter (or have do_allocate
handle alignment and call a new allocate_slow(aligned) path); alternatively, if
you decide not to support over-alignment, add clear documentation on
recycling_memory_resource::do_allocate / allocate_fast that only default
alignment is guaranteed.
🧹 Nitpick comments (1)
src/ex/recycling_memory_resource.cpp (1)
10-14: Consider adding a high-level implementation overview comment.This file contains non-trivial allocation logic (thread-local + global two-tier pool, Peter Dimov bucket transfer, two-level virtual dispatch). A brief
/* */block comment after the include would help future maintainers quickly orient themselves.As per coding guidelines, "Files containing non-trivial implementation logic should include a
/* */block comment after the includes that provides a high-level overview of how the implementation works."
| void* | ||
| recycling_memory_resource::do_allocate(std::size_t bytes, std::size_t alignment) | ||
| { | ||
| return allocate_fast(bytes, alignment); | ||
| } | ||
|
|
||
| void | ||
| recycling_memory_resource::do_deallocate(void* p, std::size_t bytes, std::size_t alignment) | ||
| { | ||
| deallocate_fast(p, bytes, alignment); | ||
| } |
There was a problem hiding this comment.
Alignment parameter is accepted but effectively ignored.
do_allocate forwards to allocate_fast, which (per the header) takes the alignment parameter as unnamed and discards it. Since allocations are served from ::operator new(rounded) in the slow path, the returned pointer is only guaranteed to satisfy __STDCPP_DEFAULT_NEW_ALIGNMENT__ (typically 16 bytes). Callers requesting over-aligned allocations through the standard std::pmr::memory_resource::allocate(bytes, alignment) interface may receive under-aligned memory.
This is pre-existing in allocate_fast, but the new do_allocate wrapper now exposes it through the standard PMR interface where callers are more likely to rely on alignment guarantees.
Consider either:
- Documenting that this resource only supports default alignment, or
- Using
::operator new(rounded, std::align_val_t{alignment})on the fallback path when alignment exceeds the default.
🤖 Prompt for AI Agents
In `@src/ex/recycling_memory_resource.cpp` around lines 55 - 65, do_allocate
currently forwards the alignment to allocate_fast which ignores it, meaning
over-aligned requests can get only default alignment; update the fallback/slow
allocation path to honor requested alignment by calling ::operator new(rounded,
std::align_val_t{alignment}) when alignment > __STDCPP_DEFAULT_NEW_ALIGNMENT__,
and ensure allocate_fast is changed to accept and propagate the alignment
parameter (or have do_allocate handle alignment and call a new
allocate_slow(aligned) path); alternatively, if you decide not to support
over-alignment, add clear documentation on
recycling_memory_resource::do_allocate / allocate_fast that only default
alignment is guaranteed.
2 participants
Summary by CodeRabbit
Documentation
New Features / Examples
Performance / Bug Fixes
Refactor