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

Collect eval memory before building with nix-build #5747

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

andir
Copy link
Member

@andir andir commented Dec 8, 2021

nix-build: free eval memory before building paths

This ensures that we free some of the memory that was allocated during
the eval of the build target. In particular this will free the Boehm GC
managed memory allocations.

On my sample system I was able to reduce the memory consumption from
550MiB to about 200MiB when building the NixOS GNOME test.

The memory did initially climb to 550MiB and then shrink down to 200MiB
once the builds were kicked off.

This means I can now run more build processes (without having to resort
to swap) on resource constrained systems.

Fixes #5200 (for me)

This made it easier for me to manuall track the lifetime of some of the
variabes involved  in the code.
This ensures that we free some of the memory that was allocated during
the eval of the build target. In particular this will free the Boehm GC
managed memory allocations.

On my sample system I was able to reduce the memory consumption from
550MiB to about 200MiB when building the NixOS GNOME test.

The memory did initially climb to 550MiB and then shrink down to 200MiB
once the builds were kicked off.

This means I can now run more build processes (without having to resort
to swap) on resource constrained systems.

Fixes NixOS#5200 (for me)
@edolstra
Copy link
Member

It would be nice to have some numbers on the performance cost. Doing a GC slows down nix-build, so ideally this is something we only do in low-memory conditions.

I'm also concerned with doing the GC 10 times, which seems strange. I wouldn't expect a mark-and-sweep collector to free more memory on subsequent runs. The main risk here is that if there is any (false) pointer that keeps the pkgs data structure alive, then we'll be doing 10 sweeps over a multi-gigabyte heap without freeing up a lot of memory. (False pointers can easily happen, see for instance 455d1f0.) In fact, it might not free any memory, since Boehm is not a compacting collector, so due to heap fragmentation it probably wouldn't be able to return a lot of memory to the OS.

BTW, as a workaround, you can use nix-store -r $(nix-instantiate ...). nix-build used to be implemented that way.

@andir
Copy link
Member Author

andir commented Dec 15, 2021

It would be nice to have some numbers on the performance cost. Doing a GC slows down nix-build, so ideally this is something we only do in low-memory conditions.

I'm also concerned with doing the GC 10 times, which seems strange. I wouldn't expect a mark-and-sweep collector to free more memory on subsequent runs.

I do agree. I just went by what was in some of the related BoehmGC discussions and documentation. IIRC I tried with just one GC run and that wasn't sufficient to see a difference when I original tested this (~6M ago).

The main risk here is that if there is any (false) pointer that keeps the pkgs data structure alive, then we'll be doing 10 sweeps over a multi-gigabyte heap without freeing up a lot of memory. (False pointers can easily happen, see for instance 455d1f0.)

But then that is a bug that has to be fixed. It wouldn't surface right now because we don't care about the memory. We probably should care about the memory and fix the issues once we observe them.

In fact, it might not free any memory, since Boehm is not a compacting collector, so due to heap fragmentation it probably wouldn't be able to return a lot of memory to the OS.

Apparently it does so anyway.

BTW, as a workaround, you can use nix-store -r $(nix-instantiate ...). nix-build used to be implemented that way.

I know. We discussed this in #5200. I dont' think that is a valid workaround for regular uses that just want to call nixos-rebuild to rebuild their system configuration. It would just be yet another oddity while learning how to deal with nix and the nix language.

@andir
Copy link
Member Author

andir commented Dec 15, 2021

Here are the results of some basic build testing. I've built the attribute before running the test. There were no actual builds performed during the testing.

"new" is at commit ba655d46171b9eae5d5a3c54e47a0d13c0861ac6
"old" is at commit f66923e
"new-one-gc" is the same as new but with a signle GC round.

The nixpkgs checkout was a2e281f5770247855b85d70c43454ba5bff34613.

Benchmark 1: ./old/bin/nix-build ~/dev/nixos/nixpkgs/nixos/tests/simple.nix
  Time (mean ± σ):      2.275 s ±  0.029 s    [User: 1.710 s, System: 0.292 s]
  Range (min … max):    2.234 s …  2.339 s    10 runs
 
Benchmark 2: ./new/bin/nix-build ~/dev/nixos/nixpkgs/nixos/tests/simple.nix
  Time (mean ± σ):      2.544 s ±  0.127 s    [User: 2.962 s, System: 0.309 s]
  Range (min … max):    2.430 s …  2.724 s    10 runs
 
Benchmark 3: ./new_one_gc/bin/nix-build ~/dev/nixos/nixpkgs/nixos/tests/simple.nix
  Time (mean ± σ):      2.441 s ±  0.033 s    [User: 2.089 s, System: 0.303 s]
  Range (min … max):    2.395 s …  2.519 s    10 runs
 
Summary
  './old/bin/nix-build ~/dev/nixos/nixpkgs/nixos/tests/simple.nix' ran
    1.07 ± 0.02 times faster than './new_one_gc/bin/nix-build ~/dev/nixos/nixpkgs/nixos/tests/simple.nix'
    1.12 ± 0.06 times faster than './new/bin/nix-build ~/dev/nixos/nixpkgs/nixos/tests/simple.nix'

It is indeed slower. Personally I'd prefer correctness (no leaked memory, system can reclaim space) over a fraction of a second in interactive evals.

@Mic92
Copy link
Member

Mic92 commented Dec 16, 2021

BTW, as a workaround, you can use nix-store -r $(nix-instantiate ...). nix-build used to be implemented that way.

Could this be racy when garbage collection is used?

@@ -570,8 +579,21 @@ static void main_nix_build(int argc, char * * argv)
else
drvMap[drvPath] = {drvMap.size(), {outputName}};
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do the evaluation in a child process and reclaim the memory this way?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this option was discussed but dismissed.

@edolstra
Copy link
Member

Could this be racy when garbage collection is used?

Yes, if you want to do it right you have to do nix-store -r ... --gc-root ....

@thufschmitt
Copy link
Member

Since these GC run aren’t (hopefully) changing anything to the semantics of the program, could it make sense to start them in a different thread while the build is starting? That way the speed impact would be minimal

@stale stale bot added the stale label Nov 12, 2022
@roberth roberth added language The Nix expression language; parser, interpreter, primops, evaluation, etc performance labels Jul 1, 2023
@stale stale bot removed the stale label Jul 1, 2023
@roberth
Copy link
Member

roberth commented Jul 1, 2023

Some possible mitigations for the performance cost:
a. reduce the priority of the thread that performs the GC
b. don't GC if there's no memory pressure; while (!memoryPressure() && !interrupted) { sleep(1s); }; if (!interrupted) gc()

Regardless of this GC may still incur a cost by increasing utilization of the memory bus, which may very well be the bottleneck of a build process, so (a) is not a complete fix, but (b) is a "complete" if the system has plenty of memory.

Since these GC run aren’t (hopefully) changing anything to the semantics of the program, could it make sense to start them in a different thread while the build is starting? That way the speed impact would be minimal

Some things to make sure

  • Boehm GC doesn't stop threads involved in the monitoring and scheduling of the builds
  • The threads don't reference any GC-allocated memory. If they do, provide a garbage collection root that isn't the thread that uses them; that thread can't serve as a root because the GC would have to pause that thread

@roberth roberth mentioned this pull request Dec 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
language The Nix expression language; parser, interpreter, primops, evaluation, etc performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants