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

Fix adding large paths to the store #619

Closed
wants to merge 1 commit into from

Conversation

Mathnerd314
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

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 Mathnerd314 force-pushed the dump-fix branch 5 times, most recently from 854316c to 50feda6 Compare August 27, 2015 14:26
@Mathnerd314
Copy link
Contributor Author

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

@domenkozar
Copy link
Member

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

@Mathnerd314
Copy link
Contributor Author

Mathnerd314 commented May 31, 2016

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
Copy link
Member

@edolstra does this need anything else besides resolving conflicts?

@shlevy
Copy link
Member

shlevy commented Jan 12, 2017

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

@ip1981
Copy link

ip1981 commented Feb 3, 2017

Does this cover large directories?

@shlevy
Copy link
Member

shlevy commented Feb 3, 2017

@edolstra ping

@bjornfor
Copy link
Contributor

bjornfor commented May 2, 2017

Bump. Still very interested in this.

@copumpkin
Copy link
Member

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

@butterflya
Copy link

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
Copy link
Member

domenkozar commented Jul 14, 2017

@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
Copy link

butterflya commented Jul 14, 2017

@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
Copy link
Member

copumpkin commented Jul 14, 2017

@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
Copy link

@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
Copy link
Member

domenkozar commented Jul 14, 2017

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
Copy link
Member

@bgamari maybe you can open a PR :)

@edolstra
Copy link
Member

edolstra commented Jan 2, 2018

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

@Mathnerd314
Copy link
Contributor Author

Mathnerd314 commented Jan 4, 2018

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

@bgamari
Copy link
Contributor

bgamari commented Feb 8, 2018

@edolstra any word on this?

@edolstra
Copy link
Member

edolstra commented Feb 8, 2018

@bgamari No.

@coretemp
Copy link

coretemp commented Mar 7, 2018

@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
Copy link

KiaraGrouwstra commented Feb 9, 2019

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

edit: workaround as posted in a related thread.

@tbenst
Copy link

tbenst commented Jan 4, 2020

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
Copy link
Member

edolstra commented Jan 5, 2020

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

@bgamari
Copy link
Contributor

bgamari commented Feb 27, 2020

@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
Copy link

tbenst commented Jun 26, 2020

@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
Copy link

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
Copy link
Member

@edolstra 🙏

outputLock.setDeletion(true);
}

return dstPath;
Copy link
Contributor

@tfc tfc Jul 5, 2020

Choose a reason for hiding this comment

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

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
Copy link
Contributor

blitz commented Jul 13, 2020

Is this PR obsolete now that #3684 is resolved?

@Ericson2314
Copy link
Member

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
Copy link
Member

@Ericson2314 I still see warnLargeDump being used?

@domenkozar
Copy link
Member

Closing as this is completely out of date now.

@domenkozar domenkozar closed this Sep 11, 2020
zolodev pushed a commit to zolodev/nix that referenced this pull request Jan 1, 2024
Remove trailing backtick.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet