Reduce segment copies #4578
Reduce segment copies #4578
Conversation
WalkthroughThe pull request modifies segment comparison and management by removing the Changes
Possibly related PRs
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
wled00/json.cpp (1)
18-44: Good implementation of segment comparison without requiring copies.This new standalone
differsfunction provides a more efficient way to compare segments by working directly with memory representations instead of creating full copies of the segment objects. The implementation carefully checks each relevant property of the segment.Some observations:
- Line 30 duplicates the palette check from line 27
- The bitmask
0b1111111111011110Ushould have a comment explaining which bits are excluded from the comparison- if (segment.palette != segmentBackup.palette) return true;wled00/FX_fcn.cpp (1)
1766-1766: Optimize segment construction with move semantics.This change optimizes the
resetSegments()method by using move semantics to transfer ownership of the segment object rather than creating a potentially expensive copy. This is particularly efficient sinceSegmentobjects might contain dynamically allocated resources.wled00/led.cpp (2)
31-43: Improved efficiency by caching segment properties.The refactored code now uses constant references to store the properties of the first selected segment, rather than repeatedly accessing them during comparisons. This reduces function calls and potential memory operations.
44-52: Simplified loop logic with direct reference comparison.The segment selection logic is more efficient by directly comparing references rather than calling
isActive()andisSelected()for segments other than the first one. This addresses the issue mentioned in the comment on line 28, ensuring all selected segments will be updated regardless of the first segment's values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wled00/FX.h(1 hunks)wled00/FX_fcn.cpp(1 hunks)wled00/json.cpp(3 hunks)wled00/led.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: wled_build / Build Enviornments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Enviornments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Enviornments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Enviornments (lolin_s2_mini)
- GitHub Check: wled_build / Build Enviornments (esp32_eth)
- GitHub Check: wled_build / Build Enviornments (esp32dev_V4)
- GitHub Check: wled_build / Build Enviornments (esp32dev)
- GitHub Check: wled_build / Build Enviornments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Enviornments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Enviornments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Enviornments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Enviornments (esp8266_2m_160)
- GitHub Check: wled_build / Build Enviornments (nodemcuv2_160)
- GitHub Check: wled_build / Build Enviornments (esp01_1m_full)
- GitHub Check: wled_build / Build Enviornments (esp8266_2m)
- GitHub Check: wled_build / Build Enviornments (nodemcuv2)
🔇 Additional comments (3)
wled00/FX.h (1)
885-885: Excellent move semantics optimization!Changing the method to accept an rvalue reference (
Segment &&) and usingstd::move()eliminates unnecessary copying when appending segments. This allows the compiler to use move operations which are more efficient for complex objects likeSegment.wled00/json.cpp (2)
65-66: Efficient backup using memcpy instead of copy construction.Using
std::memcpyto create a byte-level copy of the segment is more efficient than using the copy constructor, especially when the only purpose is to check for differences later. This avoids running constructors and destructors unnecessarily.
325-325: Using the new comparison function to detect changes.This line replaces the previous method call to
seg.differs(prev)with the new standalone function. This change maintains the same functionality while benefiting from the optimized comparison approach that doesn't require a full segment copy.
- Avoid calling Segment copy constructor and destructor. This is fine as long as we only read primitive values. - Only diff stuff we are actually interested in. - Only diff stuff if it has an effect.
TripleWhy
force-pushed
the
reduceSegmentCopies
branch
from
0c6df89 to
a0fc6bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
wled00/json.cpp (3)
18-44: Consider potential maintenance overhead of raw byte-level comparisons.
Thediffers()function usesreinterpret_castand compares fields individually, but introduces a risk ifSegmentgains non-trivial or non-POD members. Future changes toSegmentmight break this comparison silently. A safer approach could either rely on specialized comparisons or enforcestatic_assert(std::is_trivially_copyable<Segment>::value)to ensure data consistency.
64-66: Validate that direct memcpy ofSegmentremains valid.
Thealignas(Segment) std::array<byte, …>strategy works here, but ifSegmentbecomes more complex (e.g., if pointers or vtables are introduced), this backup mechanism can yield unpredictable comparisons. Consider adding a compile-time check or updating documentation to protect against non-trivial changes.
325-325: Be mindful of omitted fields or pointers in the change detection.
Usingdiffers(seg, segmentBackup)ignores some pointers (e.g.,nameor transient data pointers). If name changes should also triggerstateChanged, consider adding a comparison for those. Otherwise, it's consistent with the current bitmask approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wled00/FX.h(1 hunks)wled00/FX_fcn.cpp(1 hunks)wled00/json.cpp(3 hunks)wled00/led.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- wled00/FX.h
- wled00/led.cpp
🔇 Additional comments (1)
wled00/FX_fcn.cpp (1)
1766-1766: Great use of move semantics for segment insertion.
Usingstd::move(seg)eliminates unnecessary copies and helps optimize memory usage. The approach aligns well with the PR's objective to reduce overhead.
| @@ -28,21 +28,27 @@ void setValuesFromSegment(uint8_t s) | |||
| // problem: if the first selected segment already has the value to be set, other selected segments are not updated | |||
| void applyValuesToSelectedSegs() | |||
| { | |||
| // copy of first selected segment to tell if value was updated | |||
| unsigned firstSel = strip.getFirstSelectedSegId(); | |||
| Segment selsegPrev = strip.getSegment(firstSel); | |||
There was a problem hiding this comment.
This was an oversight in original. Const reference can (and should) be used. Fixed long time ago in my fork.
There was a problem hiding this comment.
Judging by the comments and how it is used, that was clearly intentional. Just using a reference instead of a copy won't help here. Except if the problem function comment is something that should be addressed, see PR description.
There was a problem hiding this comment.
Comment is off. There is no need to copy the segment as it si not modified in the for loop (see firstSel).
There was a problem hiding this comment.
The loop iterates over all segments. When i == firstSel, the segment is not skipped, so seg, which is selsegPrev at that point in time if you use a reference, is modified, is it not?
There was a problem hiding this comment.
if (i == firstSel || !(seg.isActive() && seg.isSelected())) continue; that's translated if. Isn't it?
At least it should be written as this.
There was a problem hiding this comment.
if (i != firstSel && (!seg.isActive() || !seg.isSelected())) continue;this is the unmodified line.
if you want to invert it, the continue would fall away or move into the else branch:
if (!(i != firstSel && (!seg.isActive() || !seg.isSelected()))) {
// the interesting stuff: modify seg
} else {
continue; // boring
} !(i != firstSel && (!seg.isActive() || !seg.isSelected()))
= !(i != firstSel) || !(!seg.isActive() || !seg.isSelected())
= (i == firstSel) || ( seg.isActive() && seg.isSelected())
So: firstSel get's modified, no matter if it's active or selected. Additionally, other segments get modified, if they are both active and selected at the same time.
There was a problem hiding this comment.
I'm not so sure about that: it looks very intentional, and the comments align with it, too.
Squashed commit of the following: commit c4b4bf0145bacad4e75d1c5321bf4a5da26b981c Author: Joachim Dick <[email protected]> Date: Sat Mar 1 18:43:47 2025 +0100 Avoid unnecessary copy of SegmentSettings Based on a comment from blazoncek in this PR: wled#4578 (comment) commit 74d83d87d8f302f1c264eded65b919838165ebb0 Author: Joachim Dick <[email protected]> Date: Sat Mar 1 18:20:17 2025 +0100 On Segment-move-only: Replace some _segments.push_back calls with _segments.emplace_back calls Originally from TripleWhy: TripleWhy@8efc2dd commit d8131e4c017f8d01f0f20b94c33f0f7d163afb94 Author: Joachim Dick <[email protected]> Date: Sat Mar 1 17:27:35 2025 +0100 Make Segment class non-copyable and move-only Co-Authored-By: TripleWhy <[email protected]> Co-Authored-By: Blaž Kristan <[email protected]>
Based on a comment from blazoncek in this PR: wled#4578 (comment)
| @@ -28,21 +28,27 @@ void setValuesFromSegment(uint8_t s) | |||
| // problem: if the first selected segment already has the value to be set, other selected segments are not updated | |||
There was a problem hiding this comment.
Let me be more explicit about the following question:
Is this problem actually a problem? If so, we could simplify a lot here by addressing it.
There was a problem hiding this comment.
That's the case. Consider:
setValuesFromSegment(strip.getFirstSelectedSegId());
...
applyValuesToSelectedSegs();That's due to the ifs in the function.
Why they were written as they are is beyond my knowing. Probably as some optimisation. I just kept original logic when updating.
If all ifs are removed, so is the problem. 😄
In any case this is legacy code, seldom used in new principles.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/json.cpp (1)
18-43: Consider including pointer fields in the comparison or clarify scope.While this byte-level comparison captures various geometry and color properties, it omits pointers such as
name,data, and_t. If pointer-based fields matter for detecting changes, consider expanding the comparison or documenting that these fields remain excluded from the diff check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wled00/FX.h(1 hunks)wled00/json.cpp(3 hunks)wled00/udp.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: wled_build / Build Enviornments (esp32_wrover)
- GitHub Check: wled_build / Build Enviornments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Enviornments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Enviornments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Enviornments (esp32c3dev)
- GitHub Check: wled_build / Build Enviornments (lolin_s2_mini)
- GitHub Check: wled_build / Build Enviornments (esp32_eth)
- GitHub Check: wled_build / Build Enviornments (esp32dev_V4)
- GitHub Check: wled_build / Build Enviornments (esp32dev)
- GitHub Check: wled_build / Build Enviornments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Enviornments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Enviornments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Enviornments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Enviornments (esp8266_2m_160)
- GitHub Check: wled_build / Build Enviornments (nodemcuv2_160)
- GitHub Check: wled_build / Build Enviornments (esp01_1m_full)
- GitHub Check: wled_build / Build Enviornments (esp8266_2m)
- GitHub Check: wled_build / Build Enviornments (nodemcuv2)
🔇 Additional comments (4)
wled00/udp.cpp (1)
275-275: Adopted perfect-forwarding to construct segments in place.Switching from the old
appendSegmenttoemplaceSegmentdirectly constructsSegmentobjects without unnecessary copies, aligning well with the PR's objective to reduce memory usage. Just be mindful of potential fragmentation if multiple segments are repetitively created and removed over time.wled00/json.cpp (2)
65-66: Verify shallow copy impact on pointer-based members.Cloning the
Segmentinto anstd::array<byte, sizeof(Segment)>can produce a shallow snapshot of pointer fields (e.g.,name,data). This is typically safe if you only track geometry/state changes, but please confirm that ignoring pointer modifications is intentional.
325-325: Logical consistency with newdiffersfunction.Using the free function
differs(seg, segmentBackup)is consistent with the removal ofSegment::differs. This helps consolidate comparison logic outside the struct and avoids extra overhead. The usage here looks correct.wled00/FX.h (1)
885-886: Efficient segment creation method.Introducing a variadic template for
emplaceSegmentensures segments are created in-place, reducing temporary copies and aligning with the PR objective to optimize memory usage. The boundary check on_segments.size() < getMaxSegments()also prevents out-of-range insertions.
Use move instead of copy where possible, avoid complexity/memory usage of full copies in other locations.
(Currently, this version would compile with deleted Segment copy functions.)
From the json/differs commit:
Note that the json copy code is effectively very similar to before, because the copy constructor also does a memcpy, plus additional work.
About
applyValuesToSelectedSegs:This could be simplified a lot, if we were to alter the behavior such that all selected segments would be modified unconditionally. Which would also address this function comment:
Summary by CodeRabbit
Refactor
New Features