Thread and Timeline fixes for Mojo.

Thread and Timeline fixes for Mojo.

Creates an OSThread in OSThread::Current() if a thread doesn't
already have one.

Also removes the setup of an unneeded StackZone from a Timeline API
function.


BUG=

Committed: https://github.com/dart-lang/sdk/commit/618d4ee9b741e42f1de61293a4cd7f5e68a84da7

[zra, 2015-11-30 18:59:37]

zra@google.com changed reviewers:
+ iposva@google.com

[Ivan Posva, 2015-12-01 05:08:42]

https://codereview.chromium.org/1483113002/diff/1/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/1483113002/diff/1/runtime/include/dart_api.h#...
runtime/include/dart_api.h:887: * Threads created by the embedder (other than
the thread that called
This should not be necessary if I understand the design correctly. Let's discuss
offline...

https://codereview.chromium.org/1483113002/diff/1/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/1483113002/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:5753: Timeline::ReclaimCachedBlocksFromThreads();
Please add a test that does request a timeline trace from a thread that has
never been seen by the VM before to validate.

[zra, 2015-12-01 16:55:13]

https://codereview.chromium.org/1483113002/diff/1/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/1483113002/diff/1/runtime/vm/dart_api_impl.cc...
runtime/vm/dart_api_impl.cc:5753: Timeline::ReclaimCachedBlocksFromThreads();
On 2015/12/01 05:08:42, Ivan Posva wrote:
> Please add a test that does request a timeline trace from a thread that has
> never been seen by the VM before to validate.

Done.

[zra, 2015-12-01 17:38:21]

ptal

https://codereview.chromium.org/1483113002/diff/1/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/1483113002/diff/1/runtime/include/dart_api.h#...
runtime/include/dart_api.h:887: * Threads created by the embedder (other than
the thread that called
On 2015/12/01 05:08:42, Ivan Posva wrote:
> This should not be necessary if I understand the design correctly. Let's
discuss
> offline...

Move to OSThread::Current() as discussed offline.

[zra, 2015-12-01 17:46:53]

Description was changed from

==========
Thread and Timeline fixes for Mojo.

Adds an API function that allocates an OSThread object for a thread if
it does not already have one. This is needed for threads that come
from the embedder and try to create an Isolate.

Also removes the setup of an unneeded StackZone from a Timeline API
function.


BUG=
==========

to

==========
Thread and Timeline fixes for Mojo.

Creates an OSThread in OSThread::Current() if a thread doesn't
already have one.

Also removes the setup of an unneeded StackZone from a Timeline API
function.


BUG=
==========

[Ivan Posva, 2015-12-01 18:00:51]

LGTMwC

-Ivan

https://codereview.chromium.org/1483113002/diff/60001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl_test.cc (right):

https://codereview.chromium.org/1483113002/diff/60001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl_test.cc:9342: struct GlobalTimelineThreadData {
How about making this a class with a destructor and allocating the monitor as
part of the constructor, freeing as part of the destructor.
Then you can also roll the free of the data part into the destruction.

https://codereview.chromium.org/1483113002/diff/60001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl_test.cc:9397: Dart_Handle result = Dart_Invoke(lib,
Single line?

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

https://codereview.chromium.org/1483113002/diff/60001/runtime/vm/os_thread.h#...
runtime/vm/os_thread.h:130: os_thread = new OSThread();
Wondering whether we should move this out into a helper function to make this
function small and inlineable.

[zra, 2015-12-01 18:51:51]

https://codereview.chromium.org/1483113002/diff/60001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl_test.cc (right):

https://codereview.chromium.org/1483113002/diff/60001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl_test.cc:9342: struct GlobalTimelineThreadData {
On 2015/12/01 18:00:51, Ivan Posva wrote:
> How about making this a class with a destructor and allocating the monitor as
> part of the constructor, freeing as part of the destructor.
> Then you can also roll the free of the data part into the destruction.

Done.

https://codereview.chromium.org/1483113002/diff/60001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl_test.cc:9397: Dart_Handle result = Dart_Invoke(lib,
On 2015/12/01 18:00:51, Ivan Posva wrote:
> Single line?

Done.

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

https://codereview.chromium.org/1483113002/diff/60001/runtime/vm/os_thread.h#...
runtime/vm/os_thread.h:130: os_thread = new OSThread();
On 2015/12/01 18:00:51, Ivan Posva wrote:
> Wondering whether we should move this out into a helper function to make this
> function small and inlineable.

Done.

[zra, 2015-12-01 18:52:22]

Description was changed from

==========
Thread and Timeline fixes for Mojo.

Creates an OSThread in OSThread::Current() if a thread doesn't
already have one.

Also removes the setup of an unneeded StackZone from a Timeline API
function.


BUG=
==========

to

==========
Thread and Timeline fixes for Mojo.

Creates an OSThread in OSThread::Current() if a thread doesn't
already have one.

Also removes the setup of an unneeded StackZone from a Timeline API
function.


BUG=

Committed:
https://github.com/dart-lang/sdk/commit/618d4ee9b741e42f1de61293a4cd7f5e68a84da7
==========

[zra, 2015-12-01 18:52:23]

Committed patchset #6 (id:100001) manually as
618d4ee9b741e42f1de61293a4cd7f5e68a84da7 (presubmit successful).