feat(stdlib): Memory leak checker #984

cician · 2021-10-12T20:55:39Z

This PR adds a tool that is intended to help solve issues in code with manual memory management.

I've added a new module called runtimeDebug, but I'm open to suggestions if another name or placing it in an existing one is more appropriate. For example there's a debug module, but I don't know what its purpose is.

How it works

The changes to the gc module has added support to setup hooks called on each incRef, decRef, malloc and free. Otherwise the changes here are limited to expose these hooks and remove them.

The runtimeDebug exposes two functions:

ensureNoLeaks, intended for leak checks in unit testing. It only prints anything if leaks are actually found.
leakDebugBlock, intended to be used when debugging gets tricky and more details are needed. It can print every incRef, decRef, malloc and free with addresses, tags and even object contents as strings (when safe-ish to do so). This part is inspired by gc.setDebug functionally that is currently commented out.

When the hooks are connected, internally there's a bit of a delicate dance on hot ash, since runtimeDebug is using managed memory and even GC, while GC itself calls into incRef/decRef/malloc/free. To accomplish this, the hooks are inhibited temporarily.

A few details

I've used raw Wasmi32 pointers for function hooks. Maybe I should use something else? box? Or actual function type?

While the hook functions use WasmI32 pointers for tracked objects, I convert them internally to Number for use in GC land. May have used Int32 instead, but I chose Number hoping to reduce allocations at least for programs with small enough heap.

Limitations and drawbacks

Nesting ensureNoLeaks/leakDebugBlock is not supported, but not prevented.

Per function/code block based testing may not work for debugging some cases covered by the old gc.setDebug functionality. In particular when runtime modules themselves are starting up.

ensureNoLeaks is obviously useful for detecting memory leaks, but may not be all that helpful for other issues like use after free, overwritten object headers or infinite loops in malloc.free (all things experience by yours truly). It may help to run leakDebugBlock with printIncrefsAndDecRefs=true, printMallocAndFree=true and printObjectContents=false.

It adds a slight overhead to memory management for all programs, even if the hooks aren't hooked up. I haven't done any comparisons yet. Hopefully not much, just checking if the hook function is set in a variable. Some sort of conditional compilation would be nice in this case.

Finally, it's not able to leak check the leak checker, so I don't guarantee it doesn't leak memory.

Updated test snapshots.

cician · 2021-10-12T21:12:21Z

Forgot to mention. I made the CI fail intentionally for now, just to see how it looks on GitHub when a test fails for memory leaks detected. 🤭

marcusroberts · 2021-10-14T19:05:55Z

Hi @cician

Just wanted to say thanks for a great contribution and I look forward to looking through it!

- The LeakDebugSummaryMode option wasn't respected so tests were noisy and confusing. - Added filters to exclude invocation of hooks and of the function to be tested. - Added detection of deallocations of pre-existing objects. - Added detailed reporting of ref. counting issues on pre-existing objects. - Fixed decRef issues in printing object contents and added special case for printing strings. - Commented out the failing test for Buffer (the leak in question has not been fixed yet).

peblair

I finally had some time to give this a look. @cician this is incredible stuff! I've left some first-pass comments, and I'm curious to see what the other core team members have to say.

peblair · 2021-10-30T15:36:50Z

stdlib/runtime/gc.gr

+        // buffer is not being flushed? If the debugging hooks are active then
+        // behave ad if ignoreZeros was true to continue executing and give the
+        // debugging hooks an opportunity to report the issue.
+        if (WasmI32.ne(_DEBUG_HOOK_DECREF, 0n)) {


Very interesting. I wonder if we could get around this by adding a call to the flush syscall in the if (WasmI32.ne(_DEBUG_HOOK_DECREF, 0n)) expression? 🤔

stdlib/runtime/gc.gr

peblair · 2021-10-30T15:39:15Z

stdlib/runtime/runtimeDebug.gr

+
+  let tagToTypeName = (tag: Number) => {
+    match (tag) {
+      1 => "string",


Maybe we should use the constants from runtime/unsafe/tags?

I didn't use them to avoid @disableGC. Should I convert tagToTypeName to no gc?

I hadn't considered that! In that case, I think a comment is fine for now (maybe also add a comment to tags.gr so we can remember to keep this in sync?).

I changed this function to use @disableGC. It's not pretty, but probably still better than not using the constants.

stdlib/runtime/runtimeDebug.gr

peblair · 2021-10-30T15:42:13Z

stdlib/runtime/runtimeDebug.gr

+  // totals can create confusion. The simplest solution would be to get the tag
+  // of mallocated objects, but they don't yet have a tag associated. So I
+  // have to compensate again by adding up the objects that have been
+  // mallocated but never freed. Which simply means the ones in "allocations"
+  // Map.


This part loses me a little. Can you maybe elaborate a bit?

I agree this comment is pretty confusing. I'll rewrite it.

What I mean is that to correctly present the summary of incRefs/decRefs by tag, it's not enough to just track incRefs/decRefs themselves because incRef doesn't get called for newly allocated objects. They start with ref. count of 1 set directly in GC.malloc. But it also isn't as simple as adding some code in the hook for GC.malloc because freshly allocated objects don't have a tag yet. The tag is normally set after (like in dataStructures.allocateString and similar functions). So what I actually do is track the lifetime of single objects in the allocations map. Only when the object is freed I add to the mallocPlusFreeByTag map to present along with incRefs/decRefs by tag. If an object is not freed then it's left in the allocations map. So I group these leaked objects by tag as well and present the totals in the summary as "malloc without free".

peblair · 2021-10-30T15:44:39Z

stdlib/runtime/runtimeDebug.gr

+  let mallocPlusFreeByTag = Map.make(): (Map.Map<Number, Number>)
+
+  @disableGC
+  let rec readTag = (userPtrAsNumber: Number) => {


I can imagine why this is the case, but it may be worth a comment explaining why these functions use Number instead of WasmI32.

I've added a comment explaining the choice for Number.

peblair · 2021-10-30T15:46:42Z

stdlib/runtime/runtimeDebug.gr

+    if (Set.contains(userPtr, allocations)) {
+      // A new allocation, but for an object already allocated inside the debug block, but never removed.
+      print(
+        "⚠ PROBLEM DETECTED: unexpected malloc at address 0x" ++


Sort of wish we had colored output here hehe
(not really requesting this, but just a thought)

peblair · 2021-10-30T15:47:40Z

stdlib/runtime/runtimeDebug.gr

+    }
+
+    if (Map.contains(userPtr, incRefsOnPreExistingObjects) ||
+    Map.contains(userPtr, decRefsOnPreExistingObjects)) {


Is this the formatter output? If so, cc @marcusroberts (I think that Map should be indented here)

Yes, it is.

stdlib/runtime/runtimeDebug.gr

Co-authored-by: Philip Blair <[email protected]>

ospencer

Very cool! And awesome that we've already seen this in action 🙂

A couple of thoughts from me but no big blockers :)

One thing to consider— @phated when we split the runtime from the stdlib, will this be annoying to refactor? I think this makes sense to live in the runtime, but since it uses stdlibs it might be hard to break out, and I'm not sure where it really should live then. I feel like we can worry about it when we actually make that change, but figured I'd grab your thoughts.

ospencer · 2021-10-30T17:04:09Z

stdlib/runtime/gc.gr

+let mut _DEBUG_HOOK_DECREF = 0n
+let mut _DEBUG_HOOK_MALLOC = 0n
+let mut _DEBUG_HOOK_FREE = 0n
+let mut _DEBUG_HOOK_RUNNING = false


It's not obvious to me that the _DEBUG value above doesn't have anything to do with these values. I wonder if there's any renaming we can do to make that more clear.

Or maybe just move it to the commented code below? Currently it's never used, its for the debugging functions that are all commented out. I agree though that leak checker code can get mixed up with these.

ospencer · 2021-10-30T17:06:48Z

stdlib/runtime/runtimeDebug.gr

@@ -0,0 +1,712 @@
+import WasmI32 from "runtime/unsafe/wasmi32"


This filename is a little confusing vs the current runtime/unsafe/debug file. We can't combine them because runtime/debug needs to run in runtime mode—maybe we rename this file to memoryDebug.gr or something similar?

memoryDebug.gr sounds good to me.

Should I move it outside of the runtime folder as well?

This was causing memory leaks in the leak checker.

cician · 2021-10-30T22:03:01Z

I checked if the leak checker leaks memory, and it does.

At least in part it's due to #1003 and #773 (these functions are recursive in order to decRef themselves). I can work around #773 by moving some utility functions to the top level, but its difficult to diagnose further until #1003 is fixed.

…rain-lang#773 as well as for improved readability.

… operation.

cician · 2021-12-04T14:51:33Z

Probably not a big deal, but the leak checker currently reports leaks when converting numbers to strings due to buffers allocated and intentionally never released in the numberUtils module (_POWERS10, _DIGITS, _HEX_DIGITS etc).

It can be a bit confusing when you get a resulting like this:

⚠ 1 malloc/free leaks detected:
- 0x3e9c80 (ref. count: 1; tag 825241648 unknown): <unknown heap tag type: 0x31303030 | value: 0x3e9c80>

One solution would be to simply call a few functions preemptively to force initialize these buffers, but maybe we want to give them a proper reserved heap tag? Something like "cache" or "lookup table". Otherwise, if it used malloc directly instead of Memory.malloc it wouldn't get reported, but I don't know if we want that.

cician · 2021-12-04T16:26:30Z

I checked if the leak checker leaks memory, and it does.

At least in part it's due to #1003 and #773 (these functions are recursive in order to decRef themselves). I can work around #773 by moving some utility functions to the top level, but its difficult to diagnose further until #1003 is fixed.

With #1003 fixed and a fix in the checker itself it's now mostly leak free. There are still leaks elsewhere in the stdlib that make it leak indirectly when working on Sets, Maps and Arrays to print the summary.

If you're wondering how I leak check the leak checker, I'm just running it in an infinite loop and watching if the memory continues to rise indefinitely in the task manager.

ospencer · 2022-08-05T23:17:45Z

We'll want to land #1410 before we land this to minimize the effect on compiled modules.

cician added 2 commits October 12, 2021 21:19

feat(stdlib): Memory leak checker

93ea930

feat(stdlib): Memory leak checker

59243ca

Updated test snapshots.

cician mentioned this pull request Oct 24, 2021

Memory leak in indexed array access #1002

Closed

cician added 2 commits October 24, 2021 21:11

Merge remote-tracking branch 'upstream/main' into leakchecker

0818a28

peblair reviewed Oct 30, 2021

View reviewed changes

cician and others added 3 commits October 30, 2021 18:12

Applied suggested change to remove unneeded commented code.

5c8e4e4

Co-authored-by: Philip Blair <[email protected]>

Applied suggested change in the comment.

2446ef2

Co-authored-by: Philip Blair <[email protected]>

Applied suggested change in the comment.

07e224b

Co-authored-by: Philip Blair <[email protected]>

ospencer reviewed Oct 30, 2021

View reviewed changes

cician added 4 commits October 30, 2021 22:35

Remove incRefs on hook functions.

13a3aa2

This was causing memory leaks in the leak checker.

Fixed a typo.

0491438

The hook functions weren't decRefed when removed.

5939aaa

Rewrote a comment for clarity.

23db1cf

cician added 9 commits October 31, 2021 11:25

Moved some utility functions to the top level as workaround for issue g…

89b87f9

…rain-lang#773 as well as for improved readability.

Chaned tagToTypeName function to use constants from the Tags module.

f827847

Applied formatter with latest fixes.

e1c544c

Renamed the module to memoryDebug.

4e608db

Added comments about the implementation details.

3bf67eb

Merge remote-tracking branch 'upstream/main' into leakchecker

ac6037f

Sort the summary by tag number.

6378f34

Fixed incRef/decRef problems not being reported without printing each…

a4849fb

… operation.

Avoid printing object contents if they could be partially deallocated.

e8767fc

cician added 3 commits December 4, 2021 16:02

Merge remote-tracking branch 'upstream/main' into leakchecker

9c66e15

Updated test snapshots after merging main.

075633f

Fixed leaks of the hook closures.

580f059

phated marked this pull request as draft August 5, 2022 23:47

phated added the blocked label Aug 5, 2022

phated assigned cician and ospencer Aug 5, 2022

		@@ -0,0 +1,712 @@
		import WasmI32 from "runtime/unsafe/wasmi32"

Uh oh!

feat(stdlib): Memory leak checker #984

Are you sure you want to change the base?

feat(stdlib): Memory leak checker #984

Uh oh!

Conversation

cician commented Oct 12, 2021

How it works

A few details

Limitations and drawbacks

Uh oh!

cician commented Oct 12, 2021

Uh oh!

marcusroberts commented Oct 14, 2021

Uh oh!

peblair left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ospencer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cician commented Oct 30, 2021

Uh oh!

cician commented Dec 4, 2021

Uh oh!

cician commented Dec 4, 2021

Uh oh!

ospencer commented Aug 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants