grain icon indicating copy to clipboard operation
grain copied to clipboard

feat(stdlib): Memory leak checker

Open cician opened this issue 4 years ago • 6 comments

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:

  1. ensureNoLeaks, intended for leak checks in unit testing. It only prints anything if leaks are actually found.
  2. 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.

cician avatar Oct 12 '21 20:10 cician

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. 🤭

cician avatar Oct 12 '21 21:10 cician

Hi @cician

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

marcusroberts avatar Oct 14 '21 19:10 marcusroberts

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.

cician avatar Oct 30 '21 22:10 cician

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 avatar Dec 04 '21 14:12 cician

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.

cician avatar Dec 04 '21 16:12 cician

We'll want to land https://github.com/grain-lang/grain/issues/1410 before we land this to minimize the effect on compiled modules.

ospencer avatar Aug 05 '22 23:08 ospencer