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

Total harddel audit + far more thorough unit testing #2171

Merged
merged 156 commits into from
Sep 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
156 commits
Select commit Hold shift + click to select a range
97cdd39
initial changes
MarkSuckerberg Jul 15, 2023
70faf82
fuck
MarkSuckerberg Jul 15, 2023
aa0f898
I guess this doesn't actually make a difference
MarkSuckerberg Jul 15, 2023
fa770bb
please
MarkSuckerberg Jul 15, 2023
3e7f861
this is probably important
MarkSuckerberg Jul 15, 2023
f52d675
Ref Tracking: Revengance (#57728)
LemonInTheDark Mar 26, 2021
2d5fbb4
Fixes some more holes in the ref tracker (#58972)
LemonInTheDark May 14, 2021
2315cf8
Fixes a bunch of harddels that are sourced from player action (#59371)
LemonInTheDark Jun 11, 2021
c0d69c4
Del The World: Unit testing for hard deletes (#59612)
LemonInTheDark Aug 16, 2021
2fa00a4
OOPS
MarkSuckerberg Jul 16, 2023
80def1d
fixes indentation
MarkSuckerberg Jul 16, 2023
17b78d3
small tweaks
MarkSuckerberg Jul 16, 2023
c8ea2aa
Ref tracker TLC, makes unit test harddel errors more descriptive (#62…
LemonInTheDark Nov 11, 2021
1cc9fdd
tgstation/tgstation#63877
MarkSuckerberg Jul 16, 2023
808d1dd
buncha fixes
MarkSuckerberg Jul 16, 2023
079b0fd
augh
MarkSuckerberg Jul 16, 2023
c418f8b
final cleanup
MarkSuckerberg Jul 16, 2023
b091ca3
quirk reference fixes
MarkSuckerberg Jul 16, 2023
ccc78ce
more of em
MarkSuckerberg Jul 16, 2023
812529b
Proximity monitor related fixes. (#51544)
AnturK Jun 11, 2020
7e5b125
CanPass refactor (#59804)
Rohesie Jun 25, 2021
4f2f708
Automatic changelog generation for PR #63276 [ci skip]
tgstation-server Dec 22, 2021
707c96d
proximity monitors cleanup (plus connect_range and connect_containers…
Ghommie Dec 22, 2021
8a73ae6
fix
MarkSuckerberg Jul 16, 2023
6d88f35
let's not have this on by default actually
MarkSuckerberg Jul 16, 2023
ff6576b
more fixes
MarkSuckerberg Jul 17, 2023
aba70ee
circuitboard fix
MarkSuckerberg Jul 18, 2023
0be5b59
uhhh
MarkSuckerberg Jul 18, 2023
574cd13
oops
MarkSuckerberg Jul 18, 2023
c0f56ba
ALMOST DONE
MarkSuckerberg Jul 19, 2023
90f95cc
temporary revert
MarkSuckerberg Jul 19, 2023
b2b3814
Revert "temporary revert"
MarkSuckerberg Jul 19, 2023
64490d1
sigh
MarkSuckerberg Jul 19, 2023
47764c3
aaugh
MarkSuckerberg Jul 19, 2023
054c776
just one more thing
MarkSuckerberg Jul 19, 2023
48c73fe
you're FUCKING KIDDING ME
MarkSuckerberg Jul 19, 2023
06461d4
goowah
MarkSuckerberg Jul 19, 2023
b2c788a
wahooey
MarkSuckerberg Jul 19, 2023
51bf11e
whar
MarkSuckerberg Jul 20, 2023
5aff0c4
weh
MarkSuckerberg Jul 20, 2023
258ae5c
hmm
MarkSuckerberg Jul 20, 2023
a1814cd
a wee bit more
MarkSuckerberg Jul 20, 2023
e206723
final touches
MarkSuckerberg Jul 20, 2023
f5be173
curses
MarkSuckerberg Jul 21, 2023
2c31a13
this SHOULD be it
MarkSuckerberg Jul 21, 2023
d7d4b74
this was way too zealous
MarkSuckerberg Jul 21, 2023
644b431
fuck
MarkSuckerberg Jul 21, 2023
4b4fb01
goowah
MarkSuckerberg Jul 21, 2023
c4514cc
EEE
MarkSuckerberg Jul 21, 2023
7d78bfa
GUH
MarkSuckerberg Jul 21, 2023
0c404f6
fixes supply pods
MarkSuckerberg Jul 22, 2023
fe8f333
just a few more
MarkSuckerberg Jul 22, 2023
941e2f8
let's try this
MarkSuckerberg Jul 22, 2023
e364c00
Makes integration test results be in color and have annotations (#66649)
Tastyfish May 4, 2022
7c6af65
Keeps gc_destroyed from getting updated on every step thru the gc que…
MrStonedOne Jan 2, 2023
7d6c682
fixes new tests and more harddel fixes
MarkSuckerberg Jul 22, 2023
2be3fad
sheesh
MarkSuckerberg Jul 22, 2023
96ef1f1
REEE
MarkSuckerberg Jul 22, 2023
4b0842d
OH FUCK AAAA
MarkSuckerberg Jul 22, 2023
d23b3e4
gwuhh......
MarkSuckerberg Jul 23, 2023
07edd20
more harddel fixes
MarkSuckerberg Jul 25, 2023
3d25ba0
fixes projectile runtime
MarkSuckerberg Jul 25, 2023
0cbd4c9
more more more
MarkSuckerberg Jul 25, 2023
bfacb46
trying this I guess
MarkSuckerberg Jul 25, 2023
7cd0427
idfk
MarkSuckerberg Jul 25, 2023
e9add73
PLeASE
MarkSuckerberg Jul 25, 2023
c641aac
more things
MarkSuckerberg Jul 26, 2023
ac50847
Almost there
MarkSuckerberg Jul 26, 2023
4da1ca2
guh
MarkSuckerberg Jul 26, 2023
6a333a8
well, guess it doesn't matter
MarkSuckerberg Jul 26, 2023
aab0117
whoops
MarkSuckerberg Jul 26, 2023
f17edb9
soon
MarkSuckerberg Jul 26, 2023
156eaca
I Hate This
MarkSuckerberg Jul 26, 2023
b5c3ef3
bump
MarkSuckerberg Jul 26, 2023
13fb67b
oops
MarkSuckerberg Jul 26, 2023
0fb8abf
there
MarkSuckerberg Jul 27, 2023
6527792
whoopsie
MarkSuckerberg Jul 27, 2023
e4e4eb5
oh, need this too
MarkSuckerberg Jul 27, 2023
65bb603
gwuhhh.
MarkSuckerberg Jul 27, 2023
c886aea
Merge remote-tracking branch 'upstream/master' into harddel-test-thing
MarkSuckerberg Jul 28, 2023
11dfef2
more things yay
MarkSuckerberg Jul 28, 2023
99cf0c7
improve performance of vis_overlays by handling dir by using vis_flag…
Couls Sep 24, 2020
a4b7881
fixes fucking navbeacons
MarkSuckerberg Jul 28, 2023
ec70261
fixes elevator runtimes in create_and_destroy
MarkSuckerberg Jul 28, 2023
e413500
fixes one runtime
MarkSuckerberg Jul 28, 2023
6ff4578
Space/Changeturf fixes and optimizations (#73261)
MarkSuckerberg Jul 28, 2023
cc643c5
this probably won't help
MarkSuckerberg Jul 28, 2023
3d7c6e5
why the fuck are you like this
MarkSuckerberg Jul 28, 2023
2f2a116
temporary debugging thingy
MarkSuckerberg Jul 28, 2023
70fa6f8
Revert "temporary debugging thingy"
MarkSuckerberg Jul 28, 2023
dcb421d
Revert "Revert "temporary debugging thingy""
MarkSuckerberg Jul 28, 2023
13b02c2
temporary debugging thingy REVENGANCE
MarkSuckerberg Jul 28, 2023
47cfce6
temporary debugging thingy REVENGANCEERER
MarkSuckerberg Jul 28, 2023
568c753
I HATE YOU
MarkSuckerberg Jul 28, 2023
3f789ff
shrug
MrStonedOne Jun 10, 2021
65c0ff1
empty commit for tests
MarkSuckerberg Jul 28, 2023
b397ed5
empty commit for tests
MarkSuckerberg Jul 28, 2023
28c6be6
empty commit for tests
MarkSuckerberg Jul 28, 2023
3e48d76
polly & bookworm GC
MarkSuckerberg Jul 28, 2023
f1695a6
1984s poly into polly
MarkSuckerberg Jul 28, 2023
91e8984
gwah gwah
MarkSuckerberg Jul 28, 2023
6311d6e
fuuuuck me
MarkSuckerberg Jul 28, 2023
47a85fe
eeee
MarkSuckerberg Jul 29, 2023
1fe9480
guh!
MarkSuckerberg Jul 29, 2023
5e3b152
makes the test actually decent
MarkSuckerberg Jul 29, 2023
6123a8b
WOOOOO
MarkSuckerberg Jul 30, 2023
f86630b
empty commit for testing
MarkSuckerberg Jul 30, 2023
01cf5fe
empty commit for testing
MarkSuckerberg Jul 30, 2023
c82f65d
Final tweaks, probably
MarkSuckerberg Jul 30, 2023
d286733
actual final touches
MarkSuckerberg Jul 30, 2023
e310064
temporary change to cram in testmerges
MarkSuckerberg Jul 31, 2023
339f047
we're almost there
MarkSuckerberg Aug 1, 2023
9868a22
Fixes some runtime spam from lazyloading/map templates (#73037)
LemonInTheDark Feb 1, 2023
1006fe9
tweaks
MarkSuckerberg Aug 1, 2023
8eea913
I will lose my mind if this doesn't fix it
MarkSuckerberg Aug 1, 2023
6fb92bc
final map fixes, hopefully
MarkSuckerberg Aug 1, 2023
8f57a28
test commit
MarkSuckerberg Aug 1, 2023
9e2eb81
test commit
MarkSuckerberg Aug 1, 2023
350790d
test commit
MarkSuckerberg Aug 1, 2023
7380ba2
small tweaks
MarkSuckerberg Aug 1, 2023
c7485b2
Merge remote-tracking branch 'upstream/master' into harddel-test-thing
MarkSuckerberg Aug 2, 2023
1c79717
remove suicide handling
ZephyrTFA Aug 3, 2023
0f27407
Attempts fix of Runtime, missions, and spiders
MarkSuckerberg Aug 4, 2023
a60e649
SCREAM
MarkSuckerberg Aug 4, 2023
857dff4
bunch more fixes
MarkSuckerberg Aug 4, 2023
d59537b
we're getting there
MarkSuckerberg Aug 4, 2023
875697a
more tweaks
MarkSuckerberg Aug 4, 2023
f3cf347
sigh
MarkSuckerberg Aug 5, 2023
efb3323
shrug
MarkSuckerberg Aug 5, 2023
d69bb93
more !
MarkSuckerberg Aug 5, 2023
50d08a7
Fixes pixelshift + more fixes
MarkSuckerberg Aug 7, 2023
e414985
Reverts docking port changes (oops)
MarkSuckerberg Aug 7, 2023
ed6d365
Silences this rare spurious error
MarkSuckerberg Aug 7, 2023
6671c38
Merge remote-tracking branch 'upstream/master' into harddel-test-thing
MarkSuckerberg Aug 7, 2023
79e3a34
ree
MarkSuckerberg Aug 7, 2023
7dc0fdc
atmos fixes to hopefully not qdel the blasted vodka
MarkSuckerberg Aug 7, 2023
335e28a
fixes doubled pipe and other assorted runtimes, parrot gc redo
MarkSuckerberg Aug 8, 2023
f299bbe
makes sure there's no refs to hostile mobs from walk()
MarkSuckerberg Aug 8, 2023
76caa3c
mapzone gc fix
MarkSuckerberg Aug 9, 2023
4fc8093
Merge remote-tracking branch 'upstream/master' into harddel-test-thing
MarkSuckerberg Aug 10, 2023
5047976
more tweaks
MarkSuckerberg Aug 10, 2023
a8048dd
Merge remote-tracking branch 'upstream/master' into harddel-test-thing
MarkSuckerberg Aug 11, 2023
ce241ee
oops
MarkSuckerberg Aug 11, 2023
9c58a6e
Merge remote-tracking branch 'upstream/master' into harddel-test-thing
MarkSuckerberg Aug 11, 2023
c9d1a40
whar
MarkSuckerberg Aug 11, 2023
1d1d5f4
Merge remote-tracking branch 'upstream/master' into harddel-test-thing
MarkSuckerberg Sep 13, 2023
2256df7
Merge remote-tracking branch 'upstream/master' into harddel-test-thing
MarkSuckerberg Sep 13, 2023
7b2915a
Fixes
MarkSuckerberg Sep 13, 2023
05bc88d
whar
MarkSuckerberg Sep 13, 2023
77025ff
Buncha map fixes
MarkSuckerberg Sep 14, 2023
40c1deb
Merge remote-tracking branch 'upstream/master' into harddel-test-thing
MarkSuckerberg Sep 14, 2023
26c8122
whoops
MarkSuckerberg Sep 14, 2023
f0023e8
supplypod fix
MarkSuckerberg Sep 14, 2023
6cfa4a1
waogogus
MarkSuckerberg Sep 14, 2023
0c42f63
oops
MarkSuckerberg Sep 14, 2023
47b90bc
morefixes
MarkSuckerberg Sep 15, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
4 changes: 4 additions & 0 deletions .github/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,10 @@ This prevents nesting levels from getting deeper then they need to be.
- [tgui/README.md](../tgui/README.md)
- [tgui/tutorial-and-examples.md](../tgui/docs/tutorial-and-examples.md)

### Don't create code that hangs references

This is part of the larger issue of hard deletes, read this file for more info: [Guide to Harddels](HARDDEL_GUIDE.md))

### Other Notes

- Code should be modular where possible; if you are working on a new addition, then strongly consider putting it in its own file unless it makes sense to put it with similar ones (i.e. a new tool would go in the "tools.dm" file)
Expand Down
265 changes: 265 additions & 0 deletions .github/HARDDEL_GUIDE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,265 @@
# Hard Deletes
1. [What is hard deletion](#What-is-hard-deletion)
2. [Causes of hard deletes](#causes-of-hard-deletes)
3. [Detecting hard deletes](#detecting-hard-deletes)
4. [Techniques for fixing hard deletes](#techniques-for-fixing-hard-deletes)
5. [Help my code is erroring how fix](#help-my-code-is-erroring-how-fix)

## What is Hard Deletion
Hard deletion is a very expensive operation that basically clears all references to some "thing" from memory. Objects that undergo this process are referred to as hard deletes, or simply harddels

What follows is a discussion of the theory behind this, why we would ever do it, and the what we do to avoid doing it as often as possible

I'm gonna be using words like references and garbage collection, but don't worry, it's not complex, just a bit hard to pierce

### Why do we need to Hard Delete?

Ok so let's say you're some guy called Jerry, and you're writing a programming language

You want your coders to be able to pass around objects without doing a full copy. So you'll store the pack of data somewhere in memory

```dm
/someobject
var/id = 42
var/name = "some shit"
```

Then you want them to be able to pass that object into say a proc, without doing a full copy. So you let them pass in the object's location in memory instead
This is called passing something by reference

```dm
someshit(someobject) //This isn't making a copy of someobject, it's passing in a reference to it
```

This of course means they can store that location in memory in another object's vars, or in a list, or whatever

```dm
/datum
var/reference

/proc/someshit(mem_location)
var/datum/some_obj = new()
some_obj.reference = mem_location
```

But what happens when you get rid of the object we're passing around references to? If we just cleared it out from memory, everything that holds a reference to it would suddenly be pointing to nowhere, or worse, something totally different!

So then, you've gotta do something to clean up these references when you want to delete an object

We could hold a list of references to everything that references us, but god, that'd get really expensive wouldn't it

Why not keep count of how many times we're referenced then? If an object's ref count is ever 0, nothing whatsoever cares about it, so we can freely get rid of it

But if something's holding onto a reference to us, we're not gonna have any idea where or what it is

So I guess you should scan all of memory for that reference?

```dm
del(someobject) //We now need to scan memory until we find the thing holding a ref to us, and clear it
```

This pattern is about how BYOND handles this problem of hanging references, or Garbage Collection

It's not a broken system, but as you can imagine scanning all of memory gets expensive fast

What can we do to help that?

### How we can avoid hard deletes

If hard deletion is so slow, we're gonna need to clean up all our references ourselves

In our codebase we do this with `/datum/proc/Destroy()`, a proc called by `qdel()`, whose purpose I will explain later

This procs only job is cleaning up references to the object it's called on. Nothing more, nothing else. Don't let me catch you giving it side effects

There's a long long list of things this does, since we use it a TON. So I can't really give you a short description. It will always move the object to nullspace though

## Causes Of Hard Deletes

Now that you know the theory, let's go over what can actually cause hard deletes. Some of this is obvious, some of it's much less so.

The BYOND reference has a list [Here](https://secure.byond.com/docs/ref/#/DM/garbage), but it's not a complete one

* Stored in a var
* An item in a list, or associated with a list item
* Has a tag
* Is on the map (always true for turfs)
* Inside another atom's contents
* Inside an atom's vis_contents
* A temporary value in a still-running proc
* Is a mob with a key
* Is an image object attached to an atom

Let's briefly go over the more painful ones yeah?

### Sleeping procs

Any proc that calls `sleep()`, `spawn()`, or anything that creates a seperate "thread" (not technically a thread, but it's the same in these terms. Not gonna cause any race conditions tho) will hang references to any var inside it. This includes the usr it started from, the src it was called on, and any vars created as a part of processing

### Static vars

`/static` and `/global` vars count for this too, they'll hang references just as well as anything. Be wary of this, these suckers can be a pain to solve

### Range() and View() like procs

Some internal BYOND procs will hold references to objects passed into them for a time after the proc is finished doing work, because they cache the returned info to make some code faster. You should never run into this issue, since we wait for what should be long enough to avoid this issue as a part of garbage collection

This is what `qdel()` does by the by, it literally just means queue deletion. A reference to the object gets put into a queue, and if it still exists after 5 minutes or so, we hard delete it

### Walk() procs

Calling `walk()` on something will put it in an internal queue, which it'll remain in until `walk(thing, 0)` is called on it, which removes it from the queue

This sort is very cheap to harddel, since BYOND prioritizes checking this queue first when it's clearing refs, but it should be avoided since it causes false positives

You can read more about how BYOND prioritizes these things [Here](https://www.patreon.com/posts/diving-for-35855766)

## Detecting Hard Deletes

For very simple hard deletes, simple inspection should be enough to find them. Look at what the object does during `Initialize()`, and see if it's doing anything it doesn't undo later.
If that fails, search the object's typepath, and look and see if anything is holding a reference to it without regard for the object deleting

BYOND currently doesn't have the capability to give us information about where a hard delete is. Fortunately we can search for most all of then ourselves.
The procs to perform this search are hidden behind compile time defines, since they'd be way too risky to expose to admin button pressing

If you're having issues solving a harddel and want to perform this check yourself, go to `_compile_options.dm` and uncomment `TESTING`, `REFERENCE_TRACKING`, and `GC_FAILURE_HARD_LOOKUP`

You can read more about what each of these do in that file, but the long and short of it is if something would hard delete our code will search for the reference (This will look like your game crashing, just hold out) and print information about anything it finds to the runtime log, which you can find inside the round folder inside `/data/logs/year/month/day`

It'll tell you what object is holding the ref if it's in an object, or what pattern of list transversal was required to find the ref if it's hiding in a list of some sort

## Techniques For Fixing Hard Deletes

Once you've found the issue, it becomes a matter of making sure the ref is cleared as a part of Destroy(). I'm gonna walk you through a few patterns and discuss how you might go about fixing them

### Our Tools

First and simplest we have `Destroy()`. Use this to clean up after yourself for simple cases

```dm
/someobject/Initialize()
. = ..()
GLOB.somethings += src //We add ourselves to some global list

/someobject/Destroy()
GLOB.somethings -= src //So when we Destroy() clean yourself from the list
return ..()
```

Next, and slightly more complex, pairs of objects that reference each other

This is helpful when for cases where both objects "own" each other

```dm
/someobject
var/someotherobject/buddy

/someotherobject
var/someobject/friend

/someobject/Initialize()
if(!buddy)
buddy = new()
buddy.friend = src

/someotherobject/Initialize()
if(!friend)
friend = new()
friend.buddy = src

/someobject/Destroy()
if(buddy)
buddy.friend = null //Make sure to clear their ref to you
buddy = null //We clear our ref to them to make sure nothing goes wrong

/someotherobject/Destroy()
if(friend)
friend.buddy = null //Make sure to clear their ref to you
friend = null //We clear our ref to them to make sure nothing goes wrong
```

Something similar can be accomplished with `QDELETED()`, a define that checks to see if something has started being `Destroy()`'d yet, and `QDEL_NULL()`, a define that `qdel()`'s a var and then sets it to null

Now let's discuss something a bit more complex, weakrefs

You'll need a bit of context, so let's do that now

BYOND has an internal bit of behavior that looks like this

`var/string = "\ref[someobject]"`

This essentially gets that object's position in memory directly. Unlike normal references, this doesn't count for hard deletes. You can retrieve the object in question by using `locate()`

`var/someobject/someobj = locate(string)`

This has some flaws however, since the bit of memory we're pointing to might change, which would cause issues. Fortunately we've developed a datum to handle worrying about this for you, `/datum/weakref`

You can create one using the `WEAKREF()` proc, and use weakref.resolve() to retrieve the actual object

This should be used for things that your object doesn't "own", but still cares about

For instance, a paper bin would own the paper inside it, but the paper inside it would just hold a weakref to the bin

There's no need to clean these up, just make sure you account for it being null, since it'll return that if the object doesn't exist or has been queued for deletion

```dm
/someobject
var/datum/weakref/our_coin

/someobject/proc/set_coin(/obj/item/coin/new_coin)
our_coin = WEAKREF(new_coin)

/someobject/proc/get_value()
if(!our_coin)
return 0

var/obj/item/coin/potential_coin = our_coin.resolve()
if(!potential_coin)
our_coin = null //Remember to clear the weakref if we get nothing
return 0
return potential_coin.value
```

Now, for the worst case scenario

Let's say you've got a var that's used too often to be weakref'd without making the code too expensive

You can't hold a paired reference to it because it's not like it would ever care about you outside of just clearing the ref

So then, we want to temporarily remember to clear a reference when it's deleted

This is where I might lose you, but we're gonna use signals

`qdel()`, the proc that sets off this whole deletion business, sends a signal called `COMSIG_PARENT_QDELETING`

We can listen for that signal, and if we hear it clear whatever reference we may have

Here's an example

```dm
/somemob
var/mob/target

/somemob/proc/set_target(new_target)
if(target)
UnregisterSignal(target, COMSIG_PARENT_QDELETING) //We need to make sure any old signals are cleared
target = new_target
if(target)
RegisterSignal(target, COMSIG_PARENT_QDELETING, .proc/clear_target) //Call clear_target if target is ever qdel()'d

/somemob/proc/clear_target(datum/source)
SIGNAL_HANDLER
set_target(null)
```

This really should be your last resort, since signals have some limitations. If some subtype of somemob also registered for parent_qdeleting on the same target you'd get a runtime, since signals don't support it

But if you can't do anything else for reasons of conversion ease, or hot code, this will work

## Help My Code Is Erroring How Fix

First, do a quick check.

Are you doing anything to the object in `Initialize()` that you don't undo in `Destroy()`? I don't mean like, setting its name, but are you adding it to any lists, stuff like that

If this fails, you're just gonna have to read over this doc. You can skip the theory if you'd like, but it's all pretty important for having an understanding of this problem
Loading
Loading