Skip to content

Conversation

@jschall
Copy link
Contributor

@jschall jschall commented Dec 3, 2025

No description provided.

Copy link
Contributor

@tpwrules tpwrules left a comment

Choose a reason for hiding this comment

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

I don't understand the motivation. This is technically more correct, but our allocator does not care about the size on free (though Lua does internally somewhat). So there is no memory leak that it fixes.

Please help me understand the problem, or adjust the commit message to clarify there is no actual leak. E.g. "AP_Scripting: fix Lua dataflash logging to correct garbage collector accounting". The actual issue here is minor.

@jschall
Copy link
Contributor Author

jschall commented Dec 3, 2025

Before change, memory use grew linearly over time.

After change, memory use was constant.

I'm not just throwing stuff at the wall. This is correct.

@tpwrules
Copy link
Contributor

tpwrules commented Dec 3, 2025

This PR indeed fixes an error in the accounting which might create that impression, but as far as I can tell there is no actual leak and I would like to confirm. How did you check?

@jschall
Copy link
Contributor Author

jschall commented Dec 3, 2025

By setting SCR_DEBUG_OPTS to 2 and watching memory increase, and then making this change and watching memory not increase.

@tpwrules
Copy link
Contributor

tpwrules commented Dec 3, 2025

Yes, that would show the accounting bug. But there is still no actual leak that I can tell.

In any case, there are several other places in this function that call luaM_free that also need a fix, so this PR needs revision.

Consider instead refactoring to use luaL_Buffer and luaL_buffinitsize. Then you can get rid of all the free calls, it is safe to abandon it.

@tpwrules
Copy link
Contributor

tpwrules commented Dec 5, 2025

In fact, please just make it a static 256 byte buffer. We can't have a message longer than that. Well we could, but that is a pre-existing bug that is broken with this.

@tpwrules tpwrules closed this Dec 5, 2025
@tpwrules tpwrules reopened this Dec 5, 2025
@tpwrules
Copy link
Contributor

tpwrules commented Dec 5, 2025

Wrong button...

@tpwrules
Copy link
Contributor

Thanks for reporting this. Should be fixed in that other PR.

@tpwrules tpwrules closed this Dec 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants