Skip to content

Bugfixes in FX data allocation #4783

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 23, 2025
Merged

Bugfixes in FX data allocation #4783

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1960066
Bugfixes in FX data allocation
DedeHai Jul 19, 2025
04c9c93
only allow realloc() for pixelbuffer if it grows + bugfix
DedeHai Jul 19, 2025
633eb04
reverting realloc() block as it was cleaner before
DedeHai Jul 19, 2025
3ce0cc7
added realloc_malloc functions, increased min heap constant
DedeHai Jul 20, 2025
716da32
adding missing function declare
DedeHai Jul 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 20 additions & 12 deletions wled00/FX_fcn.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,17 @@ bool Segment::allocateData(size_t len) {
return false;
}
// prefer DRAM over SPI RAM on ESP32 since it is slow
if (data) data = (byte*)d_realloc(data, len);
else data = (byte*)d_malloc(len);
if (data) {
void* newData = d_realloc(data, len); // note: realloc returns null if it fails but does not free the original pointer
if (!newData || newData != data) // realloc failed or used a new memory block causing fragmentation. free and allocate new block
d_free(newData);
d_free(data);
Copy link
Member

@willmmiles willmmiles Jul 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to free the old buffer, realloc() has already free()d it for you.

Copy link
Member

@willmmiles willmmiles Jul 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(unless it returned null)

Copy link
Collaborator Author

@DedeHai DedeHai Jul 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but only if it does not fail, i.e. if newData is valid. Or ist that incorrect?

Copy link
Member

@willmmiles willmmiles Jul 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

But if realloc did allocate a new buffer for you, there's no point in releasing it and trying again; malloc could just give you the new buffer back, since there wasn't enough space to extend the old one.

Copy link
Collaborator Author

@DedeHai DedeHai Jul 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right, I had an error in my thought process thinking that if a new segment is created and the old data copied it would help fragmentation if deleting all and allocating new but that is exactly what realloc() actually does.

data = nullptr; // reset pointer to null
Segment::addUsedSegmentData(-_dataLen); // subtract original buffer size
_dataLen = 0; // reset data length
}
else data = (byte*)d_malloc(len);

