Running runloop inside another runloop

[Anton Bikineev]

In Blink platform tests we have the function blink::test::RunPendingTasks(), which executes base::RunLoop().Run(). We've been explicitly prohibiting GCs before the runloop, because for the top-level runloop the garbage collector assumes that there is no stack and doesn't scan the stack for pointers, but in the tests we don't want to miss pointers that are allocated in frames above the runloop. The problem is that this way of disabling GC doesn't work anymore, since we now have incremental marking running in tasks, which relies on GC being not forbidden.

What we essentially want is to run GCs in platform tests but with the stack being visited. For that, we would need to force GCs to be executed as nested tasks. The question is how to force the runloop/tasks to be nested? In pseudoapi, we would probably want something like as follows

base::RunLoop top_level; base::RunLoop nested; top_level.RegisterTask([&nested] { nested.Run(); }); top_level.Run();

Is there an API (or better, more appropriate way) for achieving this?

[Kentaro Hara]

I'm just rephrasing what Anton mentioned, but to put it differently, we want to get the following behavior:

class GCTaskObserver final : public Thread::TaskObserver { // gc_task_runner.h

  GCTaskObserver() : nesting_(0) {}

  void WillProcessTask(const base::PendingTask&, bool) override {

    // If we're processing a task invoked by blink::test::RunPendingTasks(), nesting_ == 1 at this point.

    nesting_++;  // And nesting_ becomes 2.

  }

};

I think this is a reasonable expectation because blink::test::RunPendingTasks() is called inside the main run loop. How can we make that happen? :)

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABH6udaY82kLfuiFwJ9EEZys%3DWFftcDAqrzeTxdn9TON46%2BB0A%40mail.gmail.com.

--

Kentaro Hara, Tokyo, Japan

[Alexander Timin]

Hm, I'm not sure that I fully follow this, but here is my understanding:

Tests (unlike real code) do not have a top-level runloop — by default hey execute all of the code synchronously inside TestBody.

If a test wants to do some async work, they have to call either RunLoop::Run() or blink::RunPendingTasks. Note that this will be the "top-level" runloop and not a nested one (as there is nothing above it).

(In order to get a nested runloop, you need to do something like that:

base::RunLoop run_loop;

run_loop->PostTask([]() { RunPendingTasks(); });

run_loop.Run();

It's not impossible, but it's rather uncommon).

However, the assumption that there is nothing on the stack doesn't hold true for this new "top-level" runloop.
I wonder if instead of prohibiting GC we should just tweak the mode — either by adding a scope changing NoHeapPointerOnStack to HeapPointerOnStack in RunPendingTasks (bad, because it misses RunLoop::Run)

or by changing the GC behaviour to assume that there are pointers on the stack by default and letting the scheduler inform the GC when it is sure that there are no pointers on the stack (== when we are inside renderer_main's RunLoop). 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jw-m%3D3rCmYbFrkksASOJQPuLZeMB8q5z9yAyz5JEH0f7A%40mail.gmail.com.

[Kentaro Hara]

Tests (unlike real code) do not have a top-level runloop — by default hey execute all of the code synchronously inside TestBody.

Stupid question: Is it challenging to add a top-level runloop to tests by default?

You received this message because you are subscribed to the Google Groups "scheduler-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scheduler-de...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/scheduler-dev/CALHg4nnwZ8dUE3vttD0ivvmaQSp2k8oY9QPJkoEHUwj_Ue_uGg%40mail.gmail.com.

[Alexander Timin]

On Thu, 13 Feb 2020 at 10:51, Kentaro Hara <har...@chromium.org> wrote:

Tests (unlike real code) do not have a top-level runloop — by default hey execute all of the code synchronously inside TestBody.

Stupid question: Is it challenging to add a top-level runloop to tests by default?

The underlying problem is not just adding a top-level runloop but the fact that code in the test expects to run synchronously and the test finishes when the end of code block finishes.

Making all (or most) of the tests async sounds like a giant undertaking and without it top-level runloop wouldn't be useful.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jytNh_uTRZJzxS9gvnG1EhiNG7N%3DQV45Lvyserf6w63-Q%40mail.gmail.com.

[Kentaro Hara]

The underlying problem is not just adding a top-level runloop but the fact that code in the test expects to run synchronously and the test finishes when the end of code block finishes.
Making all (or most) of the tests async sounds like a giant undertaking and without it top-level runloop wouldn't be useful.

I'm wondering if it's possible to insert a top-level runloop to a testing driver, not each individual test.

base::RunLoop run_loop;

... // Run multiple tests

[Joe Mason]

Can you start the loop in setUpTestSuite for a fixture?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/scheduler-dev/CABg10jyAME0Qh2M8MJYmZNffsFo38UP9yU9p5pbGfZSU%2B_9rZQ%40mail.gmail.com.

[Alexander Timin]

Yeah, it is possible, but I think that it's not something that we want. Mostly because 

a) this runloop is not going to be useful as the tasks won't have a chance to run before the test finishes anyway.

