Skip to content
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.

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

Leak a little less memory with dynamic allocations in Lua #5508

Closed
wants to merge 13 commits into from

Conversation

Mm2PL
Copy link
Collaborator

@Mm2PL Mm2PL commented Jul 13, 2024

This PR ensures that any potentially large objects will get deallocated on error instead of getting forgotten and leaked.

This would have leaked memory:

function cmd_leak()
    local LARGE = string.rep("12345", 100)
    for i = 1, 1000000 do
        pcall(c2.register_command, LARGE) -- this errors and the error is thrown away
    end
end
c2.register_command("/leak", cmd_leak)

Now it doesn't leak this memory.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@@ -69,14 +69,33 @@ StackIdx push(lua_State *L, const bool &b);
StackIdx push(lua_State *L, const int &b);
StackIdx push(lua_State *L, const api::CompletionEvent &ev);

struct PeekResult {
bool ok = true;
std::vector<QString> errorReason = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: initializer for member 'errorReason' is redundant [readability-redundant-member-init]

Suggested change
std::vector<QString> errorReason = {};
std::vector<QString> errorReason;

@Mm2PL Mm2PL force-pushed the chore/leak-less-memory-lua branch from 3e9e61f to d97b720 Compare July 22, 2024 14:04
@Mm2PL Mm2PL marked this pull request as ready for review September 2, 2024 18:29
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/controllers/plugins/LuaUtilities.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

* Helps you avoid accidentally dropping a pointer not the object behind it.
*/
template <typename T>
inline void drop(T * /*var*/) = delete;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: function 'drop' has inline specifier but is implicitly inlined [readability-redundant-inline-specifier]

Suggested change
inline void drop(T * /*var*/) = delete;
void drop(T * /*var*/) = delete;

@Mm2PL Mm2PL requested a review from pajlada September 2, 2024 19:39
@Nerixyz
Copy link
Contributor

Nerixyz commented Sep 3, 2024

You're invoking undefined behavior in many cases by using luaL_error which uses longjmp under the hood. On Windows, your approach can (and currently does; at least in the debug configuration) result in a double-free, as longjmp uses the same stack-unwinding as exceptions.

Speaking of exceptions... why not use them? At that point, it's probably best to use some bindings library (most (if not all) support incremental adoption). sol2 has a built-in trampoline. LuaBridge3 doesn't seem to have anything like it. There are probably others with support for that. I tried to convert the channel-api to use sol2 in https://github.com/Nerixyz/chatterino2/tree/experiment/sol3 (Nerixyz@45b7c0a) which has, despite adding adaptions for Qt types, more deletions than additions.

@Mm2PL
Copy link
Collaborator Author

Mm2PL commented Sep 3, 2024

Actually lua has an exception backend which could be used if we compiled it as c++ instead of c. This would then call destructors i think?

@Mm2PL
Copy link
Collaborator Author

Mm2PL commented Sep 3, 2024

I took a look at the sol this. And not having to do the SharedUserData is appealing but I'm not sure that it's appealing enough to rip everything out and convert it to sol.

@Mm2PL
Copy link
Collaborator Author

Mm2PL commented Sep 3, 2024

image
Closing in favor of moving everything to sol.

@Mm2PL Mm2PL closed this Sep 3, 2024
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