if (data) {
memset(data, 0, len); // erase buffer
Segment::addUsedSegmentData(len - _dataLen);
Expand All @@ -170,7 +179,6 @@ bool Segment::allocateData(size_t len) {
}
// allocation failed
DEBUG_PRINTLN(F("!!! Allocation failed. !!!"));
Segment::addUsedSegmentData(-_dataLen); // subtract original buffer size
errorFlag = ERR_NORAM;
return false;
}
Expand Down Expand Up @@ -449,8 +457,8 @@ void Segment::setGeometry(uint16_t i1, uint16_t i2, uint8_t grp, uint8_t spc, ui
}
// re-allocate FX render buffer
if (length() != oldLength) {
if (pixels) pixels = static_cast<uint32_t*>(d_realloc(pixels, sizeof(uint32_t) * length()));
else pixels = static_cast<uint32_t*>(d_malloc(sizeof(uint32_t) * length()));
if (pixels) d_free(pixels); //pixels = static_cast<uint32_t*>(d_realloc(pixels, sizeof(uint32_t) * length())); // using realloc can cause additional fragmentation instead of reducing it
Copy link
Collaborator

@blazoncek blazoncek Jul 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a misleading comment.

Copy link
Collaborator Author

@DedeHai DedeHai Jul 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is what I have seen in tests: when using realloc, fragmentation was worse. When using free/malloc it was somewhat better but both approaches do cause fragmentation. The idea behind this is that free + malloc allows it to fill gaps left behind.
Got any test scenario that is good to test which works better in practice?

Copy link
Collaborator

@blazoncek blazoncek Jul 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sole purpose of realloc() functions is to reduce fragmentation. free + malloc will only fill gaps if garbage collector did its job. Unfortunately I have no knowledge of MM on ESP platforms.

IIRC @softhack007 was having a lot of issues with memory fragmentation in the past but I no longer remember what was the outcome (except that FX memory is no longer released and it only grows once allocated).

Copy link
Collaborator Author

@DedeHai DedeHai Jul 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is zero garbage collection, its first come, first serve from the bottom up.
so if a block of memory divides a larger block, free -> malloc should move it to the bottom, realloc will keep it there if it fits.
Using realloc can prevent fragmentation but it can also make it worse if large buffers are at play. At least that is my understanding and also what I have seen in tests.

Copy link
Collaborator

@blazoncek blazoncek Jul 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then go a step further and simplify p_realloc and d_realloc to use p_free/p_malloc and d_free/d_malloc instead.
It is the cleanest solution.
However ESP8266 will need the same.

pixels = static_cast<uint32_t*>(d_malloc(sizeof(uint32_t) * length()));
if (!pixels) {
DEBUG_PRINTLN(F("!!! Not enough RAM for pixel buffer !!!"));
errorFlag = ERR_NORAM_PX;
Expand Down Expand Up @@ -563,8 +571,8 @@ Segment &Segment::setName(const char *newName) {
if (newName) {
const int newLen = min(strlen(newName), (size_t)WLED_MAX_SEGNAME_LEN);
if (newLen) {
if (name) name = static_cast<char*>(d_realloc(name, newLen+1));
else name = static_cast<char*>(d_malloc(newLen+1));
if (name) d_free(name); // free old name
name = static_cast<char*>(d_malloc(newLen+1));
if (name) strlcpy(name, newName, newLen+1);
name[newLen] = 0;
return *this;
Expand Down Expand Up @@ -729,8 +737,8 @@ void IRAM_ATTR_YN Segment::setPixelColor(int i, uint32_t col) const
}
break;
case M12_pCorner:
for (int x = 0; x <= i; x++) setPixelColorRaw(XY(x, i), col);
for (int y = 0; y < i; y++) setPixelColorRaw(XY(i, y), col);
for (int x = 0; x <= i; x++) setPixelColorXY(x, i, col); // note: <= to include i=0. Relies on overflow check in sPC()
for (int y = 0; y < i; y++) setPixelColorXY(i, y, col);
break;
case M12_sPinwheel: {
// Uses Bresenham's algorithm to place coordinates of two lines in arrays then draws between them
Expand Down Expand Up @@ -1189,8 +1197,8 @@ void WS2812FX::finalizeInit() {
deserializeMap(); // (re)load default ledmap (will also setUpMatrix() if ledmap does not exist)

// allocate frame buffer after matrix has been set up (gaps!)
if (_pixels) _pixels = static_cast<uint32_t*>(d_realloc(_pixels, getLengthTotal() * sizeof(uint32_t)));
else _pixels = static_cast<uint32_t*>(d_malloc(getLengthTotal() * sizeof(uint32_t)));
if (_pixels) d_free(_pixels);
_pixels = static_cast<uint32_t*>(d_malloc(getLengthTotal() * sizeof(uint32_t)));
DEBUG_PRINTF_P(PSTR("strip buffer size: %uB\n"), getLengthTotal() * sizeof(uint32_t));

DEBUG_PRINTF_P(PSTR("Heap after strip init: %uB\n"), ESP.getFreeHeap());
Expand Down Expand Up @@ -1677,7 +1685,7 @@ void WS2812FX::setTransitionMode(bool t) {

// wait until frame is over (service() has finished or time for 1 frame has passed; yield() crashes on 8266)
void WS2812FX::waitForIt() {
unsigned long maxWait = millis() + getFrameTime();
unsigned long maxWait = millis() + getFrameTime() + 100; // TODO: this needs a proper fix for timeout!
while (isServicing() && maxWait > millis()) delay(1);
#ifdef WLED_DEBUG
if (millis() >= maxWait) DEBUG_PRINTLN(F("Waited for strip to finish servicing."));
Expand Down