Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2916313003:
[Fuchsia] Enable CPU profiling for the standalone VM (Closed)
|
Created:
3 years, 6 months ago by zra Modified:
3 years, 6 months ago CC:
reviews_dartlang.org, vm-dev_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
Description[Fuchsia] Enable CPU profiling for the standalone VM
R=asiva@google.com
Committed: https://github.com/dart-lang/sdk/commit/5e8799ce8162b35466dc8afe0db95de2796b6cc1
Patch Set 1 #
Total comments: 7
Patch Set 2 : Poll until suspended #
Total comments: 5
Patch Set 3 : Address comments #Patch Set 4 : MG-795 is fixed #Patch Set 5 : Only poll 10 times #Patch Set 6 : Format #
Messages
Total messages: 14 (3 generated)
zra@google.com changed reviewers: + asiva@google.com, dje@google.com
https://codereview.chromium.org/2916313003/diff/1/runtime/vm/thread_interrupt... File runtime/vm/thread_interrupter_fuchsia.cc (right): https://codereview.chromium.org/2916313003/diff/1/runtime/vm/thread_interrupt... runtime/vm/thread_interrupter_fuchsia.cc:162: if (thread_info.state != MX_THREAD_STATE_SUSPENDED) { @dje: Frequently, after calling mx_task_suspend() on a thread, that thread's state will still be MX_THREAD_STATE_RUNNING. Is it expected that that thread will soon enter the SUSPENDED state? Should I poll to wait for it?
dje@chromium.org changed reviewers: + dje@chromium.org
https://codereview.chromium.org/2916313003/diff/1/runtime/vm/thread_interrupt... File runtime/vm/thread_interrupter_fuchsia.cc (right): https://codereview.chromium.org/2916313003/diff/1/runtime/vm/thread_interrupt... runtime/vm/thread_interrupter_fuchsia.cc:78: char buf[MX_MAX_THREAD_STATE_SIZE]; This won't necessarily have sufficient alignment, will it? https://codereview.chromium.org/2916313003/diff/1/runtime/vm/thread_interrupt... runtime/vm/thread_interrupter_fuchsia.cc:162: if (thread_info.state != MX_THREAD_STATE_SUSPENDED) { On 2017/06/02 16:51:23, zra wrote: > @dje: Frequently, after calling mx_task_suspend() on a thread, that thread's > state will still be MX_THREAD_STATE_RUNNING. Is it expected that that thread > will soon enter the SUSPENDED state? Should I poll to wait for it? Yeah, you currently have to poll to wait for the thread to suspend. This is a good reason to add something better (e.g., waiting for some kind of "signal" (in quotes to not suggest any particular solution)).
https://codereview.chromium.org/2916313003/diff/1/runtime/vm/thread_interrupt... File runtime/vm/thread_interrupter_fuchsia.cc (right): https://codereview.chromium.org/2916313003/diff/1/runtime/vm/thread_interrupt... runtime/vm/thread_interrupter_fuchsia.cc:78: char buf[MX_MAX_THREAD_STATE_SIZE]; On 2017/06/02 17:09:06, dje wrote: > This won't necessarily have sufficient alignment, will it? The docs don't mention alignment requirements for this buffer. https://codereview.chromium.org/2916313003/diff/1/runtime/vm/thread_interrupt... runtime/vm/thread_interrupter_fuchsia.cc:162: if (thread_info.state != MX_THREAD_STATE_SUSPENDED) { On 2017/06/02 17:09:06, dje wrote: > On 2017/06/02 16:51:23, zra wrote: > > @dje: Frequently, after calling mx_task_suspend() on a thread, that thread's > > state will still be MX_THREAD_STATE_RUNNING. Is it expected that that thread > > will soon enter the SUSPENDED state? Should I poll to wait for it? > > Yeah, you currently have to poll to wait for the thread to suspend. This is a > good reason to add something better (e.g., waiting for some kind of "signal" (in > quotes to not suggest any particular solution)). Changed to poll.
https://codereview.chromium.org/2916313003/diff/20001/runtime/vm/thread_inter... File runtime/vm/thread_interrupter_fuchsia.cc (right): https://codereview.chromium.org/2916313003/diff/20001/runtime/vm/thread_inter... runtime/vm/thread_interrupter_fuchsia.cc:50: } Not sure I understand why suspended_ remains true when status returned is ERR_NOT_SUPPORTED, when suspended_ is true we end up calling PollThreadUntilSuspended which would loop for ever right? https://codereview.chromium.org/2916313003/diff/20001/runtime/vm/thread_inter... runtime/vm/thread_interrupter_fuchsia.cc:157: } Why not change the API of mx_thread_read_state(...) to return an error indicating the thread is running MX_THREAD_STATE_RUNNING when called on a running thread. That way we could suspend the thread while (true) { retval = GrabRegisters if (retval == NO_ERROR) { Sample the thread; return; } if (retval == MX_THREAD_STATE_RUNNING) { continue; } handle error case; }
https://codereview.chromium.org/2916313003/diff/1/runtime/vm/thread_interrupt... File runtime/vm/thread_interrupter_fuchsia.cc (right): https://codereview.chromium.org/2916313003/diff/1/runtime/vm/thread_interrupt... runtime/vm/thread_interrupter_fuchsia.cc:78: char buf[MX_MAX_THREAD_STATE_SIZE]; On 2017/06/02 17:46:47, zra wrote: > On 2017/06/02 17:09:06, dje wrote: > > This won't necessarily have sufficient alignment, will it? > > The docs don't mention alignment requirements for this buffer. The requirements are more related to how the contents of the buffer are used. You're casting a pointer to a plain array of chars to a pointer to something with higher alignment requirements and then dereferencing the pointer. There are several solutions. You could just declare the buffer to be mx_${arch}_general_regs_t (suitably ifdef'd of course). Or you could make the buffer a union of the char array and a uint64_t (which is sufficient for this particular case). Or you could use __attribute__((aligned(sizeof(uint64_t))) on buf. Or make buf an array of uint64_t instead of chars.
https://codereview.chromium.org/2916313003/diff/20001/runtime/vm/thread_inter... File runtime/vm/thread_interrupter_fuchsia.cc (right): https://codereview.chromium.org/2916313003/diff/20001/runtime/vm/thread_inter... runtime/vm/thread_interrupter_fuchsia.cc:157: } On 2017/06/02 18:28:32, siva wrote: > Why not change the API of mx_thread_read_state(...) to return an error > indicating the thread is running MX_THREAD_STATE_RUNNING when called on a > running thread. > > That way we could > > suspend the thread > while (true) { > retval = GrabRegisters > if (retval == NO_ERROR) { > Sample the thread; > return; > } > if (retval == MX_THREAD_STATE_RUNNING) { > continue; > } > handle error case; > } This exists as ERR_BAD_STATE. https://fuchsia.googlesource.com/magenta/+/master/docs/syscalls/thread_read_s...
https://codereview.chromium.org/2916313003/diff/20001/runtime/vm/thread_inter... File runtime/vm/thread_interrupter_fuchsia.cc (right): https://codereview.chromium.org/2916313003/diff/20001/runtime/vm/thread_inter... runtime/vm/thread_interrupter_fuchsia.cc:157: } On 2017/06/02 18:48:51, dje wrote: > On 2017/06/02 18:28:32, siva wrote: > > Why not change the API of mx_thread_read_state(...) to return an error > > indicating the thread is running MX_THREAD_STATE_RUNNING when called on a > > running thread. > > > > That way we could > > > > suspend the thread > > while (true) { > > retval = GrabRegisters > > if (retval == NO_ERROR) { > > Sample the thread; > > return; > > } > > if (retval == MX_THREAD_STATE_RUNNING) { > > continue; > > } > > handle error case; > > } > > This exists as ERR_BAD_STATE. > https://fuchsia.googlesource.com/magenta/+/master/docs/syscalls/thread_read_s... ERR_BAD_STATE could potentially be returned for other kinds of errors too right not just for the fact that the thread has not been suspended.
On 2017/06/02 19:43:57, siva wrote: > https://codereview.chromium.org/2916313003/diff/20001/runtime/vm/thread_inter... > File runtime/vm/thread_interrupter_fuchsia.cc (right): > > https://codereview.chromium.org/2916313003/diff/20001/runtime/vm/thread_inter... > runtime/vm/thread_interrupter_fuchsia.cc:157: } > On 2017/06/02 18:48:51, dje wrote: > > On 2017/06/02 18:28:32, siva wrote: > > > Why not change the API of mx_thread_read_state(...) to return an error > > > indicating the thread is running MX_THREAD_STATE_RUNNING when called on a > > > running thread. > > > > > > That way we could > > > > > > suspend the thread > > > while (true) { > > > retval = GrabRegisters > > > if (retval == NO_ERROR) { > > > Sample the thread; > > > return; > > > } > > > if (retval == MX_THREAD_STATE_RUNNING) { > > > continue; > > > } > > > handle error case; > > > } > > > > This exists as ERR_BAD_STATE. > > > https://fuchsia.googlesource.com/magenta/+/master/docs/syscalls/thread_read_s... > > ERR_BAD_STATE could potentially be returned for other kinds of errors too right > not just for the fact that the thread has not been suspended. Potentially. If we get the ability to wait for the thread to suspend then this won't be needed. If one wanted to be absolutely safe in the mean time one can get the thread state with mx_object_get_info(MX_INFO_THREAD) and test mx_info_thread_t.state == MX_THREAD_STATE_SUSPENDED.
https://codereview.chromium.org/2916313003/diff/1/runtime/vm/thread_interrupt... File runtime/vm/thread_interrupter_fuchsia.cc (right): https://codereview.chromium.org/2916313003/diff/1/runtime/vm/thread_interrupt... runtime/vm/thread_interrupter_fuchsia.cc:78: char buf[MX_MAX_THREAD_STATE_SIZE]; On 2017/06/02 18:40:07, dje wrote: > On 2017/06/02 17:46:47, zra wrote: > > On 2017/06/02 17:09:06, dje wrote: > > > This won't necessarily have sufficient alignment, will it? > > > > The docs don't mention alignment requirements for this buffer. > > The requirements are more related to how the contents of the buffer are used. > You're casting a pointer to a plain array of chars to a pointer to something > with higher alignment requirements and then dereferencing the pointer. > > There are several solutions. You could just declare the buffer to be > mx_${arch}_general_regs_t (suitably ifdef'd of course). Or you could make the > buffer a union of the char array and a uint64_t (which is sufficient for this > particular case). Or you could use __attribute__((aligned(sizeof(uint64_t))) on > buf. Or make buf an array of uint64_t instead of chars. Done. https://codereview.chromium.org/2916313003/diff/20001/runtime/vm/thread_inter... File runtime/vm/thread_interrupter_fuchsia.cc (right): https://codereview.chromium.org/2916313003/diff/20001/runtime/vm/thread_inter... runtime/vm/thread_interrupter_fuchsia.cc:50: } On 2017/06/02 18:28:32, siva wrote: > Not sure I understand why suspended_ remains true when status returned is > ERR_NOT_SUPPORTED, when suspended_ is true we end up calling > PollThreadUntilSuspended which would loop for ever right? Whoops. You're right. Changed to set suspended_ to false whenever status != NO_ERROR.
lgtm
Description was changed from ========== [Fuchsia] Enable CPU profiling for the standalone VM ========== to ========== [Fuchsia] Enable CPU profiling for the standalone VM R=asiva@google.com Committed: https://github.com/dart-lang/sdk/commit/5e8799ce8162b35466dc8afe0db95de2796b6cc1 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as 5e8799ce8162b35466dc8afe0db95de2796b6cc1 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
