Initial design for background compilation

Initial design for background compilation


BUG=
R=koda@google.com

Committed: https://github.com/dart-lang/sdk/commit/193eadbc10399325cd083b519445b7bc71c944ca

[srdjan, 2015-10-01 21:48:33]

srdjan@google.com changed reviewers:
+ iposva@google.com, koda@google.com

[srdjan, 2015-10-01 21:48:33]

Missing pieces for making it work:
- Move VM tags from isolates to thread (next CL)
- Handles must use zone instead of isolates.

[koda, 2015-10-01 22:27:02]

Initial comments.

https://codereview.chromium.org/1386503002/diff/20001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/1386503002/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:1474: // bool_ is owned by 'Stop', do not delete here.
bool_ -> done_

https://codereview.chromium.org/1386503002/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:1503: // still happens make function as not optimizable.
make -> mark?

https://codereview.chromium.org/1386503002/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:1556: }
Starting here, 'task' is concurrently destructed by the thread pool worker,
right? So, we're assuming the C++ compiler doesn't touch 'task'. Better at least
add a comment about this.

https://codereview.chromium.org/1386503002/diff/20001/runtime/vm/compiler.h
File runtime/vm/compiler.h (right):

https://codereview.chromium.org/1386503002/diff/20001/runtime/vm/compiler.h#n...
runtime/vm/compiler.h:107: ~CompileOptimizedTask();
I believe this is the first task that defines a destructor. Couldn't you do all
cleanup in Stop, to avoid the split?

Also consider adding 'virtual', for clarity.

https://codereview.chromium.org/1386503002/diff/20001/runtime/vm/compiler.h#n...
runtime/vm/compiler.h:118: static void Init(Isolate* isolate);
What happens if multiple threads call 'Init'? Consider renaming EnsureInit and
use the isolate's mutex.

https://codereview.chromium.org/1386503002/diff/20001/runtime/vm/thread_regis...
File runtime/vm/thread_registry.cc (right):

https://codereview.chromium.org/1386503002/diff/20001/runtime/vm/thread_regis...
runtime/vm/thread_registry.cc:42: // Make sure the task moves from Wait to a
safepoint.
This should not be needed anymore, now that you enter/exit the isolate in the
loop. I.e., while it's Waiting, the compilation task is not in this isolate, so
will not be included in the rendezvous.

[srdjan, 2015-10-01 23:11:24]

https://codereview.chromium.org/1386503002/diff/20001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/1386503002/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:1474: // bool_ is owned by 'Stop', do not delete here.
On 2015/10/01 22:27:02, koda wrote:
> bool_ -> done_

Done.

https://codereview.chromium.org/1386503002/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:1503: // still happens make function as not optimizable.
On 2015/10/01 22:27:02, koda wrote:
> make -> mark?

Done.

https://codereview.chromium.org/1386503002/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:1556: }
On 2015/10/01 22:27:02, koda wrote:
> Starting here, 'task' is concurrently destructed by the thread pool worker,
> right? So, we're assuming the C++ compiler doesn't touch 'task'. Better at
least
> add a comment about this.

Done.

https://codereview.chromium.org/1386503002/diff/20001/runtime/vm/compiler.h
File runtime/vm/compiler.h (right):

https://codereview.chromium.org/1386503002/diff/20001/runtime/vm/compiler.h#n...
runtime/vm/compiler.h:107: ~CompileOptimizedTask();
On 2015/10/01 22:27:02, koda wrote:
> I believe this is the first task that defines a destructor. Couldn't you do
all
> cleanup in Stop, to avoid the split?
> 
> Also consider adding 'virtual', for clarity.

Removes destructor, all cleanup in Stop.

https://codereview.chromium.org/1386503002/diff/20001/runtime/vm/compiler.h#n...
runtime/vm/compiler.h:118: static void Init(Isolate* isolate);
On 2015/10/01 22:27:02, koda wrote:
> What happens if multiple threads call 'Init'? Consider renaming EnsureInit and
> use the isolate's mutex.

Done. There is currently only one Dart code thread, but better safe!

https://codereview.chromium.org/1386503002/diff/20001/runtime/vm/thread_regis...
File runtime/vm/thread_registry.cc (right):

https://codereview.chromium.org/1386503002/diff/20001/runtime/vm/thread_regis...
runtime/vm/thread_registry.cc:42: // Make sure the task moves from Wait to a
safepoint.
On 2015/10/01 22:27:02, koda wrote:
> This should not be needed anymore, now that you enter/exit the isolate in the
> loop. I.e., while it's Waiting, the compilation task is not in this isolate,
so
> will not be included in the rendezvous.

Done.

[Florian Schneider, 2015-10-02 11:10:32]

fschneider@google.com changed reviewers:
+ fschneider@google.com

[Florian Schneider, 2015-10-02 11:10:33]

dbc:

https://codereview.chromium.org/1386503002/diff/40001/runtime/vm/thread_regis...
File runtime/vm/thread_registry.cc (right):

https://codereview.chromium.org/1386503002/diff/40001/runtime/vm/thread_regis...
runtime/vm/thread_registry.cc:7: #include "vm/compiler.h"
There is no other change in this file. Accidentally left over?

[srdjan, 2015-10-02 15:24:13]

https://codereview.chromium.org/1386503002/diff/40001/runtime/vm/thread_regis...
File runtime/vm/thread_registry.cc (right):

https://codereview.chromium.org/1386503002/diff/40001/runtime/vm/thread_regis...
runtime/vm/thread_registry.cc:7: #include "vm/compiler.h"
On 2015/10/02 11:10:32, Florian Schneider wrote:
> There is no other change in this file. Accidentally left over?

Done.

[koda, 2015-10-02 16:53:47]

https://codereview.chromium.org/1386503002/diff/40001/runtime/vm/thread_regis...
File runtime/vm/thread_registry.cc (right):

https://codereview.chromium.org/1386503002/diff/40001/runtime/vm/thread_regis...
runtime/vm/thread_registry.cc:7: #include "vm/compiler.h"
Remove.

https://codereview.chromium.org/1386503002/diff/60001/runtime/vm/code_generat...
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/1386503002/diff/60001/runtime/vm/code_generat...
runtime/vm/code_generator.cc:1455: if (isolate->compile_optimized_task() ==
NULL) {
This initial check is now redundant and can be removed.

https://codereview.chromium.org/1386503002/diff/60001/runtime/vm/compiler.h
File runtime/vm/compiler.h (right):

https://codereview.chromium.org/1386503002/diff/60001/runtime/vm/compiler.h#n...
runtime/vm/compiler.h:104: class CompileOptimizedTask : public ThreadPool::Task
{
The name "task" is slightly confusing to me, since it contains a work queue
itself. Did you consider something a bit more general like BackgroundCompiler or
ConcurrentOptimizer? It's up to you. (If you do decide to change it, then please
also change the isolate field name.)

[srdjan, 2015-10-02 18:19:20]

https://codereview.chromium.org/1386503002/diff/40001/runtime/vm/thread_regis...
File runtime/vm/thread_registry.cc (right):

https://codereview.chromium.org/1386503002/diff/40001/runtime/vm/thread_regis...
runtime/vm/thread_registry.cc:7: #include "vm/compiler.h"
On 2015/10/02 16:53:46, koda wrote:
> Remove.

Done.

https://codereview.chromium.org/1386503002/diff/60001/runtime/vm/code_generat...
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/1386503002/diff/60001/runtime/vm/code_generat...
runtime/vm/code_generator.cc:1455: if (isolate->compile_optimized_task() ==
NULL) {
On 2015/10/02 16:53:46, koda wrote:
> This initial check is now redundant and can be removed.

Done.

https://codereview.chromium.org/1386503002/diff/60001/runtime/vm/compiler.h
File runtime/vm/compiler.h (right):

https://codereview.chromium.org/1386503002/diff/60001/runtime/vm/compiler.h#n...
runtime/vm/compiler.h:104: class CompileOptimizedTask : public ThreadPool::Task
{
On 2015/10/02 16:53:46, koda wrote:
> The name "task" is slightly confusing to me, since it contains a work queue
> itself. Did you consider something a bit more general like BackgroundCompiler
or
> ConcurrentOptimizer? It's up to you. (If you do decide to change it, then
please
> also change the isolate field name.)

Renamed to BackgroundCompiler. Why should "Isolate* isolate_" name be changed?

[koda, 2015-10-02 18:58:05]

https://codereview.chromium.org/1386503002/diff/60001/runtime/vm/compiler.h
File runtime/vm/compiler.h (right):

https://codereview.chromium.org/1386503002/diff/60001/runtime/vm/compiler.h#n...
runtime/vm/compiler.h:104: class CompileOptimizedTask : public ThreadPool::Task
{
On 2015/10/02 18:19:20, srdjan wrote:
> On 2015/10/02 16:53:46, koda wrote:
> > The name "task" is slightly confusing to me, since it contains a work queue
> > itself. Did you consider something a bit more general like
BackgroundCompiler
> or
> > ConcurrentOptimizer? It's up to you. (If you do decide to change it, then
> please
> > also change the isolate field name.)
> 
> Renamed to BackgroundCompiler. Why should "Isolate* isolate_" name be changed?

Not that field; I meant the added fields/accessors in the class Isolate.

https://codereview.chromium.org/1386503002/diff/80001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/1386503002/diff/80001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:604: BackgroundCompiler* compile_optimized_task() const {
Rename accessors.

[srdjan, 2015-10-02 19:44:57]

https://codereview.chromium.org/1386503002/diff/80001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/1386503002/diff/80001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:604: BackgroundCompiler* compile_optimized_task() const {
On 2015/10/02 18:58:05, koda wrote:
> Rename accessors.

Done.

[koda, 2015-10-03 00:11:55]

Will you add a unit test that sets the flag, to exercise the code?

[srdjan, 2015-10-05 17:48:19]

On 2015/10/03 00:11:55, koda wrote:
> Will you add a unit test that sets the flag, to exercise the code?

Not planned at this stage. Once I get some simple background compilation going
(at least two more CLs pending) I will add a test that runs with background
compilation on.

[koda, 2015-10-05 18:13:00]

https://codereview.chromium.org/1386503002/diff/100001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/1386503002/diff/100001/runtime/vm/compiler.cc...
runtime/vm/compiler.cc:1408: class CompilationWorkQueue : public ValueObject {
Be consistent w.r.t. "work queue" vs "workqueue".

https://codereview.chromium.org/1386503002/diff/100001/runtime/vm/compiler.cc...
runtime/vm/compiler.cc:1435: RawFunction* Remove() {
Clarify/emphasize that AddUnique/Remove together make a FIFO, if you believe
this will remain true. (And if so, consider more standard names like PushFront
and PopBack.)

https://codereview.chromium.org/1386503002/diff/100001/runtime/vm/compiler.cc...
runtime/vm/compiler.cc:1480: // TODO(srdjan): We do not expect errors in
optimized code. If it
Clarify that you mean errors *while compiling* optimized code, rather than
running it.

https://codereview.chromium.org/1386503002/diff/100001/runtime/vm/compiler.cc...
runtime/vm/compiler.cc:1530: // 'task' will be deleted by thread pool.
This comment should go before NULL'ing task, to be accurate.

https://codereview.chromium.org/1386503002/diff/100001/runtime/vm/compiler.cc...
runtime/vm/compiler.cc:1553: Dart::thread_pool()->Run(task);
This does not need to be under the lock and is a non-trivial operation that
could involve other locks, so please move it outside the scope of 'ml'.

https://codereview.chromium.org/1386503002/diff/100001/runtime/vm/compiler.h
File runtime/vm/compiler.h (right):

https://codereview.chromium.org/1386503002/diff/100001/runtime/vm/compiler.h#...
runtime/vm/compiler.h:104: // Class to run optimizing compilation in a
background thread.
State that there is (at most) one per isolate.

https://codereview.chromium.org/1386503002/diff/100001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/1386503002/diff/100001/runtime/vm/isolate.h#n...
runtime/vm/isolate.h:35: class BackgroundCompiler;
Sort.

[srdjan, 2015-10-05 19:16:47]

PTAL

https://codereview.chromium.org/1386503002/diff/100001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/1386503002/diff/100001/runtime/vm/compiler.cc...
runtime/vm/compiler.cc:1408: class CompilationWorkQueue : public ValueObject {
On 2015/10/05 18:13:00, koda wrote:
> Be consistent w.r.t. "work queue" vs "workqueue".

Done; Two words.

https://codereview.chromium.org/1386503002/diff/100001/runtime/vm/compiler.cc...
runtime/vm/compiler.cc:1435: RawFunction* Remove() {
On 2015/10/05 18:13:00, koda wrote:
> Clarify/emphasize that AddUnique/Remove together make a FIFO, if you believe
> this will remain true. (And if so, consider more standard names like PushFront
> and PopBack.)

Renamed to PushFront/PopBack.

https://codereview.chromium.org/1386503002/diff/100001/runtime/vm/compiler.cc...
runtime/vm/compiler.cc:1480: // TODO(srdjan): We do not expect errors in
optimized code. If it
On 2015/10/05 18:13:00, koda wrote:
> Clarify that you mean errors *while compiling* optimized code, rather than
> running it.

Done.

https://codereview.chromium.org/1386503002/diff/100001/runtime/vm/compiler.cc...
runtime/vm/compiler.cc:1530: // 'task' will be deleted by thread pool.
On 2015/10/05 18:13:00, koda wrote:
> This comment should go before NULL'ing task, to be accurate.

Done.

https://codereview.chromium.org/1386503002/diff/100001/runtime/vm/compiler.cc...
runtime/vm/compiler.cc:1553: Dart::thread_pool()->Run(task);
On 2015/10/05 18:13:00, koda wrote:
> This does not need to be under the lock and is a non-trivial operation that
> could involve other locks, so please move it outside the scope of 'ml'.

Done.

https://codereview.chromium.org/1386503002/diff/100001/runtime/vm/compiler.h
File runtime/vm/compiler.h (right):

https://codereview.chromium.org/1386503002/diff/100001/runtime/vm/compiler.h#...
runtime/vm/compiler.h:104: // Class to run optimizing compilation in a
background thread.
On 2015/10/05 18:13:00, koda wrote:
> State that there is (at most) one per isolate.

Done.

https://codereview.chromium.org/1386503002/diff/100001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/1386503002/diff/100001/runtime/vm/isolate.h#n...
runtime/vm/isolate.h:35: class BackgroundCompiler;
On 2015/10/05 18:13:00, koda wrote:
> Sort.

Done.

[koda, 2015-10-05 19:22:54]

lgtm

[srdjan, 2015-10-05 19:29:44]

Committed patchset #7 (id:120001) manually as
193eadbc10399325cd083b519445b7bc71c944ca (presubmit successful).