Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29370876: Issue #4692, #3595 - Stop sleeping in the implementation of `SetTimeout`

Created:
Jan. 9, 2017, 12:45 p.m. by Eric
Modified:
Jan. 10, 2017, 2:46 p.m.
Reviewers:
sergei, Felix Dahlke
Visibility:
Public.

Description

Issue #4692, #3595 - Stop sleeping in the implementation of `SetTimeout` New source files "Timeout.{cpp,h}" for implementation of timeout. Some code had been in "GlobalJsObject.{cpp,h}". Add class `DelayedExecutor`, which has member function `RunWithDelay()`. This has much the same effect as `sleep_for()` but is interruptible. The essence of the new mechanism is a call to `wait_for` on a condition variable, which can emerge from waiting either after it times out or upon notice. Implement `DelayedExecutor` as an aspected class that allows structured white- box testing. Implement unit tests that ensure particular execution paths both with and without interruption. Add support classes `PausePoint` and `AntiLock` to support this testing. In the timeout task class, replace its `JsEnginePtr` member with a pointer to `JsEngineInternal`. Schedule it with the engine instead of making a thread for itself. Add member of type `DelayedExecutor`, and replace the call to `sleep_for()` with a call to `RunWithDelay`. For #3595, the present change prepares to eliminate as-if detached threads that arise from long delays in JS "SetTimeout". Although this does not itself end the timeout early, the member function `ForestallExecution` is available to do so in a subsequent change. Remove dead code. `StartImmediatelyInSingleUseDetachedThread`, `HeapFunction`, `MakeHeapFunction`. Timeout was the last remaining use of these transitional facilities.

Patch Set 1 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1030 lines, -235 lines) Patch
M libadblockplus.gyp View 2 chunks +5 lines, -0 lines 0 comments Download
M src/GlobalJsObject.h View 2 chunks +0 lines, -6 lines 0 comments Download
M src/GlobalJsObject.cpp View 1 chunk +0 lines, -102 lines 0 comments Download
M src/JsEngine.cpp View 2 chunks +4 lines, -1 line 0 comments Download
M src/JsEngineInternal.h View 2 chunks +24 lines, -42 lines 0 comments Download
M src/Scheduler.h View 7 chunks +76 lines, -57 lines 0 comments Download
M src/Scheduler.cpp View 1 chunk +0 lines, -19 lines 0 comments Download
A src/Timeout.h View 1 chunk +303 lines, -0 lines 0 comments Download
A src/Timeout.cpp View 1 chunk +120 lines, -0 lines 0 comments Download
A test/PausePoint.h View 1 chunk +173 lines, -0 lines 0 comments Download
A test/PausePoint.cpp View 1 chunk +70 lines, -0 lines 0 comments Download
M test/SchedulerTest.cpp View 4 chunks +35 lines, -8 lines 0 comments Download
A test/TimeoutTest.cpp View 1 chunk +220 lines, -0 lines 0 comments Download

Messages

Total messages: 1
Eric
Jan. 9, 2017, 1:47 p.m. (2017-01-09 13:47:43 UTC) #1
This change completes work on both listed issues. For #4692, a shared_ptr to the
engine is now held in the scheduler adjacent to the task, rather than in the
task itself. This could be improved further still, but it's adequate for now.
For #3595, all the threads we create are now joined in the destructor of the
scheduler, and hence also in the engine. Timeout threads are interruptible, so
we no longer hang while waiting for the hour-long update timer to fire.

I wrote an interruptible function executor to implement interruptible threads.
There are other ways to do this, such as using a priority queue in a more
sophisticated thread. I made my implementation decision out of expediency, so
that we can move on with other issue immediately rather than stacking up more
important tasks while waiting on a better scheduler. I've written schedulers
before, and they are difficult to make completely correct (free of race
conditions, etc.). It's better to proceed now with something that doesn't
perform quite as well instead of waiting. Nothing in the present code precludes
better scheduling. According to the English proverb: Don't let the best be the
enemy of the good.

This change finishes the last two blocking items for #3593 "Fix v8::Isolate
management", which we can now address. Because all the task threads terminate
quickly enough now, we can rely upon ordinarily C++ object lifetimes for
managing the isolate.

Powered by Google App Engine
This is Rietveld