Reduce memory leaks

[Et7f3]

Motivation

I wanted to evaluate all nixpkgs recursively. So I can get the derivers for the package I can and dependents. I saw the warning but just wanted to evaluate so it should be safe ? When I did it, the process hang and with sampling I saw it was in GC code very often

I saw virtual memory going crazy (75Go I think it is uncompressed because I haven't seen a such large swap file) and it shows me:

I didn't fixed all the leaks but almost an halved the node allocation and solved 90% of leaked bytes. It is a memory usage improvement because I move instead of copying.

On master:

leaks Report Version: 4.0, multi-line stacks
Process 69043: 9018814 nodes malloced for 461290 KB
Process 69043: 3920303 leaks for 152631056 total leaked bytes.

after my PR:

leaks Report Version: 4.0, multi-line stacks
Process 69681: 5704562 nodes malloced for 351694 KB
Process 69681: 388510 leaks for 25151056 total leaked bytes.

LeakSanitizer doesn't works with nixpkgs so I used:
1>leaks_log_master 2>&1 /usr/bin/leaks --atExit -- nix-master/bin/nix repl ../nixpkgs <<<$(echo legacyPackages.x86_64-darwin); 1>leaks_log_built 2>&1 /usr/bin/leaks --atExit -- src/nix/nix repl ../nixpkgs <<<$(echo legacyPackages.x86_64-darwin)

I identified other leaks root cause:

• AST is never freed should be ok for casual usage. It can't be simply deleted because Value keep pointer on Expr*
• Expr and derived doesn't implement destructor correctly so even with the GC they can't free the whole memory used.

Those cause would imply a more bigger change so I open-ed PR to see if it is worth.

Context

Each commit I diffed with master to see no regression and didn't see one so it should be correct even if installcheck isn't running on master.

The first commit isn't worth a PR (so it is their) but while I changed code I just saw and I modified it.

Each commit should be droppable but I intend to merge all expect the first.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

• agreed on idea
• agreed on implementation strategy
• tests, as appropriate
  • functional tests - tests/**.sh
  • unit tests - src/*/tests
  • integration tests - tests/nixos/*
• documentation in the manual
• code and comments are self-explanatory
• commit message explains why the change was made
• new feature or incompatible change: updated release notes

[roberth]

This looks great!

/usr/bin/leaks

What's /usr/bin? ;)
This seems to be Apple-specific. A quick search did not find an open source version of this. Is that correct?

Could you add your findings to the doc/manual/src/contributing/hacking.md file?

[Et7f3]

This looks great!

/usr/bin/leaks

What's /usr/bin? ;) This seems to be Apple-specific. A quick search did not find an open source version of this. Is that correct?

Could you add your findings to the doc/manual/src/contributing/hacking.md file?

I don't know how I got this binary. Brew should install in /usr/local/bin so it seems builtin in MacOS . I can add those but I prefer using LeakSanitizer (better error reporting) I don't know why it stopped working and I haven't a commit where it worked (I will search one with date)

[jeff-hykin]

Thanks for making this! I'm also trying to eval nixpkgs recursively and hitting memory errors. This PR might help with #7816

[roberth]

Code lgtm. Could you squash the commits?

[Et7f3]

Code lgtm. Could you squash the commits?

done

[roberth]

Thank you so much @Et7f3 !

I identified other leaks root cause:

• AST is never freed should be ok for casual usage. It can't be simply deleted because Value keep pointer on Expr*
• Expr and derived doesn't implement destructor correctly so even with the GC they can't free the whole memory used.

I think the reasoning was that it's more efficient not to clean those up correctly. If that's really the case, perhaps we should have a compilation flag that enables correct cleanup, just for the purpose of evaluating code quality.
Correct cleanup could mean either of two things:

• put the Exprs on the GC-managed heap and let them be released as appropriate, using either regular GC root pointers, or weak pointers
• or, just delete the Exprs when the EvalState is deleted, hoping that all references to them have been removed before the deletion. This wouldn't help for anything, except memory stats, which is a valid reason as long as it's behind a compilation flag. i don't think we want to risk this in a real world process that's about to exit anyway.

[Et7f3]

I think the reasoning was that it's more efficient not to clean those up correctly

It is not a performance choice because it copy

nix/src/libexpr/eval.cc

Lines 905 to 908 in a88ae62

	void Value::mkString(std::string_view s)
	{
	mkString(makeImmutableString(s));
	}

if leakage was intentional I would just use (new std::string {...})->c_str() and leak only the structure or keep using std::string_view

And delete where used before (commit on master before merging)

nix/src/libexpr/parser.y

Line 185 in e4726a0

delete formals;

I would advocate for second option: by managing ourself it might show place where we could do move and get a perf boost.

Also an other argument is in the sampling, the GC is already overloaded and AST node are not needed at eval time since Value are created.

Do you want I move the delete I added under a flag ?

[Et7f3]

For my use case I use repl and it leaks a lot (it is acceptable for a short lived program). What nix program are log-lived server that might leak ? nix-store --serve ?