Limit foreground task draining loop in NodePlatform

[ulan]

See https://groups.google.com/forum/#!topic/v8-users/tunDe2yVUoQ for the context.

Currently the NodePlatform drains the foreground_tasks_ queue until the queue becomes empty:
while (std::unique_ptr task = foreground_tasks_.Pop()) {
did_work = true;
RunForegroundTask(std::move(task));
}

This does not work well for tasks that re-post themselves. An example of such tasks is incremental marking task that does 1ms marking work and re-posts itself.

With the current loop draining implementation, incremental marking effectively becomes non-incremental because the loop stops only when incremental marking finishes.

I would like to propose to limit the draining loop to the number of tasks that were in the queue at the start of the loop. The re-posted tasks would be processed via libuv, which would give other tasks (like JS code) chance to run.

[bnoordhuis]

Sounds reasonable to me. Pop() should probably be replaced with a method that does task_queue_.swap(...).

@addaleax Good idea / bad idea?

[ulan]

@bnoordhuis, @addaleax, another possible fix assuming that the queue is FIFO:

size_t limit = foreground_tasks.Size();
for (size_t i = 0; i < limit && (task = foreground_tasks_.Pop()); ++i) {
    did_work = true;
    RunForegroundTask(std::move(task));  
}

The same for the delayed tasks.

We probably need an option for FlushForegroundTasksInternal to drain all the tasks, because it is also used the destructor of PerIsolatePlatformData.

[ryzokuken]

@bnoordhuis @addaleax I'm terrible at C++ work, and haven't submitted a single C++ PR yet, but I'd love to get more into that side of things in the codebase.

If any of you would be willing to provide a little guidance regarding this one, I think I could take it up.

[bnoordhuis]

@ryzokuken Sure thing. If you have specific questions, I'll try to answer them.

[ryzokuken]

@bnoordhuis Great, thanks. I'll be posting things in here as they come up, then.

[ulan]

@ryzokuken @bnoordhuis, oh, I was planning to work on this since I already did debugging. I should have mentioned that before, sorry. I was waiting for @addaleax's reply and the decision on whether to go with swap or limit approach before uploading PR.

@ryzokuken, did you already start working on this?

[ryzokuken]

@ulan I haven't done much, if you've started working on this, please go ahead.

[addaleax]

@ulan I would go with the swapping solution like Ben suggested, but mostly because that seems simpler and doesn’t require setting some slightly arbitrary value as a limit. If you think that V8 has different requirements, feel free to ignore me. :)

[ulan]

@addaleax, thanks! I implemented the swap approach.