b) it will turn all other runloops used by tests into nested ones, changing the behaviour (as we disable a few features for nested runloops — e.g. not running non-nestable tasks) in the ways we don't want.

c) it will make it easier to leak information across tests (by posting a task).

I think that it's much easier for the test suite to let the GC logic know that it should scan the stack for pointers.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAH%3DT95QtYndxLmAmBDS4jbk_DDjRUuoj39BNyV6GRxm_3NxvcQ%40mail.gmail.com.

[Kentaro Hara]

Yeah, it is possible, but I think that it's not something that we want.

^^^ I think this is the key question :)

Is the expectation that tests are NOT running inside a runloop?

If the answer is yes, I agree with Alexander. If the answer is no, we should think about ways to add a top-level runloop to tests.

Any other opinions?

[Michael Lippautz]

On Fri, Feb 14, 2020 at 12:08 AM Alexander Timin <alt...@chromium.org> wrote:

Yeah, it is possible, but I think that it's not something that we want. Mostly because 

a) this runloop is not going to be useful as the tasks won't have a chance to run before the test finishes anyway.

b) it will turn all other runloops used by tests into nested ones, changing the behaviour (as we disable a few features for nested runloops — e.g. not running non-nestable tasks) in the ways we don't want.

So that means we currently execute non-nestable tasks in those testing calls? If yes, that seems like another problem. If JS is run in tests, V8 may post non-nestable tasks to finalize garbage collections to avoid scanning the stack. For unified heap this means that V8 would tell Oilpan that it's safe to ignore the stack when finalizing GC through such a task.

We could solve it by disabling non-nestable tasks through NonNestableTasksEnabled() for tests.

 

c) it will make it easier to leak information across tests (by posting a task).

I think that it's much easier for the test suite to let the GC logic know that it should scan the stack for pointers.

That's what we thought as well. The GC really doesn't care how things are executed (sync vs async) as these are properties of how we would like our tests to execute and there seems to be good reasons for both. The GC only cares that it gets the pointer states right.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CALHg4nnp8NKmy1BNoqOJqwS-0RQTwtbSmzXh6Qu34Pjuyr70Bw%40mail.gmail.com.

[Anton Bikineev]

> I wonder if instead of prohibiting GC we should just tweak the mode — either by adding a scope changing NoHeapPointerOnStack to HeapPointerOnStack in RunPendingTasks

This is what we implemented initially, but due to the bad hook-style design didn't want to have this workaround on the GC side.

[Alexander Timin]

On Fri, 14 Feb 2020 at 08:44, Michael Lippautz <mlip...@chromium.org> wrote:

On Fri, Feb 14, 2020 at 12:08 AM Alexander Timin <alt...@chromium.org> wrote:

Yeah, it is possible, but I think that it's not something that we want. Mostly because 

a) this runloop is not going to be useful as the tasks won't have a chance to run before the test finishes anyway.

b) it will turn all other runloops used by tests into nested ones, changing the behaviour (as we disable a few features for nested runloops — e.g. not running non-nestable tasks) in the ways we don't want.

So that means we currently execute non-nestable tasks in those testing calls? If yes, that seems like another problem. If JS is run in tests, V8 may post non-nestable tasks to finalize garbage collections to avoid scanning the stack. For unified heap this means that V8 would tell Oilpan that it's safe to ignore the stack when finalizing GC through such a task.

We could solve it by disabling non-nestable tasks through NonNestableTasksEnabled() for tests.

Yes, base::RunLoop / blink::test::RunPendingTasks will run non-nestable tasks (the GC is explicitly prohibited in RunPendingTasks, however) and I believe that this is what we want in a typical use-case:

TEST_F(Foo, Test) {

  DoStuff();

  base::RunLoop().RunUntilIdle();

  DoMoreStuff();

  base::RunLoop().RunUntilIdle();
}

(the runloops are normally called directly from the test to emulate top-level runloop while keeping the test code sync).

  

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAH%2BmL5Av12JVzKx8Dm_JssDBtn7UdqB7gTocP_AOJ-mVzrp%3D3g%40mail.gmail.com.

[Alexander Timin]

Could you point me to the place where we choose between HeapPointers and NoHeapPointers? I know about one check in GCTaskRunner, but I wonder if there is anything else?

Tweaking this logic there seems like a good idea.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABH6udb%2BO%2Bt1HYbreScnEYGEdsx72%2BtbxCVNVrJGWx0C6SwG2w%40mail.gmail.com.

[Michael Lippautz]

Blink's no-GC scope can only work for Blink-local operations. It does not work when invoking JS operations or draining the task queue as with unified heap V8 is allowed to finalize a collection cycle as well.

