Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2189513002:
Move SimulatorHelper into V8 out of profiler clients. (Closed)
DescriptionMove SimulatorHelper into V8 out of profiler clients.
This patch is based on alph's CL https://codereview.chromium.org/2128613004/.
This patch makes GetStackSample propogate the register state when using
simulator helper, and adds argument to avoid using register state from simulator
when pass the native register state.
BUG=v8:4789
LOG=N
Committed: https://crrev.com/c72f637c73823c64b3baea069be73efbca01e747
Cr-Commit-Position: refs/heads/master@{#38554}
Patch Set 1 #
Total comments: 2
Patch Set 2 : update and rebase #
Total comments: 14
Patch Set 3 : Address alph's comments #
Total comments: 12
Patch Set 4 : Update #
Messages
Total messages: 35 (21 generated)
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lpy@chromium.org changed reviewers: + alph@chromium.org
PTAL.
Honestly, I don't like how the API looks after this patch. https://codereview.chromium.org/2189513002/diff/1/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/2189513002/diff/1/include/v8-profiler.h#newco... include/v8-profiler.h:78: RegisterState& updated_state, Hmm. Is it an output parameter? Where is it used? https://codereview.chromium.org/2189513002/diff/1/include/v8-profiler.h#newco... include/v8-profiler.h:80: bool use_first_state = false); The name looks a bit awkward. Not sure about a better name though. :-/
On 2016/07/28 23:57:06, alph wrote: > Honestly, I don't like how the API looks after this patch. > > https://codereview.chromium.org/2189513002/diff/1/include/v8-profiler.h > File include/v8-profiler.h (right): > > https://codereview.chromium.org/2189513002/diff/1/include/v8-profiler.h#newco... > include/v8-profiler.h:78: RegisterState& updated_state, > Hmm. Is it an output parameter? Where is it used? Yes. It's used in TickSample::Init, line 166 in the patch, which is #if defined(USE_SIMULATOR) RegisterState regs = use_first_state ? first_state : updated_state; #else const RegisterState& regs = first_state; #endif > https://codereview.chromium.org/2189513002/diff/1/include/v8-profiler.h#newco... > include/v8-profiler.h:80: bool use_first_state = false); > The name looks a bit awkward. Not sure about a better name though. :-/ will try to think a better name.
On 2016/07/29 01:06:58, lpy wrote: > On 2016/07/28 23:57:06, alph wrote: > > Honestly, I don't like how the API looks after this patch. > > > > https://codereview.chromium.org/2189513002/diff/1/include/v8-profiler.h > > File include/v8-profiler.h (right): > > > > > https://codereview.chromium.org/2189513002/diff/1/include/v8-profiler.h#newco... > > include/v8-profiler.h:78: RegisterState& updated_state, > > Hmm. Is it an output parameter? Where is it used? > > Yes. It's used in TickSample::Init, line 166 in the patch, which is > > #if defined(USE_SIMULATOR) > RegisterState regs = use_first_state ? first_state : updated_state; > #else > const RegisterState& regs = first_state; > #endif > Well, it is used as an input parameter here. Then having two parameters and a flag that commands which one to pick looks weird. There could be just one. As I said in my first comment this patch looks quite weird to me. Replacing a single transform function FillRegisters with a bunch of flags and multiple 'regs' arguments doesn't look like API simplification. > > > https://codereview.chromium.org/2189513002/diff/1/include/v8-profiler.h#newco... > > include/v8-profiler.h:80: bool use_first_state = false); > > The name looks a bit awkward. Not sure about a better name though. :-/ > > will try to think a better name.
On 2016/08/02 07:20:29, alph_slow wrote: > On 2016/07/29 01:06:58, lpy wrote: > > On 2016/07/28 23:57:06, alph wrote: > > > Honestly, I don't like how the API looks after this patch. > > > > > > https://codereview.chromium.org/2189513002/diff/1/include/v8-profiler.h > > > File include/v8-profiler.h (right): > > > > > > > > > https://codereview.chromium.org/2189513002/diff/1/include/v8-profiler.h#newco... > > > include/v8-profiler.h:78: RegisterState& updated_state, > > > Hmm. Is it an output parameter? Where is it used? > > > > Yes. It's used in TickSample::Init, line 166 in the patch, which is > > > > #if defined(USE_SIMULATOR) > > RegisterState regs = use_first_state ? first_state : updated_state; > > #else > > const RegisterState& regs = first_state; > > #endif > > > > Well, it is used as an input parameter here. Then having two parameters and a > flag that commands which one to pick looks weird. There could be just one. > > As I said in my first comment this patch looks quite weird to me. > Replacing a single transform function FillRegisters with a bunch of flags and > multiple 'regs' arguments doesn't look like API simplification. > I am not sure I understand correctly. We want to centralize the use of SimulatorHelper into GetStackSample, which is the place to get samples. But under some circumstances we don't want to use register state from simulator, so a flag is needed. And depends on where we want to propagate the register state comes from simulator, we definitely need the register state from simulator in TickSample::Init, but do we also need it in the caller of TickSample::Init?
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I made a few updates, ptal.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2189513002/diff/20001/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/2189513002/diff/20001/include/v8-profiler.h#n... include/v8-profiler.h:66: * \param isolate The currect isolate. typo. Perhaps just "The isolate" would be fine. https://codereview.chromium.org/2189513002/diff/20001/include/v8-profiler.h#n... include/v8-profiler.h:77: bool update_state = true); How does it update the state if it's declared const? Perhaps the name is misleading. How about use_simulator_state? https://codereview.chromium.org/2189513002/diff/20001/include/v8-profiler.h#n... include/v8-profiler.h:94: * interrupted. Otherwise the behavior is undefined. Could you please also note that the function is thread and signal safe. https://codereview.chromium.org/2189513002/diff/20001/include/v8-profiler.h#n... include/v8-profiler.h:96: static bool GetStackSample(Isolate* isolate, v8::RegisterState& state, if state is an in-out parameter now, it should be passed by pointer. https://codereview.chromium.org/2189513002/diff/20001/src/profiler/cpu-profil... File src/profiler/cpu-profiler.cc (right): https://codereview.chromium.org/2189513002/diff/20001/src/profiler/cpu-profil... src/profiler/cpu-profiler.cc:30: sample->Init(isolate, regs, TickSample::kIncludeCEntryFrame, false); Why you changed update_stats to false? Seems to be wrong. https://codereview.chromium.org/2189513002/diff/20001/src/profiler/tick-sampl... File src/profiler/tick-sample.cc (right): https://codereview.chromium.org/2189513002/diff/20001/src/profiler/tick-sampl... src/profiler/tick-sample.cc:165: RegisterState regs = update_state ? tmp_state : reg_state; It should be fine to always use tmp_state. right? https://codereview.chromium.org/2189513002/diff/20001/src/profiler/tick-sampl... src/profiler/tick-sample.cc:204: if (!i::SimulatorHelper::FillRegisters(isolate, &state)) { nit: you can omit {}
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the comments, I made a few updates. PTAL. https://codereview.chromium.org/2189513002/diff/20001/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/2189513002/diff/20001/include/v8-profiler.h#n... include/v8-profiler.h:66: * \param isolate The currect isolate. On 2016/08/08 21:36:57, alph wrote: > typo. Perhaps just "The isolate" would be fine. Done. https://codereview.chromium.org/2189513002/diff/20001/include/v8-profiler.h#n... include/v8-profiler.h:77: bool update_state = true); On 2016/08/08 21:36:57, alph wrote: > How does it update the state if it's declared const? My question is where we want to propagate the updated register state from simulator? Here I assume the caller of TickSample::Init doesn't need to know. In the current CL only the caller of TickSample::GetStackSample get the updated register state. > Perhaps the name is misleading. How about use_simulator_state? Is it OK to expose the fact that we are running on simulator? What about `allow_updated_state`? https://codereview.chromium.org/2189513002/diff/20001/include/v8-profiler.h#n... include/v8-profiler.h:94: * interrupted. Otherwise the behavior is undefined. On 2016/08/08 21:36:57, alph wrote: > Could you please also note that the function is thread and signal safe. Done. https://codereview.chromium.org/2189513002/diff/20001/include/v8-profiler.h#n... include/v8-profiler.h:96: static bool GetStackSample(Isolate* isolate, v8::RegisterState& state, On 2016/08/08 21:36:57, alph wrote: > if state is an in-out parameter now, it should be passed by pointer. Done. https://codereview.chromium.org/2189513002/diff/20001/src/profiler/cpu-profil... File src/profiler/cpu-profiler.cc (right): https://codereview.chromium.org/2189513002/diff/20001/src/profiler/cpu-profil... src/profiler/cpu-profiler.cc:30: sample->Init(isolate, regs, TickSample::kIncludeCEntryFrame, false); On 2016/08/08 21:36:57, alph wrote: > Why you changed update_stats to false? Seems to be wrong. It's a mistake, sorry. Done. https://codereview.chromium.org/2189513002/diff/20001/src/profiler/tick-sampl... File src/profiler/tick-sample.cc (right): https://codereview.chromium.org/2189513002/diff/20001/src/profiler/tick-sampl... src/profiler/tick-sample.cc:165: RegisterState regs = update_state ? tmp_state : reg_state; On 2016/08/08 21:36:57, alph wrote: > It should be fine to always use tmp_state. right? Done. https://codereview.chromium.org/2189513002/diff/20001/src/profiler/tick-sampl... src/profiler/tick-sample.cc:204: if (!i::SimulatorHelper::FillRegisters(isolate, &state)) { On 2016/08/08 21:36:57, alph wrote: > nit: you can omit {} Done.
lgtm https://codereview.chromium.org/2189513002/diff/40001/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/2189513002/diff/40001/include/v8-profiler.h#n... include/v8-profiler.h:70: * \param update_state If set to true, V8 will update the register state Please change description to something like: When set to true and V8 is running under a simulator, the method will use the simulator register state rather than the one provided with |state| argument. Otherwise the method will use provided register |state| as is. https://codereview.chromium.org/2189513002/diff/40001/include/v8-profiler.h#n... include/v8-profiler.h:71: when running on simulator. Default value is true, missing * https://codereview.chromium.org/2189513002/diff/40001/include/v8-profiler.h#n... include/v8-profiler.h:90: when running on simulator. Default value is true, Missing * https://codereview.chromium.org/2189513002/diff/40001/include/v8-profiler.h#n... include/v8-profiler.h:101: bool update_state = true); I'd still mention the word simulator here, as otherwise it's not clear why would it need to update the state. So something like use_simulator_reg_state looks better to me. https://codereview.chromium.org/2189513002/diff/40001/src/profiler/tick-sampl... File src/profiler/tick-sample.cc (right): https://codereview.chromium.org/2189513002/diff/40001/src/profiler/tick-sampl... src/profiler/tick-sample.cc:150: const RegisterState& reg_state, state https://codereview.chromium.org/2189513002/diff/40001/src/profiler/tick-sampl... src/profiler/tick-sample.cc:197: if (update_state) { update_state is never referenced when not under simulator. You may get some compiler warning. USE(update_state) should help.
The CQ bit was checked by lpy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lpy@chromium.org changed reviewers: + yangguo@chromium.org
Thanks, I updated the CL. + Yang@ for review, ptal. https://codereview.chromium.org/2189513002/diff/40001/include/v8-profiler.h File include/v8-profiler.h (right): https://codereview.chromium.org/2189513002/diff/40001/include/v8-profiler.h#n... include/v8-profiler.h:70: * \param update_state If set to true, V8 will update the register state On 2016/08/09 19:03:41, alph wrote: > Please change description to something like: > When set to true and V8 is running under a simulator, the method will use the > simulator register state rather than the one provided with |state| argument. > Otherwise the method will use provided register |state| as is. Done. https://codereview.chromium.org/2189513002/diff/40001/include/v8-profiler.h#n... include/v8-profiler.h:71: when running on simulator. Default value is true, On 2016/08/09 19:03:41, alph wrote: > missing * Done. https://codereview.chromium.org/2189513002/diff/40001/include/v8-profiler.h#n... include/v8-profiler.h:90: when running on simulator. Default value is true, On 2016/08/09 19:03:41, alph wrote: > Missing * Done. https://codereview.chromium.org/2189513002/diff/40001/include/v8-profiler.h#n... include/v8-profiler.h:101: bool update_state = true); On 2016/08/09 19:03:41, alph wrote: > I'd still mention the word simulator here, as otherwise it's not clear why would > it need to update the state. > > So something like use_simulator_reg_state looks better to me. Done. https://codereview.chromium.org/2189513002/diff/40001/src/profiler/tick-sampl... File src/profiler/tick-sample.cc (right): https://codereview.chromium.org/2189513002/diff/40001/src/profiler/tick-sampl... src/profiler/tick-sample.cc:150: const RegisterState& reg_state, On 2016/08/09 19:03:41, alph wrote: > state TickSample already has a different type of data member called state. https://codereview.chromium.org/2189513002/diff/40001/src/profiler/tick-sampl... src/profiler/tick-sample.cc:197: if (update_state) { On 2016/08/09 19:03:41, alph wrote: > update_state is never referenced when not under simulator. You may get some > compiler warning. USE(update_state) should help. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/10 05:50:04, lpy wrote: > Thanks, I updated the CL. > > + Yang@ for review, ptal. > > https://codereview.chromium.org/2189513002/diff/40001/include/v8-profiler.h > File include/v8-profiler.h (right): > > https://codereview.chromium.org/2189513002/diff/40001/include/v8-profiler.h#n... > include/v8-profiler.h:70: * \param update_state If set to true, V8 will update > the register state > On 2016/08/09 19:03:41, alph wrote: > > Please change description to something like: > > When set to true and V8 is running under a simulator, the method will use the > > simulator register state rather than the one provided with |state| argument. > > Otherwise the method will use provided register |state| as is. > > Done. > > https://codereview.chromium.org/2189513002/diff/40001/include/v8-profiler.h#n... > include/v8-profiler.h:71: when running on simulator. Default value is true, > On 2016/08/09 19:03:41, alph wrote: > > missing * > > Done. > > https://codereview.chromium.org/2189513002/diff/40001/include/v8-profiler.h#n... > include/v8-profiler.h:90: when running on simulator. Default value is true, > On 2016/08/09 19:03:41, alph wrote: > > Missing * > > Done. > > https://codereview.chromium.org/2189513002/diff/40001/include/v8-profiler.h#n... > include/v8-profiler.h:101: bool update_state = true); > On 2016/08/09 19:03:41, alph wrote: > > I'd still mention the word simulator here, as otherwise it's not clear why > would > > it need to update the state. > > > > So something like use_simulator_reg_state looks better to me. > > Done. > > https://codereview.chromium.org/2189513002/diff/40001/src/profiler/tick-sampl... > File src/profiler/tick-sample.cc (right): > > https://codereview.chromium.org/2189513002/diff/40001/src/profiler/tick-sampl... > src/profiler/tick-sample.cc:150: const RegisterState& reg_state, > On 2016/08/09 19:03:41, alph wrote: > > state > > TickSample already has a different type of data member called state. > > https://codereview.chromium.org/2189513002/diff/40001/src/profiler/tick-sampl... > src/profiler/tick-sample.cc:197: if (update_state) { > On 2016/08/09 19:03:41, alph wrote: > > update_state is never referenced when not under simulator. You may get some > > compiler warning. USE(update_state) should help. > > Done. lgtm.
The CQ bit was checked by lpy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org Link to the patchset: https://codereview.chromium.org/2189513002/#ps60001 (title: "Update")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Move SimulatorHelper into V8 out of profiler clients. This patch is based on alph's CL https://codereview.chromium.org/2128613004/. This patch makes GetStackSample propogate the register state when using simulator helper, and adds argument to avoid using register state from simulator when pass the native register state. BUG=v8:4789 LOG=N ========== to ========== Move SimulatorHelper into V8 out of profiler clients. This patch is based on alph's CL https://codereview.chromium.org/2128613004/. This patch makes GetStackSample propogate the register state when using simulator helper, and adds argument to avoid using register state from simulator when pass the native register state. BUG=v8:4789 LOG=N Committed: https://crrev.com/c72f637c73823c64b3baea069be73efbca01e747 Cr-Commit-Position: refs/heads/master@{#38554} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c72f637c73823c64b3baea069be73efbca01e747 Cr-Commit-Position: refs/heads/master@{#38554} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
