Appearances arn't being cleaned up when no longer in use.

ID:2492560
 
Aug 3 2019, 1:19 am (Edited on Aug 3 2019, 2:38 am)
Resolved
Temporary mutable appearances used in animate() didn't clean up properly, resulting in an appearance leak.
BYOND Version:512.1470 and 512.1475
Operating System:Windows 10 Pro 64-bit
Web Browser:Chrome 75.0.3770.142
Applies to:Dream Daemon
Status: Resolved (512.1476)
This issue has been resolved.
Descriptive Problem Summary:
Appearances arn't being cleaned up when no longer in use.
Numbered Steps to Reproduce Problem:
Run in Dream Daemon, connect a mob to the world, and watch the appearance count grow out of control.
Code Snippet (if applicable) to Reproduce Problem:
mob
    Login()
        ..()
        bugproof()

    proc
        bugproof()
            set waitfor = 0
            while(src)
                color = rgb(rand(0,255),rand(0,255),rand(0,255))
                sleep(world.tick_lag)

Open the link > Top right Download button.
Test case 1 - https://www.dropbox.com/s/rv0qs8tvbw9rf9n/ ApperanceBug.rar?dl=0 - Ter's Test case.
Test case 2 - https://www.dropbox.com/s/p3bikqy97u7ia7y/ AnimateTest.rar?dl=0 - My own Test case.
Test case 3 - https://www.dropbox.com/s/gqmnsl5n9484lhe/ ApperanceBug2.rar?dl=0 - Luber's Test case(It's not realiable and instnatly reproducable, since we don't know yet what exactly is triggering it, mostly playing around with 2 verbs and checking appearance counts + changing the appearance when you move), also using older version, results my differ on current version.
Expected Results:
Memory being freed up
Actual Results:
Memory explodes and crashes your DD, if you leave it out of control.
Disconnecting from DD as a user, will not free up memory either.

Does the problem occur:
Every time? Or how often?
Every Time
In other games?
Probably all are effected
In other user accounts?
Sure
On other computers?
Sure

When does the problem NOT occur?
Always occures
Did the problem NOT occur in any earlier versions? If so, what was the last version that worked? (Visit http://www.byond.com/download/build to download old versions for testing.)

Haven't tested beyond versions 512.1470 and 512.1475

Workarounds:
Don't think there is one.

One more thing i want to add, even though those memory usages seem low on the test cases, it's quite different and massive on larger projects, as to where you'd start a server with 100mb usage, with only 1 to 5 players, it went up to 600mb in less then 24 hours.
Aug 3 2019, 10:23 am
This is honestly a huge surprise. Appearances should be let go of once the client runs the garbage collector and sees they're no longer needed. Either the client GC is misbehaving or something is wrong on the server end. Either way I'll get to the bottom of it.

It's possible the GC changes in an earlier 512 build caused this. Did 511 have this problem?

Worth adding: I don't know if the appearance bag will reclaim memory used by lower ID numbers if the chunks it allocated are no longer needed. I think it might, but I'm not sure. If it doesn't, then under proper GC conditions the memory would be expected to plateau. Sounds like this is still exceeding proper conditions however.
Aug 3 2019, 10:38 am (Edited on Aug 3 2019, 10:50 am)
It's basically slowly destroying our server. If 10 people get to use the rocks proc from Test case 2, I'd say it would take 8 hours before the server gets to over 60% mem usage and at that point a server crash is incoming.

No matter if those players are online or offline, the mem usage lingers.
Even rebooting doesn't fix the problem, only a full server shutdown and rehosting it.

have not tested on 511, no.
Aug 3 2019, 3:17 pm
Here are my results so far from Ter's test case alone:

1) Running in DS: Appearances are being correctly recycled. The client is telling the server about its garbage and the server is recycling it. Only three "chunks" of appearances (room for 768 total) get allocated.

2) Running in DD+DS, debugging server: Some appearances do not appear to recycle, or are recycling in a delayed manner, such that although the client is sending garbage messages the server is allocating as many as six chunks. Appearances are, however, freed when the client closes.

3) Running in DD+DS, debugging client: The delays caused by my breakpoints appear to exacerbate the issue, which makes sense because the server is still running, but the client is definitely sending garbage messages.

4) Running DD+DS, no debugging: Identical to case 2. Appearance chunk count never exceeds 6, memory remains stable, appearances deallocate on disconnect.

Haven't tried your animate() test case yet; that's next. I'm baffled by the #2 results in this test, but equally baffled that Ter definitely saw a leak and I can't replicate it.
Aug 3 2019, 11:20 pm
Let Test 1, 2, and 3 run for about an hour, with DD open and you as DS in it, you'll see the leaks.
Aug 3 2019, 11:45 pm
I sped up test 1 by a factor of 10 and several minutes did not result in a leak. All I can verify is that some appearances seem to linger longer in DD+DS mode but I couldn't confirm a leak. Maybe if I ran the sped-up one for an hour it'd be different, but I don't know. Perhaps the "delay" gets worse and worse? I do need to figure out what causes this case to hold onto more appearances regardless, because a mere timing differential makes too little sense. In DS it topped out around 700, in DD it was 1400.

Test 2 might be more promising when I dig into that. OTOH, test 1is so straightforward that if there's a leak here and it's a common cause for all the cases, test 1 would be the obvious point of attack.

Regardless I have a lot more work to do on this. The findings above were just my initial tests. I'm gonna need to add a crapload of debugging info to get to the bottom of this.
Aug 4 2019, 1:19 am
Ter's input might be more helpful but i think he ragequitted byond yestarday.
Aug 5 2019, 1:18 pm
Update on Ter's test case: I've confirmed running in DD+DS that the appearances are in fact being recycled; it appears they're simply delayed for some reason. It may be what Ter was seeing was that the delay was getting worse over time. I can say in medium-length runs I didn't have that problem. So it appears this case has no leak, just a slow falling-behind.

I'll have to test your animate() case to see if there's a leak there. That might be something different, although it might also be related in some way.

I believe the fix I'll have to make for the first case is to alter the way appearance refcounts are handled, allowing the server to give up on an appearance if the only refs left are "a client saw this" refs. The way to do that will be with a separate count, OR by completely changing how client refs get handled.
Aug 5 2019, 1:33 pm
Is your memory also clearing up, because on our end when checking DD via task master it kept going up?
Aug 5 2019, 2:33 pm
I haven't seen any evidence of DD's memory growing unbounded. It fluctuates a ton, goes up and down, but it doesn't keep going up and up and up.
Aug 5 2019, 11:31 pm
What windows are you using?
Aug 6 2019, 11:39 am
Completely irrelevant, but it's Windows 10. The problem would be the same on any OS.
Aug 6 2019, 8:48 pm
I'm gonna look into your test case soon since I'm told it's gonna show me a more concrete issue. I do however suspect that the approach I'm working on that will eventually deal with the delay in Ter's case will solve your issue as well.
Aug 6 2019, 9:48 pm
Lummox JR resolved issue with message:
Temporary mutable appearances used in animate() didn't clean up properly, resulting in an appearance leak.