V8 has per-design never allowed a no-GC scope on its API level because there should be no way for the embedder to create a state where we cannot finalize. Same holds true for unified heap.

(Especially, when invoking some non-trivial workload like RunLoop() that can execute anything we should allow GC to happen to catch as many issues as we can in tests.)

On Fri, Feb 14, 2020 at 11:09 AM Alexander Timin <alt...@chromium.org> wrote:

Could you point me to the place where we choose between HeapPointers and NoHeapPointers? I know about one check in GCTaskRunner, but I wonder if there is anything else?

Tweaking this logic there seems like a good idea.

I think Anton's initial question is how do we nicely allow GC and adjust GCTaskRunner to signal that we have pointers on the stack in the current RunLoop architecture.

The V8 parts are here:

https://cs.chromium.org/chromium/src/v8/src/heap/incremental-marking-job.cc?sq=package:chromium&g=0&l=55

 

V8 does check whether the embedder supports nested tasks which can be overridden in such scenarios.

[Kentaro Hara]

Could you point me to the place where we choose between HeapPointers and NoHeapPointers? I know about one check in GCTaskRunner, but I wonder if there is anything else?
Tweaking this logic there seems like a good idea.

For this approach, Anton already has a workable solution. I thought that that is a bad hook-style design working around a real problem around tests & runloops and was wondering if the testing infrastructure should support a top-level runloop.

However, after seeing the discussion on this thread, I'm convinced that the expectation is that tasks executed by RunPendingTasks() are run as if they are run in a top-level runloop. If that is the case, Anton's solution & Alexander's proposal sound reasonable. We should just make RunPendingTasks() tell Oilpan that the tasks executed there will have on-stack pointers.

(Anton: I apologize my concern led to a bunch of exploration and we finally got back to your original solution.)

[Alexander Timin]

On Fri, 14 Feb 2020 at 10:30, Kentaro Hara <har...@chromium.org> wrote:

Could you point me to the place where we choose between HeapPointers and NoHeapPointers? I know about one check in GCTaskRunner, but I wonder if there is anything else?
Tweaking this logic there seems like a good idea.

For this approach, Anton already has a workable solution. I thought that that is a bad hook-style design working around a real problem around tests & runloops and was wondering if the testing infrastructure should support a top-level runloop.

However, after seeing the discussion on this thread, I'm convinced that the expectation is that tasks executed by RunPendingTasks() are run as if they are run in a top-level runloop. If that is the case, Anton's solution & Alexander's proposal sound reasonable. We should just make RunPendingTasks() tell Oilpan that the tasks executed there will have on-stack pointers.

(Anton: I apologize my concern led to a bunch of exploration and we finally got back to your original solution.)

I would still like it if we could support RunLoop::Run() here as well.

I'd suggest the following:

- Add a method to MainThreadScheduler (HasMainRunLoop, not sure about the best name), returning false by default.

- Make it return true only when MainThreadScheduler was created from renderer_main.cc (so it'll always be false in the tests). 

- Check for MainThreadScheduler::HasMainRunLoop && !nesting in GCTaskRunner instead of just !nesting.

(There are way too many ways to enable RunLoop in the tests, I don't think that it's possible to account for all of them — opting in "no pointers" mode seems more robust than opting out from it)

On Fri, Feb 14, 2020 at 2:21 AM Michael Lippautz <mlip...@chromium.org> wrote:

Blink's no-GC scope can only work for Blink-local operations. It does not work when invoking JS operations or draining the task queue as with unified heap V8 is allowed to finalize a collection cycle as well.

V8 has per-design never allowed a no-GC scope on its API level because there should be no way for the embedder to create a state where we cannot finalize. Same holds true for unified heap.

(Especially, when invoking some non-trivial workload like RunLoop() that can execute anything we should allow GC to happen to catch as many issues as we can in tests.)

On Fri, Feb 14, 2020 at 11:09 AM Alexander Timin <alt...@chromium.org> wrote:

Could you point me to the place where we choose between HeapPointers and NoHeapPointers? I know about one check in GCTaskRunner, but I wonder if there is anything else?

Tweaking this logic there seems like a good idea.

I think Anton's initial question is how do we nicely allow GC and adjust GCTaskRunner to signal that we have pointers on the stack in the current RunLoop architecture.

The V8 parts are here:

https://cs.chromium.org/chromium/src/v8/src/heap/incremental-marking-job.cc?sq=package:chromium&g=0&l=55

 

V8 does check whether the embedder supports nested tasks which can be overridden in such scenarios.

Yeah, I think that we want to export a more fine-grained information there as well, similar to my suggestion above.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jxOfXBVj%2BxkqXWyU5bgGkX0RZoXYmuoywSksW2i%3DA3nGw%40mail.gmail.com.