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

runtime: scheduler testing controls #54475

Open
prattmic opened this issue Aug 16, 2022 · 1 comment
Open

runtime: scheduler testing controls #54475

prattmic opened this issue Aug 16, 2022 · 1 comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@prattmic
Copy link
Member

Today, it is difficult to trigger specific conditions in the scheduler from a high level Go level. This is because certain behaviors or bugs only manifest via certain multi-threaded race conditions that are difficult or impossible to guarantee. As a result, fixes for bugs in the scheduler often either have strange probabilistic tests that try to entice the scheduler into a specific state (and likely degrade quickly), or they go untested entirely because they are too difficult to reproduce.

Scheduler maintainability could be greatly improved through some kind of testing framework that makes it possible to directly achieve some of these states. There are many forms this could take. e.g., the ability to construct a test scheduler with fake Ms, Ps, and Gs whose states can be explicitly transitioned.

@prattmic prattmic added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 16, 2022
@prattmic prattmic added this to the Backlog milestone Aug 16, 2022
@gopherbot
Copy link

Change https://go.dev/cl/424254 mentions this issue: runtime: drop function context from traceback

@joedian joedian added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 18, 2022
gopherbot pushed a commit that referenced this issue Sep 2, 2022
Currently, gentraceback tracks the closure context of the outermost
frame. This used to be important for "unstarted" calls to reflect
function stubs, where "unstarted" calls are either deferred functions
or the entry-point of a goroutine that hasn't run. Because reflect
function stubs have a dynamic argument map, we have to reach into
their closure context to fetch to map, and how to do this differs
depending on whether the function has started. This was discovered in
issue #25897.

However, as part of the register ABI, "go" and "defer" were made much
simpler, and any "go" or "defer" of a function that takes arguments or
returns results gets wrapped in a closure that provides those
arguments (and/or discards the results). Hence, we'll see that closure
instead of a direct call to a reflect stub, and can get its static
argument map without any trouble.

The one case where we may still see an unstarted reflect stub is if
the function takes no arguments and has no results, in which case the
compiler can optimize away the wrapper closure. But in this case we
know the argument map is empty: the compiler can apply this
optimization precisely because the target function has no argument
frame.

As a result, we no longer need to track the closure context during
traceback, so this CL drops all of that mechanism.

We still have to be careful about the unstarted case because we can't
reach into the function's locals frame to pull out its context
(because it has no locals frame). We double-check that in this case
we're at the function entry.

I would prefer to do this with some in-code PCDATA annotations of
where to find the dynamic argument map, but that's a lot of mechanism
to introduce for just this. It might make sense to consider this along
with #53609.

Finally, we beef up the test for this so it more reliably forces the
runtime down this path. It's fundamentally probabilistic, but this
tweak makes it better. Scheduler testing hooks (#54475) would make it
possible to write a reliable test for this.

For #54466, but it's a nice clean-up all on its own.

Change-Id: I16e4f2364ba2ea4b1fec1e27f971b06756e7b09f
Reviewed-on: https://go-review.googlesource.com/c/go/+/424254
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Auto-Submit: Austin Clements <austin@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
Development

No branches or pull requests

3 participants