Disable GC during coroutine execution + test #7725

yorickvP · 2023-01-31T16:59:39Z

Motivation

Regression test for #7679

Context

Without @roberth's boehmgc-coroutine-sp-fallback patch, nix mostly works but sometimes segfaults. This PR provides a test that should catch this bug.

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

agreed on idea
agreed on implementation strategy
tests, as appropriate
- functional tests - tests/**.sh
- unit tests - src/*/tests
- integration tests - tests/nixos/*
documentation in the manual
code and comments are self-explanatory
commit message explains why the change was made
new feature or bug fix: updated release notes

yorickvP · 2023-01-31T18:04:19Z

I'll investigate macos tomorrow.

cole-h · 2023-01-31T18:32:50Z

src/libexpr/tests/coro-gc.cc

+#include "serialise.hh"
+
+
+#define guard_gc(x) GC_register_finalizer((void*)x, finalizer, x##_collected, nullptr, nullptr)


Should this also be guarded with #if HAVE_BOEHMGC?

roberth · 2023-01-31T23:53:29Z

I'll investigate macos tomorrow.

I think the patch for the darwin c file never worked. Its regular stack code is different, so the linux patch doesn't translate to it, and the alt stack that was patched is quite irrelevant; used very shortly during signal handling only.

yorickvP · 2023-02-03T16:53:13Z

I've resorted to disabling GC before entering a coroutine on mac OS. I hope that's an acceptable workaround for now.

thufschmitt

Left a few local suggestions, but looks good overall 👍

Note that this isn't really correct in that you could have interleaved coroutines, in which case the first to finish will restart the GC while the other is still running. But it's good-enough for now (we could certainly do something with a counter or a smart use of a shared_ptr, but let's not over-think it).

thufschmitt · 2023-02-07T09:25:37Z

src/libutil/serialise.hh

+public:
+    DisableGC() {};
+    virtual ~DisableGC() {};
+    static std::shared_ptr<DisableGC> (*create)();


Suggested change

static std::shared_ptr<DisableGC> (*create)();

static std::function<std::shared_ptr<DisableGC>()> create;

Doesn't change too much, but let's make this more idiomatic C++

I actually don't want std::function here. It's quite an expensive wrapper and allows more weird stuff (like functors and std::bind and lambda captures), so using an old function pointer is probably more typesafe for now.

See https://stackoverflow.com/a/9054802

thufschmitt · 2023-02-10T09:16:08Z

src/libutil/serialise.hh

+/* Disabling GC when entering a coroutine (on macos).
+   ::create is to avoid boehm gc dependency in libutil.
+ */
+class DisableGC {


Could we maybe make this a slightly more generic CoroutineContext or something like that? Just so that

this layer doesn't have to bother about the GC

Rather than having all these #if __APPLE__ in serialise.cc we can just unconditionally call CoroutineContext.create() and put this #if inside the constructor/destructor of the BoehmDisableGC class

Good idea, I do want to keep it obvious what it's doing and why, though. I've added the disablegc call inside the coroutine definition now.
I considered having a generic hook system (with std::list and everything) so anywhere in the code can add arbitrary coroutine context hooks, but this seems overkill, slower and less clear.

src/libexpr/tests/coro-gc.cc

yorickvP · 2023-02-10T15:19:47Z

GC_enable/disable actually does the counting itself. Boehm only enables GC when the amount of calls to enable and disable are equal.

roberth · 2023-02-12T03:17:06Z

Before overengineering this workaround, we might consider fixing the problem, which is detecting whether the stack pointer is in the given stack and marking the all the stack sections of the segmented darwin stack if it is not. That'd be the equivalent of the linux solution which works quite well.

That's not to mention that the linux solution isn't the final solution either, but we Nix maintainers don't have the bandwidth to make architectural changes to bdwgc.
If you or anyone finds this kind of thing a nice challenge, I could provide a bit more context to the problem, which is currently spread between a number of issues here and on the ivmai/bdwgc repo.

nixos-discourse · 2023-02-17T13:24:48Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-44/25546/1

Regression test for NixOS#7679

yorickvP · 2023-03-01T14:08:40Z

I've added a #define to the patch and disable GC in a coroutine when this is not active. However, the tests now succeed when the patch is not applied, which was the entire point of this PR.

yorickvP · 2023-03-01T15:44:40Z

I would suggest a ./configure warning if gc.h is missing this define, but also, I don't want to touch autoconf today. Maybe tomorrow.

roberth · 2023-03-02T12:13:19Z

However, the tests now succeed when the patch is not applied, which was the entire point of this PR.

That's fine, the main requirement is that it doesn't crash. I don't think we have very long running coroutines, unless you're filtering a huge source in terms of number of assessed files, so the likelihood seems quite small that we'll request more memory instead of collecting, for more than once or twice in a row.
Maybe we should leave a lvlTalkative log message so that it doesn't go entirely unnoticed to someone who's troubleshooting their eval. If we have some numbers that say that a GC would be desirable while blocked, we could log a warning, but that might be difficult and result in overengineering and/or useless warnings.

roberth · 2023-03-02T13:41:16Z

Maybe we should

... not over-engineer this, because we may be able to implement the coroutine integration properly in the foreseeable future.

Coroutines support ivmai/bdwgc#362

yorickvP · 2023-03-03T10:44:26Z

Alright, added this message.

edolstra · 2023-03-08T12:59:32Z

This is causing some test failures: https://hydra.nixos.org/build/211782650, https://hydra.nixos.org/build/211782591

yorickvP · 2023-03-08T13:00:58Z

Ah, great, musl and i686. Hmm. Please revert and I'll debug that.

nixos-discourse · 2023-03-16T12:43:34Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-45/26397/1

nixos-discourse · 2023-04-21T11:57:24Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nix-team-report-2022-10-2023-03/27486/1

yorickvP requested review from edolstra and thufschmitt as code owners January 31, 2023 16:59

cole-h reviewed Jan 31, 2023

View reviewed changes

yorickvP force-pushed the check-coro-gc branch 2 times, most recently from f32e765 to e8f5588 Compare February 7, 2023 14:34

thufschmitt suggested changes Feb 10, 2023

View reviewed changes

yorickvP requested review from thufschmitt and removed request for edolstra February 10, 2023 15:19

yorickvP mentioned this pull request Feb 24, 2023

doc: Document Boehm GC patch in Prerequisites #7902

Closed

7 tasks

roberth self-assigned this Feb 26, 2023

yorickvP added 5 commits March 1, 2023 13:55

tests/coro-gc: create test for boehm stack patch

0fd8f54

Regression test for NixOS#7679

Disable GC inside coroutines on mac OS

eaeb994

tests/coro-gc: refactor and split into 3 tests

53bb4a5

DisableGC: replace by CoroutineContext, std::shared_ptr<void>

4c73eab

Always disable GC in a coroutine unless the patch is applied

1760057

yorickvP force-pushed the check-coro-gc branch from e4d7064 to 1760057 Compare March 1, 2023 14:07

Add talkative msg for coro gc debug

2683734

roberth changed the title ~~tests/coro-gc: create test for boehm stack patch~~ Disable GC during coroutine execution + test Mar 7, 2023

roberth approved these changes Mar 7, 2023

View reviewed changes

thufschmitt approved these changes Mar 8, 2023

View reviewed changes

thufschmitt merged commit 4a6244d into NixOS:master Mar 8, 2023

thufschmitt mentioned this pull request Mar 8, 2023

Revert "Disable GC during coroutine execution + test" #8012

Merged

thufschmitt pushed a commit to tweag/nix that referenced this pull request Apr 7, 2023

Cherry-pick NixOS#7725

15ce51f

This was referenced Apr 7, 2023

disable gc on coroutine #8179

Merged

Random segfaults on the MacOS CI #8175

Closed

yorickvP mentioned this pull request Apr 11, 2023

Ressurect tests for GC+coroutines #8204

Open

8 tasks

roberth mentioned this pull request May 30, 2023

Evaluating nixos linux .iso on darwin causes segfault #8412

Closed

roberth mentioned this pull request Dec 18, 2023

Deterministic or guaranteed recursion depth (stack size) #9627

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable GC during coroutine execution + test #7725

Disable GC during coroutine execution + test #7725

yorickvP commented Jan 31, 2023

yorickvP commented Jan 31, 2023

cole-h Jan 31, 2023

roberth commented Jan 31, 2023

yorickvP commented Feb 3, 2023

thufschmitt left a comment

thufschmitt Feb 7, 2023

yorickvP Feb 10, 2023

thufschmitt Feb 10, 2023

yorickvP Feb 10, 2023

yorickvP commented Feb 10, 2023

roberth commented Feb 12, 2023

nixos-discourse commented Feb 17, 2023

yorickvP commented Mar 1, 2023

yorickvP commented Mar 1, 2023

roberth commented Mar 2, 2023

roberth commented Mar 2, 2023

yorickvP commented Mar 3, 2023

edolstra commented Mar 8, 2023

yorickvP commented Mar 8, 2023

nixos-discourse commented Mar 16, 2023

nixos-discourse commented Apr 21, 2023

		#include "serialise.hh"


		#define guard_gc(x) GC_register_finalizer((void*)x, finalizer, x##_collected, nullptr, nullptr)

	static std::shared_ptr<DisableGC> (*create)();
	static std::function<std::shared_ptr<DisableGC>()> create;

Disable GC during coroutine execution + test #7725

Disable GC during coroutine execution + test #7725

Conversation

yorickvP commented Jan 31, 2023

Motivation

Context

Checklist for maintainers

yorickvP commented Jan 31, 2023

cole-h Jan 31, 2023

Choose a reason for hiding this comment

roberth commented Jan 31, 2023

yorickvP commented Feb 3, 2023

thufschmitt left a comment

Choose a reason for hiding this comment

thufschmitt Feb 7, 2023

Choose a reason for hiding this comment

yorickvP Feb 10, 2023

Choose a reason for hiding this comment

thufschmitt Feb 10, 2023

Choose a reason for hiding this comment

yorickvP Feb 10, 2023

Choose a reason for hiding this comment

yorickvP commented Feb 10, 2023

roberth commented Feb 12, 2023

nixos-discourse commented Feb 17, 2023

yorickvP commented Mar 1, 2023

yorickvP commented Mar 1, 2023

roberth commented Mar 2, 2023

roberth commented Mar 2, 2023

yorickvP commented Mar 3, 2023

edolstra commented Mar 8, 2023

yorickvP commented Mar 8, 2023

nixos-discourse commented Mar 16, 2023

nixos-discourse commented Apr 21, 2023