AddLog delays player reference GC in a lot of cases (more of a question than a report)

[JaksonD]

Version channel

Stable (Default)

Loader version

No response

What part of Adonis is this related to?

Other

What happened?

Currently, there's an issue I'm having with Adonis where a large amount of places that call AddLog pass the player into the log as well. This results in the server holding onto the player reference for longer than it should; major issue in servers with large playercounts and high turnover rate.

I understand some things probably use the player (probably plugins) but nothing in base Adonis seems to, so I attempted to alleviate this issue for my own game by overriding Logs.AddLog entirely using a plugin and erasing the player field of the table, but ran into another issue; most places that use AddLog save it to a variable on init, preventing my changes from mattering as they have the old version of the function (before my function overwrote it).

So, given the circumstances, I wanted to ask for how to go forward in a case like this.
Should I open a PR to unlocalize all instances of AddLog?
Should this be a config option? (storing the player obj in the first place)
Or something else? Originally I was going to submit a PR to remove ever storing the player obj in the log, but realized that would likely break plugins

Steps to reproduce

1. Have someone do anything that causes a log output for their player
2. Have them leave
3. Check in server LuauHeap -> Unique References
4. Observe the 50 references to the player object

Device

Windows

Relevant log output

[ccuser44]

Yes. This is moreso an unintended side effect on how player logs are handled than a bug.

This will still likely be fixed sometime when we rewrite how player logs are stored.

The goods news is that settings.MaxLogs limits the theoretical maximum amount of references to players to MaxLogs * PlrLogTypes meaning in the mean time lowering settings.MaxLogs is an effective mitigation tactic for the memory leaks.

[JaksonD]

Yes. This is moreso an unintended side effect on how player logs are handled than a bug.

This will still likely be fixed sometime when we rewrite how player logs are stored.

The goods news is that settings.MaxLogs limits the theoretical maximum amount of references to players to MaxLogs * PlrLogTypes meaning in the mean time lowering settings.MaxLogs is an effective mitigation tactic for the memory leaks.

Any possible fix in the short term? Or do I have to fork Adonis entirely? I line out why a plugin won't work for fixing this, and I'd prefer to have the normal max log amount

[ccuser44]

Yes. This is moreso an unintended side effect on how player logs are handled than a bug.

This will still likely be fixed sometime when we rewrite how player logs are stored.

The goods news is that settings.MaxLogs limits the theoretical maximum amount of references to players to MaxLogs * PlrLogTypes meaning in the mean time lowering settings.MaxLogs is an effective mitigation tactic for the memory leaks.

Any possible fix in the short term? Or do I have to fork Adonis entirely? I line out why a plugin won't work for fixing this, and I'd prefer to have the normal max log amount

Yes. You can add a plugin with the following source code. @JaksonD

return function(Vargs)
local server, service = Vargs.Server, Vargs.Service

local Logs = server.Logs
local n = 0

service.Events.LogAdded:Connect(function(_, log)
if log.Player then
log.Player = nil
n += 1
end
end)

for k in pairs(Logs.IndToName) do
local v = Logs[k]
if v.__meta == "DLL" or getmetatable(v) and getmetatable(v).__meta == "DLL" then
local log = v.snode
while log do
if log.Player then
log.Player = nil
n += 1
end
log = log.next
end
end
else
for _, log in ipairs(v) do
if log.Player then
log.Player = nil
n += 1
end
end
end

Logs:AddLog("Script", `Removed {n} player references from logs.`)
end

[JaksonD]

I can't believe I didn't think of listening to the LogAdded event and removing it from there. Thank you so much!
I'll leave the issue open in case you want to track this for that future log rewrite, but this does solve my issue for now. Cheers

[kaiserandaxl]

@JaksonD

Sorry this is a little late, I'll make a PR to change the behavior of the Function to remove the reference after adding the log.