Make LocalStore::optimisePath_() non-recursive

[edolstra]

Motivation

Note: this is best reviewed by ignoring whitespace changes.

I encountered this failure during a test:

$ nix-instantiate --store /tmp/nix-gc-test '<nixpkgs>' -A hello --add-root x --auto-optimise-store
error: stack overflow (possible infinite recursion)

This turned out to be due to the recursion in optimisePath_() when called from a coroutine (which has a small stack):

  #0  0x00007ffff7e3e45d in nix::drainFD (fd=0, sink=..., opts=...) at ../src/libutil/file-descriptor.cc:131
  #1  0x00007ffff7ee6369 in nix::PosixSourceAccessor::readFile (this=0x55555617e2d0, path=..., sink=..., sizeCallback=...) at ../src/libutil/posix-source-accessor.cc:66
  #2  0x00007ffff7d38ad5 in nix::SourceAccessor::dumpPath(nix::CanonPath const&, nix::Sink&, nix::fun<bool (std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)>&)::$_0::operator()(nix::CanonPath const&) const (this=0x7fffa7d64978, path=...) at ../src/libutil/archive.cc:48
  #3  0x00007ffff7d36efc in nix::SourceAccessor::dumpPath(nix::CanonPath const&, nix::Sink&, nix::fun<bool (std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)>&)::$_1::operator()<$_1>(this $_1 const&, nix::CanonPath const&, unsigned long) (dump=..., path=..., depth=0) at ../src/libutil/archive.cc:72
  #4  0x00007ffff7d36bec in nix::SourceAccessor::dumpPath (this=0x55555617e310, path=..., sink=..., filter=...) at ../src/libutil/archive.cc:58
  #5  0x00007ffff7f1e3fa in nix::SourcePath::dumpPath (this=0x7fffa7d65f90, sink=..., filter=...) at ../src/libutil/source-path.cc:49
  #6  0x00007ffff7e3cb85 in nix::dumpPath (path=..., sink=..., method=nix::FileSerialisationMethod::NixArchive, filter=...) at ../src/libutil/file-content-address.cc:73
  #7  0x00007ffff7e3ccbd in nix::hashPath (path=..., method=nix::FileSerialisationMethod::NixArchive, ha=nix::HashAlgorithm::SHA256, filter=...) at ../src/libutil/file-content-address.cc:93
  #8  0x00007ffff763f02f in nix::LocalStore::optimisePath_ (this=0x555555eaeb00, act=0x0, stats=..., path=Python Exception <class 'gdb.error'>: No type named std::filesystem::__cxx11::path::_Cmpt.

      filesystem::path "/tmp/nix-gc-test/nix/store/9figz32vbiblaw5x3li1hc8q9qwlwf27-source/pkgs/desktops/pantheon/libraries/granite/7/default.nix", inodeHash=..., repair=nix::NoRepair)
      at ../src/libstore/optimise-store.cc:162
  #9  0x00007ffff763e70b in nix::LocalStore::optimisePath_ (this=0x555555eaeb00, act=0x0, stats=..., path=Python Exception <class 'gdb.error'>: No type named std::filesystem::__cxx11::path::_Cmpt.

      filesystem::path "/tmp/nix-gc-test/nix/store/9figz32vbiblaw5x3li1hc8q9qwlwf27-source/pkgs/desktops/pantheon/libraries/granite/7", inodeHash=..., repair=nix::NoRepair)
      at ../src/libstore/optimise-store.cc:125
  ...
  #25 0x00007ffff7f058ad in nix::sourceToSink(nix::fun<void (nix::Source&)>)::SourceToSink::operator()(std::basic_string_view<char, std::char_traits<char> >)::{lambda(boost::coroutines2::detail::pull_coroutine<bool>&)#1}::operator()(boost::coroutines2::detail::pull_coroutine<bool>&) const (this=0x7fffa7d73c98, yield=...) at ../src/libutil/serialise.cc:361
  ...
  #32 0x00007ffff3af918f in make_fcontext () from /nix/store/5xkyx9gi1s8mqp1ki1hsx8hwpr4dncxk-boost-1.87.0/lib/libboost_context.so.1.87.0

So let's make optimisePath_() non-recursive to ensure constant stack space.

Context

Taken from DeterminateSystems#508.

---

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

[xokdvium]

Hm, this change by itself is probably good.

Though this does make me wonder whether we should increase the stack size - dumpPath by itself is also recursive. The depth should already be bounded though, so maybe it's just a matter of a constant factor change?

[xokdvium]

How much memory per stack frame are we using? Surely not more than a single page?

[xokdvium]

This is probably exacerbated by drainFD using 64Kb for a buffer on the stack, which leaves very little room otherwise. We definitely should increase the coroutine stack size.

As for the stack frame size:

0000000000532e10 <_ZN3nix10LocalStore13optimisePath_EPNS_8ActivityERNS_13OptimiseStatsERKNSt10filesystem7__cxx114pathERN5boost9unordered18unordered_flat_setImNSA_4hashImEESt8equal_toImESaImEEENS_10RepairFlagE>:
  532e10:	55                   	push   %rbp
  532e11:	48 89 e5             	mov    %rsp,%rbp
  532e14:	41 57                	push   %r15
  532e16:	41 56                	push   %r14
  532e18:	41 55                	push   %r13
  532e1a:	41 54                	push   %r12
  532e1c:	53                   	push   %rbx
  532e1d:	48 81 ec f8 04 00 00 	sub    $0x4f8,%rsp

Which is just 1272 bytes.

And for dumpPath:

0000000000116630 <void nix::SourceAccessor::dumpPath(nix::CanonPath const&, nix::Sink&, nix::fun<bool (std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)>&)::$_1::operator()<$_1>(this $_1 const&, nix::SourceAccessor&, nix::CanonPath const&, nix::CanonPath const&, unsigned long)>:
  116630:	55                   	push   %rbp
  116631:	48 89 e5             	mov    %rsp,%rbp
  116634:	41 57                	push   %r15
  116636:	41 56                	push   %r14
  116638:	41 55                	push   %r13
  11663a:	41 54                	push   %r12
  11663c:	53                   	push   %rbx
  11663d:	48 81 ec 78 01 00 00 	sub    $0x178,%rsp

Which is also just 376 bytes.

So, 64 Kb buffer on the stack in drainFD leaves us at most 40 levels of recursion deep (probably much less due to other overheads).

Sounds like bumping to something like 512KiB would leave us in the clear.

[edolstra]

Yeah, I also created a DetSys PR to make dumpPath() non-recursive (DeterminateSystems#509), but it doesn't apply to master because of the readDirectory() callback changes. Probably the interface of the callback will need to be changed to take a ref<SourceAccessor>.

[edolstra]

@xokdvium Thanks, added a commit to make drainFD() use a heap allocation.

[xokdvium]

Why not just increase the coroutine stack size? That seems like the least error-prone solution - recursive algorithms are easier to read to maintain. And parseDump is also recursive. Unless we are changing all at the same time it seems like somewhat of a partial solution. Plus we do have stack depth limits now, so it should just be a matter of changing the constant factor.

Thanks, added a commit to make drainFD() use a heap allocation.

I feel like there has been a bit of a jo-jo changing from heap/stack/VLA and back historically. We probably really don't want to allocate a fresh vector for each 64KiB for each file copy.