Fix adding large paths to the store

[Mathnerd314]

This requires a patched version of the latest boost (sed 's|explicit basic_format|basic_format|' boost/format_class.hpp) compiled with -std=c++14.

[edolstra]

Hm, not too keen on adding a dependency on Boost. It's a huge dependency... (> 100 MB)

Also, why does this require C++14?

[Mathnerd314]

It's for Boost's coroutines2, which depends on C++14. See SinkSource and the documentation. I guess it could be swapped out with the old coroutines library which doesn't need C++14.
All of the C++ coroutine libraries I could find have a Boost dependency, though.

[Mathnerd314]

The coroutines can be avoided by duplicating the code from dumpPath and restorePath. So no more C++14 or Boost dependency. 😑

[domenkozar]

I really with this PR had a http://sscce.org

[Mathnerd314]

The example is:

fallocate -l 10G tenGBfile
nix-store --add tenGBfile

Currently it prints 'warning: dumping large path to store', and then runs out of memory. With this PR, it actually adds the file (after some time).

[domenkozar]

@edolstra does this need anything else besides resolving conflicts?

[shlevy]

Big 👍 on the concept, will review if @edolstra is OK with me making a merge call here.

[ip1981]

Does this cover large directories?

[shlevy]

@edolstra ping

[bjornfor]

Bump. Still very interested in this.

[copumpkin]

Same, especially if handles the large folder situation as well.

[butterflya]

It appears that everyone except one is in favor of this. The obvious solution seems to be that this person puts a big ifdef block in the code he is running on his machine and let's the rest of the world enjoy their 256GB RAM servers with TBs of storage, or their 8GB RAM phones.

Boost code is high quality and trying to rip out a few functions from it will only increase the probability of problems created by trying to be smart.

Most Boost libraries are header only, so I can't really imagine it adding 100MB to the final binary either. If anything, I would love to see less custom code written and more Boost code in nix. Boost code is run on millions of machines every day. Nix is used on less than 5000 machines by significantly less people. Boost code has regular releases, tests, etc. Nix in comparison has approximately none of that.

@Mathnerd314 even took care of the concerns regarding Boost and still it wasn't merged.

Why wasn't this merged on the very same day the PR was opened?

[domenkozar]

@butterflya there's no reason to be angry and point fingers. Please tune down your tone, there are many fixes going into Nix in daily basis so clearly this one will also eventually be there.

Insulting people won't help at all and Nix community does not tolerate such behavior. Consider this as a warning.

[butterflya]

@domenkozar What do you mean by tone? I am merely making it obvious that the wrong decision was made. Please tell me how I can insert in your brain that you are wrong, wrong, wrong in any other way than presenting an argument? Since you are threatening with violence, it appears that you have no argument.

I didn't say I was angry and there is plenty of reason to point fingers. There is only one person who held merging this back: @edolstra and apparently you are defending him without any argument, I want to stress. Tell me, why is there no reason to point fingers?

Please note that this issue wouldn't have existed if @edolstra hadn't made a mistake in the first place. Clearly, we cannot depend on his judgement alone and he needs help in decision making.

@domenkozar Why do you defend @edolstra? Even you wanted to merge this (see the thumbs up icon you gave it). Why don't you just tell @edolstra to take a hike on this issue? Are you afraid of him? You would get a lot more respect if you presented facts and arguments instead of trying to import Eastern-European practices to a mostly Western online society.

[copumpkin]

@butterflya telling the owner/founder of the project (and by far its biggest code contributor) to take a hike isn't exactly productive. I have no "warning" powers but it feels like you're just being argumentative without much context. Sometimes we (community members) disagree with @edolstra and sometimes he just forgets about issues that are important, because there are literally dozens of code contributions per day between this repo and nixpkgs, and a fairly high percentage of them ask for his input. I'd probably go crazy if I were getting pinged as often as he is 😄

Anyway, minus the accusations and tone, I agree that this should be merged in some form or another. @edolstra do you think you might take a closer look at some point? The PR now has merge conflicts but I doubt they'll be hard to resolve if it otherwise looks good.

You would get a lot more respect if you presented facts and arguments instead of trying to import Eastern-European practices to a mostly Western online society.

Also, this is inappropriate and I'd urge you to stop anything resembling that style of discourse if you want to remain in this community. I don't have banning powers but I know that almost no online technical community would stand for comments like that, so you'll probably get banned if you keep it up. I get that you're frustrated, but there's a way to effect change in a community of volunteers, and going around picking fights with established members isn't it.

[butterflya]

@copumpkin Why do you talk about mushy mushy feely stuff like frustrations? It's not like you can measure my frustration over a wire, can you?

I did not tell the owner to take a hike. I asked why @domenkozar wouldn't say this. Please, stop twisting my words.

domenkozar was threatening me and I just described his behavior. He initiated aggression, so I had every right to explain him how wrong he was again, but this time from a social point of view.

@copumpkin How can you not understand that?

There is a reason that he is pinged so much, and it's not because his attention is required. It is, because he can't delegate and cannot or does not want to organize it in such a way that his attention is required less. So, while your point of view is that the community requires his advice on every fart being made, my point of view is that without his guidance everything would work out too. That doesn't mean he isn't the arguably the most important member, because the data says he is. Except this is also a selffulfilling prophecy; if you need to wait a year to get your changes in, even if everyone wants them, then who in their right mind would ever want to send in another PR?

Wikipedia was a failure when only experts could add text. If 10 more people would get commit access to Nix, it wouldn't be the end of the world. I am not saying I want commit access, but if Mathnerd314 or shlevy would get it, I don't see how that's going to reduce the quality. If something is a really bad idea, there still exists such a thing as a revert. By the time an actual release needs to be made the owner could still revert many changes, if so inclined. In fact, everyone who can prove that they ever maintained a C++ codebase for money should be able to get commit access.

[domenkozar]

FYI: I have blocked @butterflya from NixOS organization as it's clearly a double account of already blocked @0xABAB 4 days ago: NixOS/nixpkgs#27194 (comment)

I'd love for this PR to get merged too, but any personal attacks will not be tolerated.

I suggest you stop using your other github accounts, otherwise we'll go through github to take this further on.

[domenkozar]

@bgamari maybe you can open a PR :)

[edolstra]

@Mathnerd314 Do you still have the coroutine-based version around somewhere? I'd like to have another look at it.

[Mathnerd314]

@edolstra Sure, pulled it out of my reflog: master...Mathnerd314:dump-fix-coroutine. Still requires linking to Boost_context though.

[bgamari]

@edolstra any word on this?

[edolstra]

@bgamari No.

[coretemp]

@copumpkin Related to this, do you know whether the below is expected behavior and why it does this? I saw one 80MB file, but 256MiB file. I am also not sure why dumping a file should run out of memory, because the mere act of dumping should take constant space.

Running terraform --version via nix-shell takes 4 seconds on state-of-the-art hardware. That isn't normal, is it?

 nix-shell --pure  ./terraformshell.nix -I nixpkgs=./nixpkgs-master
warning: dumping very large path (> 256 MiB); this may run out of memory

terraformshell.nix:

let
  pkgs = import <nixpkgs> {};

  inherit (pkgs) stdenv;

in stdenv.mkDerivation rec {
  name = "terraform-environment";
  buildInputs = with pkgs; [
    nix-prefetch-git
    git
    nixops
    (terraform_0_11.withPlugins (plugins: [
      plugins.packet
      plugins.local
      plugins.aws
      plugins.dns
      plugins.rabbitmq
    ]))
 ];

 NIX_PATH = "nixpkgs=${pkgs.path}";
}

[KiaraGrouwstra]

This would help solve OoM errors when installing e.g. Mathematica.

edit: workaround as posted in a related thread.

[tbenst]

I just ran into this at NixOS/hydra#366 (comment) when trying to copy a 2.4GB file from nix store. Ultimately solved bumping my machine from 4->16GB of RAM. It seems like memory usage is currently O(n^3) for copy operations.

I appreciate all the hard and fantastic work on nix! I understand that this issue is somewhat tricky to fix. In the meantime, it’d be nice to catch std::bad_alloc and print a more informative error like “Ran out of memory in nix-copy.” I spent a lot of time debugging this issue, including stepping through gdb, and it was difficult to discover.

[edolstra]

Probably you mean O(3n) (i.e. O(n)), since O(n^3) is quite a bit more :-)

[bgamari]

@edolstra, what is the status of this? Do you have an opinion on whether this approach, the coroutines approach, or neither would be acceptable? I find that I run into this issue rather regularly so it would be nice to get a sense of how progress towards a solution can be made.

[tbenst]

@edolstra oops just saw your reply. I actually do think that memory usage is quadratic right now, as moving from 1.9->2.4 -> 2.9 GB files required doubling instance RAM each time, all the way to 32GB. If this pull request fixes this issue it’s most welcome indeed! This issue has been coming up quite a bit among data scientists using nix. Many thanks for all your work and attention :)

[AleXoundOS]

On a fun note, ironically, memory consumption issues have attracted even several inappropriate behavior cases (including a lock and ban in #1681), perhaps confusing the workflow over the years.

[domenkozar]

@edolstra 🙏

[tfc]

Only a minor thing:

How about reducing scope lengths by inverting that outer if-condition and returning early, so the reader needs to track less state in their head:

(also giving repair type const bool helps seeing that no side-effects will happen to it, same with dstPath if applicable... not completely sure, i don't know all the code by heart.)

    if (!repair && isValidPath(dstPath)) {
        return dstPath;
    }

    PathLocks outputLock(singleton<PathSet, Path>(dstPath));

    if (!isValidPath(dstPath)) {

        if (pathExists(dstPath)) deletePath(dstPath);

        copyPath(srcPath, dstPath, filter, recursive);
        canonicalisePathMetaData(dstPath, -1);
        HashResult hash = hashPath(htSHA256, dstPath);
        optimisePath(dstPath); // FIXME: combine with hashPath()

        ValidPathInfo info;
        info.path = dstPath;
        info.hash = hash.first;
        info.narSize = hash.second;
        registerValidPath(info);
    }

    outputLock.setDeletion(true);

    return dstPath;

[blitz]

Is this PR obsolete now that #3684 is resolved?

[Ericson2314]

I think this might fix a few more things, but i just opened #3801 and #3802 which are small changes on top of the recent fix that get those, so I think agree we should close this.

[domenkozar]

@Ericson2314 I still see warnLargeDump being used?

[domenkozar]

Closing as this is completely out of date now.