From 0194f815870ed875b791d60335be920de249a510 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 22 Jul 2024 15:10:47 +0200 Subject: [PATCH 1/4] addToStore(): Do evaluation on the main stack This hopefully avoids the need for all our Boehm GC coroutine workarounds, since the GC roots will be on the main stack of the thread. Fixes #11141. --- src/libstore/local-store.cc | 1 + src/libstore/store-api.cc | 16 +++++++++------- src/libutil/serialise.cc | 8 ++++---- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 819cee345..956a4453d 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1245,6 +1245,7 @@ StorePath LocalStore::addToStoreFromDump( }); try { got = source.read(dumpBuffer.get() + oldSize, want); + if (!got) break; } catch (EndOfFile &) { inMemory = true; break; diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc index 2c4dee518..5d6109bae 100644 --- a/src/libstore/store-api.cc +++ b/src/libstore/store-api.cc @@ -210,14 +210,16 @@ StorePath Store::addToStore( fsm = FileSerialisationMethod::NixArchive; break; } - auto source = sinkToSource([&](Sink & sink) { - dumpPath(path, sink, fsm, filter); + std::optional storePath; + auto sink = sourceToSink([&](Source & source) { + LengthSource lengthSource(source); + storePath = addToStoreFromDump(lengthSource, name, fsm, method, hashAlgo, references, repair); + if (lengthSource.total >= settings.warnLargePathThreshold) + warn("copied large path '%s' to the store (%s)", path, renderSize(lengthSource.total)); }); - LengthSource lengthSource(*source); - auto storePath = addToStoreFromDump(lengthSource, name, fsm, method, hashAlgo, references, repair); - if (lengthSource.total >= settings.warnLargePathThreshold) - warn("copied large path '%s' to the store (%s)", path, renderSize(lengthSource.total)); - return storePath; + dumpPath(path, *sink, fsm, filter); + sink->finish(); + return storePath.value(); } void Store::addMultipleToStore( diff --git a/src/libutil/serialise.cc b/src/libutil/serialise.cc index 36b99905a..0f2febaed 100644 --- a/src/libutil/serialise.cc +++ b/src/libutil/serialise.cc @@ -243,11 +243,11 @@ std::unique_ptr sourceToSink(std::function fun) if (!coro) { CoroutineContext ctx; coro = coro_t::push_type(VirtualStackAllocator{}, [&](coro_t::pull_type & yield) { - LambdaSource source([&](char *out, size_t out_len) { + LambdaSource source([&](char * out, size_t out_len) { if (cur.empty()) { yield(); if (yield.get()) { - return (size_t)0; + return (size_t) 0; } } @@ -271,12 +271,12 @@ std::unique_ptr sourceToSink(std::function fun) void finish() override { if (!coro) return; - if (!*coro) abort(); + //if (!*coro) abort(); { CoroutineContext ctx; (*coro)(true); } - if (*coro) abort(); + //if (*coro) abort(); } }; From 609df83c011ad74c20cd806e00d59f6ed4a580b5 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 22 Jul 2024 15:18:58 +0200 Subject: [PATCH 2/4] sourceToSink(): Throw EndOfFile --- src/libstore/local-store.cc | 1 - src/libutil/serialise.cc | 9 ++++----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index 956a4453d..819cee345 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1245,7 +1245,6 @@ StorePath LocalStore::addToStoreFromDump( }); try { got = source.read(dumpBuffer.get() + oldSize, want); - if (!got) break; } catch (EndOfFile &) { inMemory = true; break; diff --git a/src/libutil/serialise.cc b/src/libutil/serialise.cc index 0f2febaed..d203a01bf 100644 --- a/src/libutil/serialise.cc +++ b/src/libutil/serialise.cc @@ -246,9 +246,8 @@ std::unique_ptr sourceToSink(std::function fun) LambdaSource source([&](char * out, size_t out_len) { if (cur.empty()) { yield(); - if (yield.get()) { - return (size_t) 0; - } + if (yield.get()) + throw EndOfFile("coroutine has finished"); } size_t n = std::min(cur.size(), out_len); @@ -271,12 +270,12 @@ std::unique_ptr sourceToSink(std::function fun) void finish() override { if (!coro) return; - //if (!*coro) abort(); + if (!*coro) abort(); { CoroutineContext ctx; (*coro)(true); } - //if (*coro) abort(); + if (*coro) abort(); } }; From ca0f7db843d2c58bb5ca7b45f2b2aa57273e5f7e Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 22 Jul 2024 15:28:21 +0200 Subject: [PATCH 3/4] Remove hacks to support evaluation from coroutines Since we're not doing this anymore. --- .../src/installation/prerequisites-source.md | 2 - src/libexpr/eval-gc.cc | 132 ------------------ src/libutil/serialise.cc | 58 +------- src/libutil/serialise.hh | 23 --- 4 files changed, 2 insertions(+), 213 deletions(-) diff --git a/doc/manual/src/installation/prerequisites-source.md b/doc/manual/src/installation/prerequisites-source.md index 4aafa6d27..c346a0a4b 100644 --- a/doc/manual/src/installation/prerequisites-source.md +++ b/doc/manual/src/installation/prerequisites-source.md @@ -39,8 +39,6 @@ `pkgconfig` and the Boehm garbage collector, and pass the flag `--enable-gc` to `configure`. - For `bdw-gc` <= 8.2.4 Nix needs a [small patch](https://github.com/NixOS/nix/blob/ac4d2e7b857acdfeac35ac8a592bdecee2d29838/boehmgc-traceable_allocator-public.diff) to be applied. - - The `boost` library of version 1.66.0 or higher. It can be obtained from the official web site . diff --git a/src/libexpr/eval-gc.cc b/src/libexpr/eval-gc.cc index ba19cd74e..4bf48194e 100644 --- a/src/libexpr/eval-gc.cc +++ b/src/libexpr/eval-gc.cc @@ -30,120 +30,6 @@ static void * oomHandler(size_t requested) throw std::bad_alloc(); } -class BoehmGCStackAllocator : public StackAllocator -{ - boost::coroutines2::protected_fixedsize_stack stack{ - // We allocate 8 MB, the default max stack size on NixOS. - // A smaller stack might be quicker to allocate but reduces the stack - // depth available for source filter expressions etc. - std::max(boost::context::stack_traits::default_size(), static_cast(8 * 1024 * 1024))}; - - // This is specific to boost::coroutines2::protected_fixedsize_stack. - // The stack protection page is included in sctx.size, so we have to - // subtract one page size from the stack size. - std::size_t pfss_usable_stack_size(boost::context::stack_context & sctx) - { - return sctx.size - boost::context::stack_traits::page_size(); - } - -public: - boost::context::stack_context allocate() override - { - auto sctx = stack.allocate(); - - // Stacks generally start at a high address and grow to lower addresses. - // Architectures that do the opposite are rare; in fact so rare that - // boost_routine does not implement it. - // So we subtract the stack size. - GC_add_roots(static_cast(sctx.sp) - pfss_usable_stack_size(sctx), sctx.sp); - return sctx; - } - - void deallocate(boost::context::stack_context sctx) override - { - GC_remove_roots(static_cast(sctx.sp) - pfss_usable_stack_size(sctx), sctx.sp); - stack.deallocate(sctx); - } -}; - -static BoehmGCStackAllocator boehmGCStackAllocator; - -/** - * When a thread goes into a coroutine, we lose its original sp until - * control flow returns to the thread. - * While in the coroutine, the sp points outside the thread stack, - * so we can detect this and push the entire thread stack instead, - * as an approximation. - * The coroutine's stack is covered by `BoehmGCStackAllocator`. - * This is not an optimal solution, because the garbage is scanned when a - * coroutine is active, for both the coroutine and the original thread stack. - * However, the implementation is quite lean, and usually we don't have active - * coroutines during evaluation, so this is acceptable. - */ -void fixupBoehmStackPointer(void ** sp_ptr, void * _pthread_id) -{ - void *& sp = *sp_ptr; - auto pthread_id = reinterpret_cast(_pthread_id); - pthread_attr_t pattr; - size_t osStackSize; - // The low address of the stack, which grows down. - void * osStackLimit; - void * osStackBase; - -# ifdef __APPLE__ - osStackSize = pthread_get_stacksize_np(pthread_id); - osStackLimit = pthread_get_stackaddr_np(pthread_id); -# else - if (pthread_attr_init(&pattr)) { - throw Error("fixupBoehmStackPointer: pthread_attr_init failed"); - } -# ifdef HAVE_PTHREAD_GETATTR_NP - if (pthread_getattr_np(pthread_id, &pattr)) { - throw Error("fixupBoehmStackPointer: pthread_getattr_np failed"); - } -# elif HAVE_PTHREAD_ATTR_GET_NP - if (!pthread_attr_init(&pattr)) { - throw Error("fixupBoehmStackPointer: pthread_attr_init failed"); - } - if (!pthread_attr_get_np(pthread_id, &pattr)) { - throw Error("fixupBoehmStackPointer: pthread_attr_get_np failed"); - } -# else -# error "Need one of `pthread_attr_get_np` or `pthread_getattr_np`" -# endif - if (pthread_attr_getstack(&pattr, &osStackLimit, &osStackSize)) { - throw Error("fixupBoehmStackPointer: pthread_attr_getstack failed"); - } - if (pthread_attr_destroy(&pattr)) { - throw Error("fixupBoehmStackPointer: pthread_attr_destroy failed"); - } -# endif - osStackBase = (char *) osStackLimit + osStackSize; - // NOTE: We assume the stack grows down, as it does on all architectures we support. - // Architectures that grow the stack up are rare. - if (sp >= osStackBase || sp < osStackLimit) { // sp is outside the os stack - sp = osStackLimit; - } -} - -/* Disable GC while this object lives. Used by CoroutineContext. - * - * Boehm keeps a count of GC_disable() and GC_enable() calls, - * and only enables GC when the count matches. - */ -class BoehmDisableGC -{ -public: - BoehmDisableGC() - { - GC_disable(); - }; - ~BoehmDisableGC() - { - GC_enable(); - }; -}; - static inline void initGCReal() { /* Initialise the Boehm garbage collector. */ @@ -164,24 +50,6 @@ static inline void initGCReal() GC_set_oom_fn(oomHandler); - StackAllocator::defaultAllocator = &boehmGCStackAllocator; - -// TODO: Remove __APPLE__ condition. -// Comment suggests an implementation that works on darwin and windows -// https://github.com/ivmai/bdwgc/issues/362#issuecomment-1936672196 -# if GC_VERSION_MAJOR >= 8 && GC_VERSION_MINOR >= 2 && GC_VERSION_MICRO >= 4 && !defined(__APPLE__) - GC_set_sp_corrector(&fixupBoehmStackPointer); - - if (!GC_get_sp_corrector()) { - printTalkative("BoehmGC on this platform does not support sp_corrector; will disable GC inside coroutines"); - /* Used to disable GC when entering coroutines on macOS */ - create_coro_gc_hook = []() -> std::shared_ptr { return std::make_shared(); }; - } -# else -# warning \ - "BoehmGC version does not support GC while coroutine exists. GC will be disabled inside coroutines. Consider updating bdw-gc to 8.2.4 or later." -# endif - /* Set the initial heap size to something fairly big (25% of physical RAM, up to a maximum of 384 MiB) so that in most cases we don't need to garbage collect at all. (Collection has a diff --git a/src/libutil/serialise.cc b/src/libutil/serialise.cc index d203a01bf..cd52fbfb4 100644 --- a/src/libutil/serialise.cc +++ b/src/libutil/serialise.cc @@ -171,55 +171,6 @@ size_t StringSource::read(char * data, size_t len) #error Coroutines are broken in this version of Boost! #endif -/* A concrete datatype allow virtual dispatch of stack allocation methods. */ -struct VirtualStackAllocator { - StackAllocator *allocator = StackAllocator::defaultAllocator; - - boost::context::stack_context allocate() { - return allocator->allocate(); - } - - void deallocate(boost::context::stack_context sctx) { - allocator->deallocate(sctx); - } -}; - - -/* This class reifies the default boost coroutine stack allocation strategy with - a virtual interface. */ -class DefaultStackAllocator : public StackAllocator { - boost::coroutines2::default_stack stack; - - boost::context::stack_context allocate() { - return stack.allocate(); - } - - void deallocate(boost::context::stack_context sctx) { - stack.deallocate(sctx); - } -}; - -static DefaultStackAllocator defaultAllocatorSingleton; - -StackAllocator *StackAllocator::defaultAllocator = &defaultAllocatorSingleton; - - -std::shared_ptr (*create_coro_gc_hook)() = []() -> std::shared_ptr { - return {}; -}; - -/* This class is used for entry and exit hooks on coroutines */ -class CoroutineContext { - /* Disable GC when entering the coroutine without the boehm patch, - * since it doesn't find the main thread stack in this case. - * std::shared_ptr performs type-erasure, so it will call the right - * deleter. */ - const std::shared_ptr coro_gc_hook = create_coro_gc_hook(); -public: - CoroutineContext() {}; - ~CoroutineContext() {}; -}; - std::unique_ptr sourceToSink(std::function fun) { struct SourceToSink : FinishSink @@ -241,8 +192,7 @@ std::unique_ptr sourceToSink(std::function fun) cur = in; if (!coro) { - CoroutineContext ctx; - coro = coro_t::push_type(VirtualStackAllocator{}, [&](coro_t::pull_type & yield) { + coro = coro_t::push_type([&](coro_t::pull_type & yield) { LambdaSource source([&](char * out, size_t out_len) { if (cur.empty()) { yield(); @@ -262,7 +212,6 @@ std::unique_ptr sourceToSink(std::function fun) if (!*coro) { abort(); } if (!cur.empty()) { - CoroutineContext ctx; (*coro)(false); } } @@ -272,7 +221,6 @@ std::unique_ptr sourceToSink(std::function fun) if (!coro) return; if (!*coro) abort(); { - CoroutineContext ctx; (*coro)(true); } if (*coro) abort(); @@ -306,8 +254,7 @@ std::unique_ptr sinkToSource( size_t read(char * data, size_t len) override { if (!coro) { - CoroutineContext ctx; - coro = coro_t::pull_type(VirtualStackAllocator{}, [&](coro_t::push_type & yield) { + coro = coro_t::pull_type([&](coro_t::push_type & yield) { LambdaSink sink([&](std::string_view data) { if (!data.empty()) yield(std::string(data)); }); @@ -319,7 +266,6 @@ std::unique_ptr sinkToSource( if (pos == cur.size()) { if (!cur.empty()) { - CoroutineContext ctx; (*coro)(); } cur = coro->get(); diff --git a/src/libutil/serialise.hh b/src/libutil/serialise.hh index 8137db5f4..a795bb088 100644 --- a/src/libutil/serialise.hh +++ b/src/libutil/serialise.hh @@ -557,27 +557,4 @@ struct FramedSink : nix::BufferedSink }; }; -/** - * Stack allocation strategy for sinkToSource. - * Mutable to avoid a boehm gc dependency in libutil. - * - * boost::context doesn't provide a virtual class, so we define our own. - */ -struct StackAllocator { - virtual boost::context::stack_context allocate() = 0; - virtual void deallocate(boost::context::stack_context sctx) = 0; - - /** - * The stack allocator to use in sinkToSource and potentially elsewhere. - * It is reassigned by the initGC() method in libexpr. - */ - static StackAllocator *defaultAllocator; -}; - -/* Disabling GC when entering a coroutine (without the boehm patch). - mutable to avoid boehm gc dependency in libutil. - */ -extern std::shared_ptr (*create_coro_gc_hook)(); - - } From b8684eb44723ad023e74a50c446f77ef56031f18 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Mon, 22 Jul 2024 20:03:29 +0200 Subject: [PATCH 4/4] sourceToSink(): Fix abort --- src/libutil/serialise.cc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/libutil/serialise.cc b/src/libutil/serialise.cc index cd52fbfb4..8fbfe248c 100644 --- a/src/libutil/serialise.cc +++ b/src/libutil/serialise.cc @@ -218,12 +218,8 @@ std::unique_ptr sourceToSink(std::function fun) void finish() override { - if (!coro) return; - if (!*coro) abort(); - { + if (coro && *coro) (*coro)(true); - } - if (*coro) abort(); } };