From 1df702d34733e69599a6ae21cb366348a2534b7d Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 1 Aug 2012 19:01:50 -0400 Subject: [PATCH 01/23] removeUnusedLinks(): Print stats on disk space saved by hard linking --- src/libstore/gc.cc | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 874efe4d3..a7547d079 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -581,6 +581,8 @@ void LocalStore::removeUnusedLinks() AutoCloseDir dir = opendir(linksDir.c_str()); if (!dir) throw SysError(format("opening directory `%1%'") % linksDir); + unsigned long long actualSize = 0, unsharedSize = 0; + struct dirent * dirent; while (errno = 0, dirent = readdir(dir)) { checkInterrupt(); @@ -592,13 +594,26 @@ void LocalStore::removeUnusedLinks() if (lstat(path.c_str(), &st) == -1) throw SysError(format("statting `%1%'") % path); - if (st.st_nlink != 1) continue; + if (st.st_nlink != 1) { + unsigned long long size = st.st_blocks * 512ULL; + actualSize += size; + unsharedSize += (st.st_nlink - 1) * size; + continue; + } printMsg(lvlTalkative, format("deleting unused link `%1%'") % path); if (unlink(path.c_str()) == -1) throw SysError(format("deleting `%1%'") % path); } + + struct stat st; + if (stat(linksDir.c_str(), &st) == -1) + throw SysError(format("statting `%1%'") % linksDir); + unsigned long long overhead = st.st_blocks * 512ULL; + + printMsg(lvlInfo, format("note: currently hard linking saves %.2f MiB") + % ((unsharedSize - actualSize - overhead) / (1024.0 * 1024.0))); } From 967d066d8e452e59507ebae7585d6f34a4edf687 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 1 Aug 2012 19:14:30 -0400 Subject: [PATCH 02/23] =?UTF-8?q?nix-store=20--gc:=20Make=20=E2=80=98--max?= =?UTF-8?q?-freed=200=E2=80=99=20do=20the=20right=20thing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit That is, delete almost nothing (it will still remove unused links from /nix/store/.links). --- src/libstore/gc.cc | 4 ++-- src/libstore/store-api.cc | 4 ++-- src/libstore/store-api.hh | 3 +-- src/nix-store/nix-store.cc | 2 +- 4 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index a7547d079..7753b3fe2 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -550,7 +550,7 @@ bool LocalStore::tryToDelete(GCState & state, const Path & path) } else deleteGarbage(state, path); - if (state.options.maxFreed && state.results.bytesFreed + state.bytesInvalidated > state.options.maxFreed) { + if (state.results.bytesFreed + state.bytesInvalidated > state.options.maxFreed) { printMsg(lvlInfo, format("deleted or invalidated more than %1% bytes; stopping") % state.options.maxFreed); throw GCLimitReached(); } @@ -675,7 +675,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) throw Error(format("cannot delete path `%1%' since it is still alive") % *i); } - } else { + } else if (options.maxFreed > 0) { if (shouldDelete(state.options.action)) printMsg(lvlError, format("deleting garbage...")); diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index b64988268..dac581e14 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -2,7 +2,7 @@ #include "globals.hh" #include "util.hh" -#include +#include namespace nix { @@ -12,7 +12,7 @@ GCOptions::GCOptions() { action = gcDeleteDead; ignoreLiveness = false; - maxFreed = 0; + maxFreed = ULLONG_MAX; } diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 9ba67852e..c0fb50f23 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -48,8 +48,7 @@ struct GCOptions /* For `gcDeleteSpecific', the paths to delete. */ PathSet pathsToDelete; - /* Stop after at least `maxFreed' bytes have been freed. 0 means - no limit. */ + /* Stop after at least `maxFreed' bytes have been freed. */ unsigned long long maxFreed; GCOptions(); diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc index 82e08fecf..9a675fcd5 100644 --- a/src/nix-store/nix-store.cc +++ b/src/nix-store/nix-store.cc @@ -583,7 +583,7 @@ static void opGC(Strings opFlags, Strings opArgs) else if (*i == "--delete") options.action = GCOptions::gcDeleteDead; else if (*i == "--max-freed") { long long maxFreed = getIntArg(*i, i, opFlags.end()); - options.maxFreed = maxFreed >= 1 ? maxFreed : 1; + options.maxFreed = maxFreed >= 0 ? maxFreed : 0; } else throw UsageError(format("bad sub-operation `%1%' in GC") % *i); From 01d56c1eeca497de247413a64a544605c53d9d41 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 1 Aug 2012 22:34:46 -0400 Subject: [PATCH 03/23] Drop the block count in the garbage collector --- src/libstore/build.cc | 11 +++++------ src/libstore/gc.cc | 5 ++--- src/libstore/local-store.hh | 3 +-- src/libstore/remote-store.cc | 2 +- src/libstore/store-api.hh | 4 ---- src/libutil/util.cc | 21 ++++++++------------- src/libutil/util.hh | 3 +-- src/nix-store/nix-store.cc | 9 ++++----- src/nix-worker/nix-worker.cc | 2 +- 9 files changed, 23 insertions(+), 37 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 290635695..91f235b7a 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -606,18 +606,17 @@ void getOwnership(const Path & path) } -void deletePathWrapped(const Path & path, - unsigned long long & bytesFreed, unsigned long long & blocksFreed) +void deletePathWrapped(const Path & path, unsigned long long & bytesFreed) { try { /* First try to delete it ourselves. */ - deletePath(path, bytesFreed, blocksFreed); + deletePath(path, bytesFreed); } catch (SysError & e) { /* If this failed due to a permission error, then try it with the setuid helper. */ if (haveBuildUsers() && !amPrivileged()) { getOwnership(path); - deletePath(path, bytesFreed, blocksFreed); + deletePath(path, bytesFreed); } else throw; } @@ -626,8 +625,8 @@ void deletePathWrapped(const Path & path, void deletePathWrapped(const Path & path) { - unsigned long long dummy1, dummy2; - deletePathWrapped(path, dummy1, dummy2); + unsigned long long dummy1; + deletePathWrapped(path, dummy1); } diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 7753b3fe2..a1bb4051c 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -425,10 +425,9 @@ bool LocalStore::isActiveTempFile(const GCState & state, void LocalStore::deleteGarbage(GCState & state, const Path & path) { printMsg(lvlInfo, format("deleting `%1%'") % path); - unsigned long long bytesFreed, blocksFreed; - deletePathWrapped(path, bytesFreed, blocksFreed); + unsigned long long bytesFreed; + deletePathWrapped(path, bytesFreed); state.results.bytesFreed += bytesFreed; - state.results.blocksFreed += blocksFreed; } diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 50910f353..721cc6afb 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -304,8 +304,7 @@ void getOwnership(const Path & path); /* Like deletePath(), but changes the ownership of `path' using the setuid wrapper if necessary (and possible). */ -void deletePathWrapped(const Path & path, - unsigned long long & bytesFreed, unsigned long long & blocksFreed); +void deletePathWrapped(const Path & path, unsigned long long & bytesFreed); void deletePathWrapped(const Path & path); diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc index cbb70b2fd..5e7f02749 100644 --- a/src/libstore/remote-store.cc +++ b/src/libstore/remote-store.cc @@ -494,7 +494,7 @@ void RemoteStore::collectGarbage(const GCOptions & options, GCResults & results) results.paths = readStrings(from); results.bytesFreed = readLongLong(from); - results.blocksFreed = readLongLong(from); + readLongLong(from); // obsolete } diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index c0fb50f23..79ae0170d 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -65,13 +65,9 @@ struct GCResults number of bytes that would be or was freed. */ unsigned long long bytesFreed; - /* The number of file system blocks that would be or was freed. */ - unsigned long long blocksFreed; - GCResults() { bytesFreed = 0; - blocksFreed = 0; } }; diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 689fc543a..9d8e4afed 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -297,8 +297,7 @@ void computePathSize(const Path & path, } -static void _deletePath(const Path & path, unsigned long long & bytesFreed, - unsigned long long & blocksFreed) +static void _deletePath(const Path & path, unsigned long long & bytesFreed) { checkInterrupt(); @@ -308,10 +307,8 @@ static void _deletePath(const Path & path, unsigned long long & bytesFreed, if (S_ISDIR(st.st_mode) || S_ISREG(st.st_mode)) makeMutable(path); - if (!S_ISDIR(st.st_mode) && st.st_nlink == 1) { - bytesFreed += st.st_size; - blocksFreed += st.st_blocks; - } + if (!S_ISDIR(st.st_mode) && st.st_nlink == 1) + bytesFreed += st.st_blocks * 512; if (S_ISDIR(st.st_mode)) { Strings names = readDirectory(path); @@ -323,7 +320,7 @@ static void _deletePath(const Path & path, unsigned long long & bytesFreed, } for (Strings::iterator i = names.begin(); i != names.end(); ++i) - _deletePath(path + "/" + *i, bytesFreed, blocksFreed); + _deletePath(path + "/" + *i, bytesFreed); } if (remove(path.c_str()) == -1) @@ -333,19 +330,17 @@ static void _deletePath(const Path & path, unsigned long long & bytesFreed, void deletePath(const Path & path) { - unsigned long long dummy1, dummy2; - deletePath(path, dummy1, dummy2); + unsigned long long dummy; + deletePath(path, dummy); } -void deletePath(const Path & path, unsigned long long & bytesFreed, - unsigned long long & blocksFreed) +void deletePath(const Path & path, unsigned long long & bytesFreed) { startNest(nest, lvlDebug, format("recursively deleting path `%1%'") % path); bytesFreed = 0; - blocksFreed = 0; - _deletePath(path, bytesFreed, blocksFreed); + _deletePath(path, bytesFreed); } diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 9b8656f70..edb7c0fa6 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -80,8 +80,7 @@ void computePathSize(const Path & path, returns the number of bytes and blocks freed. */ void deletePath(const Path & path); -void deletePath(const Path & path, unsigned long long & bytesFreed, - unsigned long long & blocksFreed); +void deletePath(const Path & path, unsigned long long & bytesFreed); /* Make a path read-only recursively. */ void makePathReadOnly(const Path & path); diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc index 9a675fcd5..c182dbe49 100644 --- a/src/nix-store/nix-store.cc +++ b/src/nix-store/nix-store.cc @@ -544,10 +544,9 @@ static void opCheckValidity(Strings opFlags, Strings opArgs) } -static string showBytes(unsigned long long bytes, unsigned long long blocks) +static string showBytes(unsigned long long bytes) { - return (format("%d bytes (%.2f MiB, %d blocks)") - % bytes % (bytes / (1024.0 * 1024.0)) % blocks).str(); + return (format("%.2f MiB") % (bytes / (1024.0 * 1024.0))).str(); } @@ -562,7 +561,7 @@ struct PrintFreed if (show) cout << format("%1% store paths deleted, %2% freed\n") % results.paths.size() - % showBytes(results.bytesFreed, results.blocksFreed); + % showBytes(results.bytesFreed); } }; @@ -735,7 +734,7 @@ static void showOptimiseStats(OptimiseStats & stats) { printMsg(lvlError, format("%1% freed by hard-linking %2% files; there are %3% files with equal contents out of %4% files in total") - % showBytes(stats.bytesFreed, stats.blocksFreed) + % showBytes(stats.bytesFreed) % stats.filesLinked % stats.sameContents % stats.totalFiles); diff --git a/src/nix-worker/nix-worker.cc b/src/nix-worker/nix-worker.cc index 74a619c71..80c0d5060 100644 --- a/src/nix-worker/nix-worker.cc +++ b/src/nix-worker/nix-worker.cc @@ -503,7 +503,7 @@ static void performOp(unsigned int clientVersion, writeStrings(results.paths, to); writeLongLong(results.bytesFreed, to); - writeLongLong(results.blocksFreed, to); + writeLongLong(0, to); // obsolete break; } From 6763084ae53fc0228d50ab94bbbced89c1b14f1c Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 1 Aug 2012 22:43:03 -0400 Subject: [PATCH 04/23] Count bytes freed deleting unused links --- src/libstore/gc.cc | 6 ++++-- src/libstore/local-store.hh | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index a1bb4051c..255dc3aa0 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -575,7 +575,7 @@ bool LocalStore::tryToDelete(GCState & state, const Path & path) safely deleted. FIXME: race condition with optimisePath(): we might see a link count of 1 just before optimisePath() increases the link count. */ -void LocalStore::removeUnusedLinks() +void LocalStore::removeUnusedLinks(const GCState & state) { AutoCloseDir dir = opendir(linksDir.c_str()); if (!dir) throw SysError(format("opening directory `%1%'") % linksDir); @@ -604,6 +604,8 @@ void LocalStore::removeUnusedLinks() if (unlink(path.c_str()) == -1) throw SysError(format("deleting `%1%'") % path); + + state.results.bytesFreed += st.st_blocks * 512; } struct stat st; @@ -732,7 +734,7 @@ void LocalStore::collectGarbage(const GCOptions & options, GCResults & results) /* Clean up the links directory. */ printMsg(lvlError, format("deleting unused links...")); - removeUnusedLinks(); + removeUnusedLinks(state); } diff --git a/src/libstore/local-store.hh b/src/libstore/local-store.hh index 721cc6afb..6a5e3641c 100644 --- a/src/libstore/local-store.hh +++ b/src/libstore/local-store.hh @@ -264,7 +264,7 @@ private: int openGCLock(LockType lockType); - void removeUnusedLinks(); + void removeUnusedLinks(const GCState & state); void startSubstituter(const Path & substituter, RunningSubstituter & runningSubstituter); From 108e14bb189fd0fb291d3494f9f3915070a7052e Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Sun, 5 Aug 2012 18:17:55 -0400 Subject: [PATCH 05/23] Fix race condition when two processes create the same link in /nix/store/.links --- src/libstore/optimise-store.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index c05447f4a..b9b878d2a 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -102,11 +102,11 @@ void LocalStore::optimisePath_(OptimiseStats & stats, const Path & path) /* Nope, create a hard link in the links directory. */ makeMutable(path); MakeImmutable mk1(path); - - if (link(path.c_str(), linkPath.c_str()) == -1) + if (link(path.c_str(), linkPath.c_str()) == 0) return; + if (errno != EEXIST) throw SysError(format("cannot link `%1%' to `%2%'") % linkPath % path); - - return; + /* Fall through if another process created ‘linkPath’ before + we did. */ } /* Yes! We've seen a file with the same contents. Replace the From b6c989b80198badf5f694340c07abc282365aaec Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Sun, 5 Aug 2012 21:41:44 -0400 Subject: [PATCH 06/23] Fix race condition when two processes create a hard link to a file in .links This is a problem because one process may set the immutable bit before the second process has created its link. Addressed random Hydra failures such as: error: cannot rename `/nix/store/.tmp-link-17397-1804289383' to `/nix/store/rsvzm574rlfip3830ac7kmaa028bzl6h-nixos-0.1pre-git/upstart-interface-version': Operation not permitted --- src/libstore/optimise-store.cc | 64 ++++++++++++++++++++-------------- 1 file changed, 37 insertions(+), 27 deletions(-) diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index b9b878d2a..35c8dd48f 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -123,9 +123,6 @@ void LocalStore::optimisePath_(OptimiseStats & stats, const Path & path) printMsg(lvlTalkative, format("linking `%1%' to `%2%'") % path % linkPath); - Path tempLink = (format("%1%/.tmp-link-%2%-%3%") - % nixStore % getpid() % rand()).str(); - /* Make the containing directory writable, but only if it's not the store itself (we don't want or need to mess with its permissions). */ @@ -140,40 +137,53 @@ void LocalStore::optimisePath_(OptimiseStats & stats, const Path & path) so make it mutable first (and make it immutable again when we're done). We also have to make ‘path’ mutable, otherwise rename() will fail to delete it. */ - makeMutable(linkPath); - MakeImmutable mk1(linkPath); - makeMutable(path); MakeImmutable mk2(path); - if (link(linkPath.c_str(), tempLink.c_str()) == -1) { - if (errno == EMLINK) { - /* Too many links to the same file (>= 32000 on most file - systems). This is likely to happen with empty files. - Just shrug and ignore. */ - printMsg(lvlInfo, format("`%1%' has maximum number of links") % linkPath); - return; + /* Another process might be doing the same thing (creating a new + link to ‘linkPath’) and make ‘linkPath’ immutable before we're + done. In that case, just retry. */ + unsigned int retries = 1024; + while (--retries > 0) { + makeMutable(linkPath); + MakeImmutable mk1(linkPath); + + Path tempLink = (format("%1%/.tmp-link-%2%-%3%") + % nixStore % getpid() % rand()).str(); + + if (link(linkPath.c_str(), tempLink.c_str()) == -1) { + if (errno == EMLINK) { + /* Too many links to the same file (>= 32000 on most + file systems). This is likely to happen with empty + files. Just shrug and ignore. */ + printMsg(lvlInfo, format("`%1%' has maximum number of links") % linkPath); + return; + } + if (errno == EPERM) continue; + throw SysError(format("cannot link `%1%' to `%2%'") % tempLink % linkPath); } - throw SysError(format("cannot link `%1%' to `%2%'") % tempLink % linkPath); - } - /* Atomically replace the old file with the new hard link. */ - if (rename(tempLink.c_str(), path.c_str()) == -1) { - if (errno == EMLINK) { - /* Some filesystems generate too many links on the rename, - rather than on the original link. (Probably it - temporarily increases the st_nlink field before - decreasing it again.) */ - printMsg(lvlInfo, format("`%1%' has maximum number of links") % linkPath); - - /* Unlink the temp link. */ + /* Atomically replace the old file with the new hard link. */ + if (rename(tempLink.c_str(), path.c_str()) == -1) { if (unlink(tempLink.c_str()) == -1) printMsg(lvlError, format("unable to unlink `%1%'") % tempLink); - return; + if (errno == EMLINK) { + /* Some filesystems generate too many links on the + rename, rather than on the original link. + (Probably it temporarily increases the st_nlink + field before decreasing it again.) */ + printMsg(lvlInfo, format("`%1%' has maximum number of links") % linkPath); + return; + } + if (errno == EPERM) continue; + throw SysError(format("cannot rename `%1%' to `%2%'") % tempLink % path); } - throw SysError(format("cannot rename `%1%' to `%2%'") % tempLink % path); + + break; } + if (retries == 0) throw Error(format("cannot link `%1%' to `%2%'") % path % linkPath); + stats.filesLinked++; stats.bytesFreed += st.st_size; stats.blocksFreed += st.st_blocks; From d025142f529731f05868f5397f5617011963c8b4 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Sun, 5 Aug 2012 21:45:27 -0400 Subject: [PATCH 07/23] Handle amount of disk space saved by hard linking being negative Fixes bogus messages like "currently hard linking saves 17592186044416.00 MiB". --- src/libstore/gc.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libstore/gc.cc b/src/libstore/gc.cc index 255dc3aa0..406fce672 100644 --- a/src/libstore/gc.cc +++ b/src/libstore/gc.cc @@ -580,7 +580,7 @@ void LocalStore::removeUnusedLinks(const GCState & state) AutoCloseDir dir = opendir(linksDir.c_str()); if (!dir) throw SysError(format("opening directory `%1%'") % linksDir); - unsigned long long actualSize = 0, unsharedSize = 0; + long long actualSize = 0, unsharedSize = 0; struct dirent * dirent; while (errno = 0, dirent = readdir(dir)) { @@ -611,7 +611,7 @@ void LocalStore::removeUnusedLinks(const GCState & state) struct stat st; if (stat(linksDir.c_str(), &st) == -1) throw SysError(format("statting `%1%'") % linksDir); - unsigned long long overhead = st.st_blocks * 512ULL; + long long overhead = st.st_blocks * 512ULL; printMsg(lvlInfo, format("note: currently hard linking saves %.2f MiB") % ((unsharedSize - actualSize - overhead) / (1024.0 * 1024.0))); From 325d1cfebf6c8ad391dc318f984feb3e5731aa5a Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 7 Aug 2012 16:22:54 -0400 Subject: [PATCH 08/23] Don't warn about maximum link count exceeded on 0-byte files --- src/libstore/optimise-store.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index 35c8dd48f..137547a8a 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -156,7 +156,8 @@ void LocalStore::optimisePath_(OptimiseStats & stats, const Path & path) /* Too many links to the same file (>= 32000 on most file systems). This is likely to happen with empty files. Just shrug and ignore. */ - printMsg(lvlInfo, format("`%1%' has maximum number of links") % linkPath); + if (st.st_size) + printMsg(lvlInfo, format("`%1%' has maximum number of links") % linkPath); return; } if (errno == EPERM) continue; @@ -172,7 +173,8 @@ void LocalStore::optimisePath_(OptimiseStats & stats, const Path & path) rename, rather than on the original link. (Probably it temporarily increases the st_nlink field before decreasing it again.) */ - printMsg(lvlInfo, format("`%1%' has maximum number of links") % linkPath); + if (st.st_size) + printMsg(lvlInfo, format("`%1%' has maximum number of links") % linkPath); return; } if (errno == EPERM) continue; From e82767910c649f160d6701e47f606f3b8dde4b29 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Sun, 12 Aug 2012 23:29:28 -0400 Subject: [PATCH 09/23] Add some basic profiling support to the evaluator Setting the environment variable NIX_COUNT_CALLS to 1 enables some basic profiling in the evaluator. It will count calls to functions and primops as well as evaluations of attributes. For example, to see where evaluation of a NixOS configuration spends its time: $ NIX_SHOW_STATS=1 NIX_COUNT_CALLS=1 ./src/nix-instantiate/nix-instantiate '' -A system --readonly-mode ... calls to 39 primops: 239532 head 233962 tail 191252 hasAttr ... calls to 1595 functions: 224157 `/nix/var/nix/profiles/per-user/root/channels/nixos/nixpkgs/pkgs/lib/lists.nix:17:19' 221767 `/nix/var/nix/profiles/per-user/root/channels/nixos/nixpkgs/pkgs/lib/lists.nix:17:14' 221767 `/nix/var/nix/profiles/per-user/root/channels/nixos/nixpkgs/pkgs/lib/lists.nix:17:10' ... evaluations of 7088 attributes: 167377 undefined position 132459 `/nix/var/nix/profiles/per-user/root/channels/nixos/nixpkgs/pkgs/lib/attrsets.nix:119:41' 47322 `/nix/var/nix/profiles/per-user/root/channels/nixos/nixpkgs/pkgs/lib/attrsets.nix:13:21' ... --- src/libexpr/eval.cc | 38 +++++++++++++++++++++++++++++++++++++- src/libexpr/eval.hh | 16 ++++++++++++++-- src/libexpr/nixexpr.hh | 9 +++++++++ src/libexpr/primops.cc | 3 ++- src/libutil/util.hh | 3 +++ 5 files changed, 65 insertions(+), 4 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index cf7c62ad2..2d41eba0e 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -144,6 +144,7 @@ EvalState::EvalState() { nrEnvs = nrValuesInEnvs = nrValues = nrListElems = 0; nrAttrsets = nrOpUpdates = nrOpUpdateValuesCopied = 0; + countCalls = getEnv("NIX_COUNT_CALLS", "0") != "0"; #if HAVE_BOEHMGC static bool gcInitialised = true; @@ -300,8 +301,10 @@ inline Value * EvalState::lookupVar(Env * env, const VarRef & var) if (var.fromWith) { while (1) { Bindings::iterator j = env->values[0]->attrs->find(var.name); - if (j != env->values[0]->attrs->end()) + if (j != env->values[0]->attrs->end()) { + if (countCalls && j->pos) attrSelects[*j->pos]++; return j->value; + } if (env->prevWith == 0) throwEvalError("undefined variable `%1%'", var.name); for (unsigned int l = env->prevWith; l; --l, env = env->up) ; @@ -619,8 +622,10 @@ void ExprSelect::eval(EvalState & state, Env & env, Value & v) } vAttrs = j->value; pos = j->pos; + if (state.countCalls && pos) state.attrSelects[*pos]++; } + state.forceValue(*vAttrs); } catch (Error & e) { @@ -700,6 +705,7 @@ void EvalState::callFunction(Value & fun, Value & arg, Value & v) vArgs[n--] = arg->primOpApp.right; /* And call the primop. */ + if (countCalls) primOpCalls[primOp->primOp->name]++; try { primOp->primOp->fun(*this, vArgs, v); } catch (Error & e) { @@ -760,6 +766,8 @@ void EvalState::callFunction(Value & fun, Value & arg, Value & v) throwTypeError("function at %1% called with unexpected argument", fun.lambda.fun->pos); } + if (countCalls) functionCalls[fun.lambda.fun->pos]++; + try { fun.lambda.fun->body->eval(*this, env2, v); } catch (Error & e) { @@ -1216,6 +1224,34 @@ void EvalState::printStats() printMsg(v, format(" number of thunks: %1%") % nrThunks); printMsg(v, format(" number of thunks avoided: %1%") % nrAvoided); printMsg(v, format(" number of attr lookups: %1%") % nrLookups); + + if (countCalls) { + + printMsg(v, format("calls to %1% primops:") % primOpCalls.size()); + typedef std::multimap PrimOpCalls_; + std::multimap primOpCalls_; + foreach (PrimOpCalls::iterator, i, primOpCalls) + primOpCalls_.insert(std::pair(i->second, i->first)); + foreach_reverse (PrimOpCalls_::reverse_iterator, i, primOpCalls_) + printMsg(v, format("%1$10d %2%") % i->first % i->second); + + printMsg(v, format("calls to %1% functions:") % functionCalls.size()); + typedef std::multimap FunctionCalls_; + std::multimap functionCalls_; + foreach (FunctionCalls::iterator, i, functionCalls) + functionCalls_.insert(std::pair(i->second, i->first)); + foreach_reverse (FunctionCalls_::reverse_iterator, i, functionCalls_) + printMsg(v, format("%1$10d %2%") % i->first % i->second); + + printMsg(v, format("evaluations of %1% attributes:") % attrSelects.size()); + typedef std::multimap AttrSelects_; + std::multimap attrSelects_; + foreach (AttrSelects::iterator, i, attrSelects) + attrSelects_.insert(std::pair(i->second, i->first)); + foreach_reverse (AttrSelects_::reverse_iterator, i, attrSelects_) + printMsg(v, format("%1$10d %2%") % i->first % i->second); + + } } diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 5103ae8ce..540f1e200 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -244,9 +244,21 @@ private: unsigned long nrAttrsets; unsigned long nrOpUpdates; unsigned long nrOpUpdateValuesCopied; - - friend class RecursionCounter; + + bool countCalls; + + typedef std::map PrimOpCalls; + PrimOpCalls primOpCalls; + + typedef std::map FunctionCalls; + FunctionCalls functionCalls; + + typedef std::map AttrSelects; + AttrSelects attrSelects; + friend class ExprOpUpdate; + friend class ExprSelect; + friend void prim_getAttr(EvalState & state, Value * * args, Value & v); }; diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 4c1a0bb2d..bc6c3287c 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -27,6 +27,15 @@ struct Pos Pos() : line(0), column(0) { }; Pos(const string & file, unsigned int line, unsigned int column) : file(file), line(line), column(column) { }; + bool operator < (const Pos & p2) const + { + int d = file.compare(p2.file); + if (d < 0) return true; + if (d > 0) return false; + if (line < p2.line) return true; + if (line > p2.line) return false; + return column < p2.column; + } }; extern Pos noPos; diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 0d4efc47e..b833413ea 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -713,7 +713,7 @@ static void prim_attrNames(EvalState & state, Value * * args, Value & v) /* Dynamic version of the `.' operator. */ -static void prim_getAttr(EvalState & state, Value * * args, Value & v) +void prim_getAttr(EvalState & state, Value * * args, Value & v) { string attr = state.forceStringNoCtx(*args[0]); state.forceAttrs(*args[1]); @@ -722,6 +722,7 @@ static void prim_getAttr(EvalState & state, Value * * args, Value & v) if (i == args[1]->attrs->end()) throw EvalError(format("attribute `%1%' missing") % attr); // !!! add to stack trace? + if (state.countCalls && i->pos) state.attrSelects[*i->pos]++; state.forceValue(*i->value); v = *i->value; } diff --git a/src/libutil/util.hh b/src/libutil/util.hh index edb7c0fa6..dc38a53ca 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -17,6 +17,9 @@ namespace nix { #define foreach(it_type, it, collection) \ for (it_type it = (collection).begin(); it != (collection).end(); ++it) +#define foreach_reverse(it_type, it, collection) \ + for (it_type it = (collection).rbegin(); it != (collection).rend(); ++it) + /* Return an environment variable. */ string getEnv(const string & key, const string & def = ""); From 62f72eb9e1a4421a9d4ea3e06f467e49869c0e51 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Sun, 12 Aug 2012 23:41:48 -0400 Subject: [PATCH 10/23] Add some more evaluations stats --- src/libexpr/eval.cc | 7 +++++++ src/libexpr/eval.hh | 6 +++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 2d41eba0e..a1a227c8a 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -144,6 +144,7 @@ EvalState::EvalState() { nrEnvs = nrValuesInEnvs = nrValues = nrListElems = 0; nrAttrsets = nrOpUpdates = nrOpUpdateValuesCopied = 0; + nrListConcats = nrPrimOpCalls = nrFunctionCalls = 0; countCalls = getEnv("NIX_COUNT_CALLS", "0") != "0"; #if HAVE_BOEHMGC @@ -705,6 +706,7 @@ void EvalState::callFunction(Value & fun, Value & arg, Value & v) vArgs[n--] = arg->primOpApp.right; /* And call the primop. */ + nrPrimOpCalls++; if (countCalls) primOpCalls[primOp->primOp->name]++; try { primOp->primOp->fun(*this, vArgs, v); @@ -766,6 +768,7 @@ void EvalState::callFunction(Value & fun, Value & arg, Value & v) throwTypeError("function at %1% called with unexpected argument", fun.lambda.fun->pos); } + nrFunctionCalls++; if (countCalls) functionCalls[fun.lambda.fun->pos]++; try { @@ -909,6 +912,7 @@ void ExprOpUpdate::eval(EvalState & state, Env & env, Value & v) void ExprOpConcatLists::eval(EvalState & state, Env & env, Value & v) { + state.nrListConcats++; Value v1; e1->eval(state, env, v1); state.forceList(v1); Value v2; e2->eval(state, env, v2); @@ -1215,6 +1219,7 @@ void EvalState::printStats() % nrEnvs % (nrEnvs * sizeof(Env) + nrValuesInEnvs * sizeof(Value *))); printMsg(v, format(" list elements: %1% (%2% bytes)") % nrListElems % (nrListElems * sizeof(Value *))); + printMsg(v, format(" list concatenations: %1%") % nrListConcats); printMsg(v, format(" values allocated: %1% (%2% bytes)") % nrValues % (nrValues * sizeof(Value))); printMsg(v, format(" attribute sets allocated: %1%") % nrAttrsets); @@ -1224,6 +1229,8 @@ void EvalState::printStats() printMsg(v, format(" number of thunks: %1%") % nrThunks); printMsg(v, format(" number of thunks avoided: %1%") % nrAvoided); printMsg(v, format(" number of attr lookups: %1%") % nrLookups); + printMsg(v, format(" number of primop calls: %1%") % nrPrimOpCalls); + printMsg(v, format(" number of function calls: %1%") % nrFunctionCalls); if (countCalls) { diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 540f1e200..4e66c4dfc 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -236,7 +236,7 @@ public: void printStats(); private: - + unsigned long nrEnvs; unsigned long nrValuesInEnvs; unsigned long nrValues; @@ -244,6 +244,9 @@ private: unsigned long nrAttrsets; unsigned long nrOpUpdates; unsigned long nrOpUpdateValuesCopied; + unsigned long nrListConcats; + unsigned long nrPrimOpCalls; + unsigned long nrFunctionCalls; bool countCalls; @@ -257,6 +260,7 @@ private: AttrSelects attrSelects; friend class ExprOpUpdate; + friend class ExprOpConcatLists; friend class ExprSelect; friend void prim_getAttr(EvalState & state, Value * * args, Value & v); }; From 4ccd48ce2478cbe1263605838969f89d5b745f0a Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 13 Aug 2012 00:28:08 -0400 Subject: [PATCH 11/23] Add a "filter" primop Evaluation of a NixOS configuration spends quite a lot of time in the "filter" function in Nixpkgs. As implemented in Nixpkgs, this is a O(n^2) operation, so it's a good candidate for providing a more efficient (i.e. primop) implementation. Using it gives a ~10% speed increase and a significant reduction in the number of evaluations. Statistics before (on a NixOS system config): time elapsed: 1.3258 size of a value: 24 environments allocated: 1980939 (50127080 bytes) list elements: 14679308 (117434464 bytes) list concatenations: 50828 values allocated: 2098938 (50374512 bytes) attribute sets allocated: 392040 right-biased unions: 186334 values copied in right-biased unions: 591137 symbols in symbol table: 18271 number of thunks: 1645752 number of thunks avoided: 1921196 number of attr lookups: 430798 number of primop calls: 838807 number of function calls: 1930107 Statistics after: time elapsed: 1.17982 size of a value: 24 environments allocated: 1543334 (39624560 bytes) list elements: 9612638 (76901104 bytes) list concatenations: 37434 values allocated: 1854933 (44518392 bytes) attribute sets allocated: 392040 right-biased unions: 186334 values copied in right-biased unions: 591137 symbols in symbol table: 18272 number of thunks: 1392467 number of thunks avoided: 1507311 number of attr lookups: 430801 number of primop calls: 691600 number of function calls: 1492502 --- src/libexpr/eval.cc | 2 +- src/libexpr/primops.cc | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index a1a227c8a..871dfa64c 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -724,7 +724,7 @@ void EvalState::callFunction(Value & fun, Value & arg, Value & v) } if (fun.type != tLambda) - throwTypeError("attempt to call something which is neither a function nor a primop (built-in operation) but %1%", + throwTypeError("attempt to call something which is not a function but %1%", showType(fun)); unsigned int size = diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index b833413ea..db7aa9e89 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -906,6 +906,30 @@ static void prim_map(EvalState & state, Value * * args, Value & v) } +/* Filter a list using a predicate; that is, return a list containing + every element from the list for which the predicate function + returns true. */ +static void prim_filter(EvalState & state, Value * * args, Value & v) +{ + state.forceFunction(*args[0]); + state.forceList(*args[1]); + + // FIXME: putting this on the stack is risky. + Value * vs[args[1]->list.length]; + unsigned int k = 0; + + for (unsigned int n = 0; n < args[1]->list.length; ++n) { + Value res; + state.callFunction(*args[0], *args[1]->list.elems[n], res); + if (state.forceBool(res)) + vs[k++] = args[1]->list.elems[n]; + } + + state.mkList(v, k); + for (unsigned int n = 0; n < k; ++n) v.list.elems[n] = vs[n]; +} + + /* Return the length of a list. This is an O(1) time operation. */ static void prim_length(EvalState & state, Value * * args, Value & v) { @@ -1120,6 +1144,7 @@ void EvalState::createBaseEnv() addPrimOp("__head", 1, prim_head); addPrimOp("__tail", 1, prim_tail); addPrimOp("map", 2, prim_map); + addPrimOp("__filter", 2, prim_filter); addPrimOp("__length", 1, prim_length); // Integer arithmetic From b9e5b908ed29bfb6cd82837f9f57293c1f63e999 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 13 Aug 2012 01:05:35 -0400 Subject: [PATCH 12/23] =?UTF-8?q?Provide=20an=20efficient=20implementation?= =?UTF-8?q?=20of=20=E2=80=98elem=E2=80=99?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The one in Nixpkgs is O(n^2), this one is O(n). Big reduction in the number of list allocations. Statistics before (on a NixOS system config): time elapsed: 1.17982 size of a value: 24 environments allocated: 1543334 (39624560 bytes) list elements: 9612638 (76901104 bytes) list concatenations: 37434 values allocated: 1854933 (44518392 bytes) attribute sets allocated: 392040 right-biased unions: 186334 values copied in right-biased unions: 591137 symbols in symbol table: 18272 number of thunks: 1392467 number of thunks avoided: 1507311 number of attr lookups: 430801 number of primop calls: 691600 number of function calls: 1492502 Statistics after: time elapsed: 1.08683 size of a value: 24 environments allocated: 1384376 (35809568 bytes) list elements: 6946783 (55574264 bytes) list concatenations: 37434 values allocated: 1760440 (42250560 bytes) attribute sets allocated: 392040 right-biased unions: 186334 values copied in right-biased unions: 591137 symbols in symbol table: 18273 number of thunks: 1297673 number of thunks avoided: 1380759 number of attr lookups: 430802 number of primop calls: 628912 number of function calls: 1333544 --- src/libexpr/primops.cc | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index db7aa9e89..aa27df6ad 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -880,7 +880,8 @@ static void prim_head(EvalState & state, Value * * args, Value & v) /* Return a list consisting of everything but the the first element of - a list. */ + a list. Warning: this function takes O(n) time, so you probably + don't want to use it! */ static void prim_tail(EvalState & state, Value * * args, Value & v) { state.forceList(*args[0]); @@ -930,6 +931,19 @@ static void prim_filter(EvalState & state, Value * * args, Value & v) } +static void prim_elem(EvalState & state, Value * * args, Value & v) +{ + bool res = false; + state.forceList(*args[1]); + for (unsigned int n = 0; n < args[1]->list.length; ++n) + if (state.eqValues(*args[0], *args[1]->list.elems[n])) { + res = true; + break; + } + mkBool(v, res); +} + + /* Return the length of a list. This is an O(1) time operation. */ static void prim_length(EvalState & state, Value * * args, Value & v) { @@ -1145,8 +1159,9 @@ void EvalState::createBaseEnv() addPrimOp("__tail", 1, prim_tail); addPrimOp("map", 2, prim_map); addPrimOp("__filter", 2, prim_filter); + addPrimOp("__elem", 2, prim_elem); addPrimOp("__length", 1, prim_length); - + // Integer arithmetic addPrimOp("__add", 2, prim_add); addPrimOp("__sub", 2, prim_sub); From 198d0338be7c105b6dbd707f98e0c223a8358240 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 13 Aug 2012 01:53:10 -0400 Subject: [PATCH 13/23] =?UTF-8?q?Add=20a=20primop=20=E2=80=98concatLists?= =?UTF-8?q?=E2=80=99?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This can serve as a generic efficient list builder. For instance, the function ‘catAttrs’ in Nixpkgs can be rewritten from attr: l: fold (s: l: if hasAttr attr s then [(getAttr attr s)] ++ l else l) [] l to attr: l: builtins.concatLists (map (s: if hasAttr attr s then [(getAttr attr s)] else []) l) Statistics before: time elapsed: 1.08683 size of a value: 24 environments allocated: 1384376 (35809568 bytes) list elements: 6946783 (55574264 bytes) list concatenations: 37434 values allocated: 1760440 (42250560 bytes) attribute sets allocated: 392040 right-biased unions: 186334 values copied in right-biased unions: 591137 symbols in symbol table: 18273 number of thunks: 1297673 number of thunks avoided: 1380759 number of attr lookups: 430802 number of primop calls: 628912 number of function calls: 1333544 Statistics after (including new catAttrs): time elapsed: 0.959854 size of a value: 24 environments allocated: 1010198 (26829296 bytes) list elements: 1984878 (15879024 bytes) list concatenations: 30488 values allocated: 1589760 (38154240 bytes) attribute sets allocated: 392040 right-biased unions: 186334 values copied in right-biased unions: 591137 symbols in symbol table: 18274 number of thunks: 1040925 number of thunks avoided: 1038428 number of attr lookups: 438419 number of primop calls: 474844 number of function calls: 959366 --- src/libexpr/eval.cc | 29 +++++++++++++++++++++-------- src/libexpr/eval.hh | 2 ++ src/libexpr/primops.cc | 10 ++++++++++ 3 files changed, 33 insertions(+), 8 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 871dfa64c..5351ca0c3 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -912,16 +912,29 @@ void ExprOpUpdate::eval(EvalState & state, Env & env, Value & v) void ExprOpConcatLists::eval(EvalState & state, Env & env, Value & v) { - state.nrListConcats++; Value v1; e1->eval(state, env, v1); - state.forceList(v1); Value v2; e2->eval(state, env, v2); - state.forceList(v2); - state.mkList(v, v1.list.length + v2.list.length); - for (unsigned int n = 0; n < v1.list.length; ++n) - v.list.elems[n] = v1.list.elems[n]; - for (unsigned int n = 0; n < v2.list.length; ++n) - v.list.elems[n + v1.list.length] = v2.list.elems[n]; + Value * lists[2] = { &v1, &v2 }; + state.concatLists(v, 2, lists); +} + + +void EvalState::concatLists(Value & v, unsigned int nrLists, Value * * lists) +{ + nrListConcats++; + + unsigned int len = 0; + for (unsigned int n = 0; n < nrLists; ++n) { + forceList(*lists[n]); + len += lists[n]->list.length; + } + + mkList(v, len); + for (unsigned int n = 0, pos = 0; n < nrLists; ++n) { + unsigned int l = lists[n]->list.length; + memcpy(v.list.elems + pos, lists[n]->list.elems, l * sizeof(Value *)); + pos += l; + } } diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 4e66c4dfc..a1f26a056 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -232,6 +232,8 @@ public: void mkAttrs(Value & v, unsigned int expected); void mkThunk_(Value & v, Expr * expr); + void concatLists(Value & v, unsigned int nrLists, Value * * lists); + /* Print statistics. */ void printStats(); diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index aa27df6ad..f5c004044 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -931,6 +931,7 @@ static void prim_filter(EvalState & state, Value * * args, Value & v) } +/* Return true if a list contains a given element. */ static void prim_elem(EvalState & state, Value * * args, Value & v) { bool res = false; @@ -944,6 +945,14 @@ static void prim_elem(EvalState & state, Value * * args, Value & v) } +/* Concatenate a list of lists. */ +static void prim_concatLists(EvalState & state, Value * * args, Value & v) +{ + state.forceList(*args[0]); + state.concatLists(v, args[0]->list.length, args[0]->list.elems); +} + + /* Return the length of a list. This is an O(1) time operation. */ static void prim_length(EvalState & state, Value * * args, Value & v) { @@ -1160,6 +1169,7 @@ void EvalState::createBaseEnv() addPrimOp("map", 2, prim_map); addPrimOp("__filter", 2, prim_filter); addPrimOp("__elem", 2, prim_elem); + addPrimOp("__concatLists", 1, prim_concatLists); addPrimOp("__length", 1, prim_length); // Integer arithmetic From 9c2d63084bd4f6a04210cd52b4fce054d248bc6b Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 13 Aug 2012 13:46:42 -0400 Subject: [PATCH 14/23] =?UTF-8?q?Add=20a=20primop=20=E2=80=98elemAt?= =?UTF-8?q?=E2=80=99=20to=20get=20an=20element=20from=20a=20list?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/libexpr/primops.cc | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index f5c004044..cb09d8ff0 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -868,14 +868,27 @@ static void prim_isList(EvalState & state, Value * * args, Value & v) } +static void elemAt(EvalState & state, Value & list, int n, Value & v) +{ + state.forceList(list); + if (n < 0 || n >= list.list.length) + throw Error(format("list index %1% is out of bounds") % n); + state.forceValue(*list.list.elems[n]); + v = *list.list.elems[n]; +} + + +/* Return the n-1'th element of a list. */ +static void prim_elemAt(EvalState & state, Value * * args, Value & v) +{ + elemAt(state, *args[0], state.forceInt(*args[1]), v); +} + + /* Return the first element of a list. */ static void prim_head(EvalState & state, Value * * args, Value & v) { - state.forceList(*args[0]); - if (args[0]->list.length == 0) - throw Error("`head' called on an empty list"); - state.forceValue(*args[0]->list.elems[0]); - v = *args[0]->list.elems[0]; + elemAt(state, *args[0], 0, v); } @@ -1164,6 +1177,7 @@ void EvalState::createBaseEnv() // Lists addPrimOp("__isList", 1, prim_isList); + addPrimOp("__elemAt", 2, prim_elemAt); addPrimOp("__head", 1, prim_head); addPrimOp("__tail", 1, prim_tail); addPrimOp("map", 2, prim_map); From 3e89ef597ce00dbf82a937aad9efab3c9c7b6dcf Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 13 Aug 2012 14:58:54 -0400 Subject: [PATCH 15/23] Optimise concatenating a list to an empty list More precisely, in concatLists, if all lists except one are empty, then just return the non-empty list. This reduces the number of list element allocations by 32% when evaluating a NixOS system configuration. --- src/libexpr/eval.cc | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 5351ca0c3..517a1151e 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -922,11 +922,19 @@ void ExprOpConcatLists::eval(EvalState & state, Env & env, Value & v) void EvalState::concatLists(Value & v, unsigned int nrLists, Value * * lists) { nrListConcats++; - + + Value * nonEmpty = 0; unsigned int len = 0; for (unsigned int n = 0; n < nrLists; ++n) { forceList(*lists[n]); - len += lists[n]->list.length; + unsigned int l = lists[n]->list.length; + len += l; + if (l) nonEmpty = lists[n]; + } + + if (nonEmpty && len == nonEmpty->list.length) { + v = *nonEmpty; + return; } mkList(v, len); From e5c589d271c62f57cd2e7eb7d9841f67d8845ff4 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 13 Aug 2012 15:02:09 -0400 Subject: [PATCH 16/23] Don't allocate empty lists This saves about 4 MB when evaluating a NixOS system configuration. --- src/libexpr/eval.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 517a1151e..cb7c22e29 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -348,7 +348,7 @@ void EvalState::mkList(Value & v, unsigned int length) { v.type = tList; v.list.length = length; - v.list.elems = (Value * *) GC_MALLOC(length * sizeof(Value *)); + v.list.elems = length ? (Value * *) GC_MALLOC(length * sizeof(Value *)) : 0; nrListElems += length; } From 767101824af1fe41b6e50791b21112c6a8d7457f Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 13 Aug 2012 15:10:29 -0400 Subject: [PATCH 17/23] Avoid concatenating lists of one string --- src/libexpr/eval.cc | 2 +- src/libexpr/parser.y | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index cb7c22e29..fadfb3b53 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -965,7 +965,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v) isPath = vStr.type == tPath; first = false; } - + s << state.coerceToString(vStr, context, false, !isPath); } diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 095e28843..1819da5e1 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -203,7 +203,7 @@ static Expr * stripIndentation(SymbolTable & symbols, vector & es) es2->push_back(new ExprString(symbols.create(s2))); } - return new ExprConcatStrings(es2); + return es2->size() == 1 ? (*es2)[0] : new ExprConcatStrings(es2); } From 862c4c5ec509e05815d99fb4b80558974148b8c5 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Sun, 19 Aug 2012 16:32:42 -0400 Subject: [PATCH 18/23] =?UTF-8?q?Fix=201755=20permission=20on=20temporary?= =?UTF-8?q?=20directories=20left=20behind=20by=20=E2=80=98-K=E2=80=99?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/libstore/build.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 91f235b7a..a7aea164c 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -1471,9 +1471,9 @@ HookReply DerivationGoal::tryBuildHook() } -void chmod(const Path & path, mode_t mode) +void chmod_(const Path & path, mode_t mode) { - if (::chmod(path.c_str(), 01777) == -1) + if (chmod(path.c_str(), mode) == -1) throw SysError(format("setting permissions on `%1%'") % path); } @@ -1675,7 +1675,7 @@ void DerivationGoal::startBuilder() instead.) */ Path chrootTmpDir = chrootRootDir + "/tmp"; createDirs(chrootTmpDir); - chmod(chrootTmpDir, 01777); + chmod_(chrootTmpDir, 01777); /* Create a /etc/passwd with entries for the build user and the nobody account. The latter is kind of a hack to support @@ -1719,7 +1719,7 @@ void DerivationGoal::startBuilder() precaution, make the fake Nix store only writable by the build user. */ createDirs(chrootRootDir + nixStore); - chmod(chrootRootDir + nixStore, 01777); + chmod_(chrootRootDir + nixStore, 01777); foreach (PathSet::iterator, i, inputPaths) { struct stat st; From f0eab0636b73a4f16b7639d30956d9072d5573cb Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 20 Aug 2012 15:27:00 -0400 Subject: [PATCH 19/23] Don't bind-mount /proc since we mount our own --- src/libstore/build.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index a7aea164c..9da8084bf 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -1859,16 +1859,16 @@ void DerivationGoal::initChild() foreach (PathSet::iterator, i, dirsInChroot) { Path source = *i; Path target = chrootRootDir + source; + if (source == "/proc") continue; // backwards compatibility debug(format("bind mounting `%1%' to `%2%'") % source % target); - createDirs(target); - if (mount(source.c_str(), target.c_str(), "", MS_BIND, 0) == -1) throw SysError(format("bind mount from `%1%' to `%2%' failed") % source % target); } /* Bind a new instance of procfs on /proc to reflect our private PID namespace. */ + createDirs(chrootRootDir + "/proc"); if (mount("none", (chrootRootDir + "/proc").c_str(), "proc", 0, 0) == -1) throw SysError("mounting /proc"); From 56e30e161cd309addb5aa95ba02a8d3371846228 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 20 Aug 2012 15:27:30 -0400 Subject: [PATCH 20/23] In the chroot, make all mounted filesystems private This is required on systemd, which mounts filesystems as "shared" subtrees. Changes to shared trees in a private mount namespace are propagated to the outside world, which is bad. --- src/libstore/build.cc | 18 ++++++++++++++++++ src/libutil/util.cc | 4 ++-- src/libutil/util.hh | 2 +- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 9da8084bf..0ed69614b 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -1853,6 +1853,24 @@ void DerivationGoal::initChild() char domainname[] = "(none)"; // kernel default setdomainname(domainname, sizeof(domainname)); + /* Make all filesystems private. This is necessary + because subtrees may have been mounted as "shared" + (MS_SHARED). (Systemd does this, for instance.) Even + though we have a private mount namespace, mounting + filesystems on top of a shared subtree still propagates + outside of the namespace. Making a subtree private is + local to the namespace, though, so setting MS_PRIVATE + does not affect the outside world. */ + Strings mounts = tokenizeString(readFile("/proc/self/mountinfo", true), "\n"); + foreach (Strings::iterator, i, mounts) { + Strings fields = tokenizeString(*i, " "); + assert(fields.size() >= 5); + Strings::iterator j = fields.begin(); + std::advance(j, 4); + if (mount(0, j->c_str(), 0, MS_PRIVATE, 0) == -1) + throw SysError(format("unable to make filesystem `%1%' private") % *j); + } + /* Bind-mount all the directories from the "host" filesystem that we want in the chroot environment. */ diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 9d8e4afed..fe4fedfa5 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -224,12 +224,12 @@ string readFile(int fd) } -string readFile(const Path & path) +string readFile(const Path & path, bool drain) { AutoCloseFD fd = open(path.c_str(), O_RDONLY); if (fd == -1) throw SysError(format("opening file `%1%'") % path); - return readFile(fd); + return drain ? drainFD(fd) : readFile(fd); } diff --git a/src/libutil/util.hh b/src/libutil/util.hh index dc38a53ca..22992bbaf 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -63,7 +63,7 @@ Strings readDirectory(const Path & path); /* Read the contents of a file into a string. */ string readFile(int fd); -string readFile(const Path & path); +string readFile(const Path & path, bool drain = false); /* Write a string to a file. */ void writeFile(const Path & path, const string & s); From d950cfe70b2b70e938ece672dbccedfd4413c295 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 20 Aug 2012 15:55:49 -0400 Subject: [PATCH 21/23] Check if MS_PRIVATE is defined http://hydra.nixos.org/build/2955671 --- src/libstore/build.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libstore/build.cc b/src/libstore/build.cc index 0ed69614b..a77644054 100644 --- a/src/libstore/build.cc +++ b/src/libstore/build.cc @@ -45,7 +45,7 @@ #include #endif -#define CHROOT_ENABLED HAVE_CHROOT && HAVE_UNSHARE && HAVE_SYS_MOUNT_H && defined(MS_BIND) && defined(CLONE_NEWNS) +#define CHROOT_ENABLED HAVE_CHROOT && HAVE_UNSHARE && HAVE_SYS_MOUNT_H && defined(MS_BIND) && defined(MS_PRIVATE) && defined(CLONE_NEWNS) #if CHROOT_ENABLED #include From 4aa1e5c55484ac02d28883292ee5c5806f5e4664 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Wed, 22 Aug 2012 10:58:24 -0400 Subject: [PATCH 22/23] Receive reserveSpace before calling startWork() Otherwise we can get a SIGPOLL. Reported by Ludovic. --- src/nix-worker/nix-worker.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/nix-worker/nix-worker.cc b/src/nix-worker/nix-worker.cc index 80c0d5060..7099eb9ee 100644 --- a/src/nix-worker/nix-worker.cc +++ b/src/nix-worker/nix-worker.cc @@ -619,6 +619,10 @@ static void processConnection() to.flush(); unsigned int clientVersion = readInt(from); + bool reserveSpace = true; + if (GET_PROTOCOL_MINOR(clientVersion) >= 11) + reserveSpace = readInt(from) != 0; + /* Send startup error messages to the client. */ startWork(); @@ -634,10 +638,6 @@ static void processConnection() throw Error("if you run `nix-worker' as root, then you MUST set `build-users-group'!"); #endif - bool reserveSpace = true; - if (GET_PROTOCOL_MINOR(clientVersion) >= 11) - reserveSpace = readInt(from) != 0; - /* Open the store. */ store = boost::shared_ptr(new LocalStore(reserveSpace)); From a9e6752bbd888ab8fbc1cda6e4d539b2858c4cef Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Fri, 24 Aug 2012 16:58:11 -0400 Subject: [PATCH 23/23] Include the output name in the GC root link Output names are now appended to resulting GC symlinks, e.g. by nix-build. For backwards compatibility, if the output is named "out", nothing is appended. E.g. doing "nix-build -A foo" on a derivation that produces outputs "out", "bin" and "dev" will produce symlinks "./result", "./result-bin" and "./result-dev", respectively. --- scripts/nix-build.in | 12 ++++++------ src/libmain/shared.cc | 10 ---------- src/libmain/shared.hh | 1 - src/nix-instantiate/nix-instantiate.cc | 8 +++++--- src/nix-store/nix-store.cc | 10 +++++++--- 5 files changed, 18 insertions(+), 23 deletions(-) diff --git a/scripts/nix-build.in b/scripts/nix-build.in index afe0679a4..3ca3a1aaf 100755 --- a/scripts/nix-build.in +++ b/scripts/nix-build.in @@ -46,7 +46,7 @@ Flags: --no-out-link: do not create the `result' symlink --out-link / -o NAME: create symlink NAME instead of `result' --attr / -A ATTR: select a specific attribute from the Nix expression - + --run-env: build dependencies of the specified derivation, then start a shell with the environment of the derivation --command: command to run with `--run-env' @@ -114,7 +114,7 @@ EOF push @buildArgs, "--dry-run"; $dryRun = 1; } - + elsif ($arg eq "--show-trace") { push @instArgs, $arg; } @@ -122,22 +122,22 @@ EOF elsif ($arg eq "-") { @exprs = ("-"); } - + elsif ($arg eq "--verbose" or substr($arg, 0, 2) eq "-v") { push @buildArgs, $arg; push @instArgs, $arg; $verbose = 1; } - + elsif ($arg eq "--quiet") { push @buildArgs, $arg; push @instArgs, $arg; } - + elsif ($arg eq "--run-env") { $runEnv = 1; } - + elsif ($arg eq "--command") { $n++; die "$0: `$arg' requires an argument\n" unless $n < scalar @ARGV; diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index d39816586..026db4173 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -33,16 +33,6 @@ static void sigintHandler(int signo) } -Path makeRootName(const Path & gcRoot, int & counter) -{ - counter++; - if (counter == 1) - return gcRoot; - else - return (format("%1%-%2%") % gcRoot % counter).str(); -} - - void printGCWarning() { static bool haveWarned = false; diff --git a/src/libmain/shared.hh b/src/libmain/shared.hh index 7849e10e3..c69879a12 100644 --- a/src/libmain/shared.hh +++ b/src/libmain/shared.hh @@ -26,7 +26,6 @@ MakeError(UsageError, nix::Error); class StoreAPI; /* Ugh. No better place to put this. */ -Path makeRootName(const Path & gcRoot, int & counter); void printGCWarning(); void printMissing(StoreAPI & store, const PathSet & paths); diff --git a/src/nix-instantiate/nix-instantiate.cc b/src/nix-instantiate/nix-instantiate.cc index d86c9fc84..bca65d072 100644 --- a/src/nix-instantiate/nix-instantiate.cc +++ b/src/nix-instantiate/nix-instantiate.cc @@ -64,9 +64,11 @@ void processExpr(EvalState & state, const Strings & attrPaths, Path drvPath = i->queryDrvPath(state); if (gcRoot == "") printGCWarning(); - else - drvPath = addPermRoot(*store, drvPath, - makeRootName(gcRoot, rootNr), indirectRoot); + else { + Path rootName = gcRoot; + if (++rootNr > 1) rootName += "-" + int2String(rootNr); + drvPath = addPermRoot(*store, drvPath, rootName, indirectRoot); + } std::cout << format("%1%\n") % drvPath; } } diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc index c182dbe49..d7b998fae 100644 --- a/src/nix-store/nix-store.cc +++ b/src/nix-store/nix-store.cc @@ -64,15 +64,19 @@ static PathSet realisePath(const Path & path) if (isDerivation(path)) { store->buildPaths(singleton(path)); Derivation drv = derivationFromPath(*store, path); + rootNr++; PathSet outputs; foreach (DerivationOutputs::iterator, i, drv.outputs) { Path outPath = i->second.path; if (gcRoot == "") printGCWarning(); - else - outPath = addPermRoot(*store, outPath, - makeRootName(gcRoot, rootNr), indirectRoot); + else { + Path rootName = gcRoot; + if (rootNr > 1) rootName += "-" + int2String(rootNr); + if (i->first != "out") rootName += "-" + i->first; + outPath = addPermRoot(*store, outPath, rootName, indirectRoot); + } outputs.insert(outPath); } return outputs;