Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1926863003:
Make Isolate::GetStackSample API support simulator (Closed)
DescriptionMake Isolate::GetStackSample API support simulator
Currently GetStackSample doesn't support simulator, thus sampler is aware of
simulator, but since we are moving it out, it shouldn't have knowledge of
simulator. This patch moves the logic using simulator accessible
to Isolate::GetStackSample, so that it supports simulator.
BUG=v8:4956
LOG=n
Committed: https://crrev.com/b027b623dff557dc88b70a7cbe422a8c56b27164
Cr-Commit-Position: refs/heads/master@{#35944}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #Patch Set 4 : #
Total comments: 10
Patch Set 5 : #
Total comments: 4
Patch Set 6 : #
Messages
Total messages: 31 (7 generated)
lpy@chromium.org changed reviewers: + alph@chromium.org, jochen@chromium.org
PTAL.
https://codereview.chromium.org/1926863003/diff/1/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1926863003/diff/1/include/v8.h#newcode5803 include/v8.h:5803: void GetStackSample(RegisterState* state, void** frames, Why do you need the second one? Can it once being run under the simulator just ignore the state arg and extract one from the simulator? https://codereview.chromium.org/1926863003/diff/1/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1926863003/diff/1/src/api.cc#newcode7530 src/api.cc:7530: void Isolate::GetStackSample(RegisterState* state, void** frames, v8 usually do not put that much implementation specific code into api.cc Also copying code is a bad practice, can you please move it into i::TickSample::GetStackSample and make the current sampler use it.
On 2016/04/27 22:29:20, alph wrote: > https://codereview.chromium.org/1926863003/diff/1/include/v8.h > File include/v8.h (right): > > https://codereview.chromium.org/1926863003/diff/1/include/v8.h#newcode5803 > include/v8.h:5803: void GetStackSample(RegisterState* state, void** frames, > Why do you need the second one? > Can it once being run under the simulator just ignore the state arg and extract > one from the simulator? The reason we need the second one is that Chrome is using this API, we want to make another one and modify Chrome to use the new one, and then delete the old one. Since the sampler library is going to use GetStackSample and simulator support is needed, jochen@ suggested putting the code here. > https://codereview.chromium.org/1926863003/diff/1/src/api.cc > File src/api.cc (right): > > https://codereview.chromium.org/1926863003/diff/1/src/api.cc#newcode7530 > src/api.cc:7530: void Isolate::GetStackSample(RegisterState* state, void** > frames, > v8 usually do not put that much implementation specific code into api.cc > Also copying code is a bad practice, can you please move it into > i::TickSample::GetStackSample and make the current sampler use it. Combining what alph@ and I discussed offline and what I know, there are several places that will call i::TickSample::Init, which will then call i::TickSample::GetStackSample with their register state, for example here: https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/profiler/cp... When running on simulator, if we putting simulator support in i::TickSample::GetStackSample, register state may get changed and lead to wrong register state. Since there are several places that will end up calling i::TickSample::GetStackSample, we may need to find a place where putting simulator support won't lead to wrong register state. I think Isolate::GetStackSample is the place since it's called by clients who are not aware of simulator, while code inside V8 knows it and thus knows how to get correct register state before calling i::TickSample::GetStackSample. Any suggestion?
Description was changed from ========== Make GetStackSample API support simulator Currently GetStackSample doesn't support simulator, thus sampler is aware of simulator, but since we are moving it out, it shouldn't have knowledge of simulator. This patch moves the logic using simulator to fill register state into GetStackSample, so that it supports simulator. BUG=v8:4956 LOG=n ========== to ========== Make GetStackSample API support simulator Currently GetStackSample doesn't support simulator, thus sampler is aware of simulator, but since we are moving it out, it shouldn't have knowledge of simulator. This patch moves the logic using simulator to fill register state into GetStackSample, so that it supports simulator. BUG=v8:4956 LOG=n ==========
On 2016/04/28 18:20:59, lpy wrote: > On 2016/04/27 22:29:20, alph wrote: > > https://codereview.chromium.org/1926863003/diff/1/include/v8.h > > File include/v8.h (right): > > > > https://codereview.chromium.org/1926863003/diff/1/include/v8.h#newcode5803 > > include/v8.h:5803: void GetStackSample(RegisterState* state, void** frames, > > Why do you need the second one? > > Can it once being run under the simulator just ignore the state arg and > extract > > one from the simulator? > > The reason we need the second one is that Chrome is using this API, we want to > make another one and modify Chrome to use the new one, and then delete the old > one. Hmm. What do you need to modify in chrome to make it use the new API? It seems to me that old and new APIs are backward compatible, so if you don't change the signature you wouldn't need to change the client in chrome. Am I missing something? > > Since the sampler library is going to use GetStackSample and simulator support > is needed, jochen@ suggested putting the code here. >
On 2016/04/29 00:12:35, alph wrote: > On 2016/04/28 18:20:59, lpy wrote: > > On 2016/04/27 22:29:20, alph wrote: > > > https://codereview.chromium.org/1926863003/diff/1/include/v8.h > > > File include/v8.h (right): > > > > > > https://codereview.chromium.org/1926863003/diff/1/include/v8.h#newcode5803 > > > include/v8.h:5803: void GetStackSample(RegisterState* state, void** frames, > > > Why do you need the second one? > > > Can it once being run under the simulator just ignore the state arg and > > extract > > > one from the simulator? > > > > The reason we need the second one is that Chrome is using this API, we want to > > make another one and modify Chrome to use the new one, and then delete the old > > one. > > Hmm. What do you need to modify in chrome to make it use the new API? It seems > to me that > old and new APIs are backward compatible, so if you don't change the signature > you > wouldn't need to change the client in chrome. Am I missing something? The signature will be changed to 'void Isolate::GetStackSample(RegisterState* state, ...)' rather than the current one 'void Isolate::GetStackSample(const RegisterState& state, ...)', because the const indicates that state won't be changed.
On 2016/04/29 00:15:23, lpy wrote: > On 2016/04/29 00:12:35, alph wrote: > > On 2016/04/28 18:20:59, lpy wrote: > > > On 2016/04/27 22:29:20, alph wrote: > > > > https://codereview.chromium.org/1926863003/diff/1/include/v8.h > > > > File include/v8.h (right): > > > > > > > > https://codereview.chromium.org/1926863003/diff/1/include/v8.h#newcode5803 > > > > include/v8.h:5803: void GetStackSample(RegisterState* state, void** > frames, > > > > Why do you need the second one? > > > > Can it once being run under the simulator just ignore the state arg and > > > extract > > > > one from the simulator? > > > > > > The reason we need the second one is that Chrome is using this API, we want > to > > > make another one and modify Chrome to use the new one, and then delete the > old > > > one. > > > > Hmm. What do you need to modify in chrome to make it use the new API? It seems > > to me that > > old and new APIs are backward compatible, so if you don't change the signature > > you > > wouldn't need to change the client in chrome. Am I missing something? > > The signature will be changed to 'void Isolate::GetStackSample(RegisterState* > state, ...)' rather than the current one 'void Isolate::GetStackSample(const > RegisterState& state, ...)', because the const indicates that state won't be > changed. why do you need the API function change the input parameter 'state'?
On 2016/04/29 00:20:28, alph wrote: > On 2016/04/29 00:15:23, lpy wrote: > > On 2016/04/29 00:12:35, alph wrote: > > > On 2016/04/28 18:20:59, lpy wrote: > > > > On 2016/04/27 22:29:20, alph wrote: > > > > > https://codereview.chromium.org/1926863003/diff/1/include/v8.h > > > > > File include/v8.h (right): > > > > > > > > > > > https://codereview.chromium.org/1926863003/diff/1/include/v8.h#newcode5803 > > > > > include/v8.h:5803: void GetStackSample(RegisterState* state, void** > > frames, > > > > > Why do you need the second one? > > > > > Can it once being run under the simulator just ignore the state arg and > > > > extract > > > > > one from the simulator? > > > > > > > > The reason we need the second one is that Chrome is using this API, we > want > > to > > > > make another one and modify Chrome to use the new one, and then delete the > > old > > > > one. > > > > > > Hmm. What do you need to modify in chrome to make it use the new API? It > seems > > > to me that > > > old and new APIs are backward compatible, so if you don't change the > signature > > > you > > > wouldn't need to change the client in chrome. Am I missing something? > > > > The signature will be changed to 'void Isolate::GetStackSample(RegisterState* > > state, ...)' rather than the current one 'void Isolate::GetStackSample(const > > RegisterState& state, ...)', because the const indicates that state won't be > > changed. > > why do you need the API function change the input parameter 'state'? We don't need to change it, we can just make a copy of 'state' and modify it if needed, but the const signature is confusing, since on simulator we are not using the register state provided by clients to get stack sample.
On 2016/04/29 00:23:31, lpy wrote: > On 2016/04/29 00:20:28, alph wrote: > > On 2016/04/29 00:15:23, lpy wrote: > > > On 2016/04/29 00:12:35, alph wrote: > > > > On 2016/04/28 18:20:59, lpy wrote: > > > > > On 2016/04/27 22:29:20, alph wrote: > > > > > > https://codereview.chromium.org/1926863003/diff/1/include/v8.h > > > > > > File include/v8.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1926863003/diff/1/include/v8.h#newcode5803 > > > > > > include/v8.h:5803: void GetStackSample(RegisterState* state, void** > > > frames, > > > > > > Why do you need the second one? > > > > > > Can it once being run under the simulator just ignore the state arg > and > > > > > extract > > > > > > one from the simulator? > > > > > > > > > > The reason we need the second one is that Chrome is using this API, we > > want > > > to > > > > > make another one and modify Chrome to use the new one, and then delete > the > > > old > > > > > one. > > > > > > > > Hmm. What do you need to modify in chrome to make it use the new API? It > > seems > > > > to me that > > > > old and new APIs are backward compatible, so if you don't change the > > signature > > > > you > > > > wouldn't need to change the client in chrome. Am I missing something? > > > > > > The signature will be changed to 'void > Isolate::GetStackSample(RegisterState* > > > state, ...)' rather than the current one 'void Isolate::GetStackSample(const > > > RegisterState& state, ...)', because the const indicates that state won't be > > > changed. > > > > why do you need the API function change the input parameter 'state'? > > We don't need to change it, we can just make a copy of 'state' and modify it if > needed, but the const signature is confusing, since on simulator we are not const signature is not related to whether the parameters is used or not. > using the register state provided by clients to get stack sample. That's fine if a parameter is not used under certain circumstances.
On 2016/04/29 00:25:32, alph wrote: > On 2016/04/29 00:23:31, lpy wrote: > > On 2016/04/29 00:20:28, alph wrote: > > > On 2016/04/29 00:15:23, lpy wrote: > > > > On 2016/04/29 00:12:35, alph wrote: > > > > > On 2016/04/28 18:20:59, lpy wrote: > > > > > > On 2016/04/27 22:29:20, alph wrote: > > > > > > > https://codereview.chromium.org/1926863003/diff/1/include/v8.h > > > > > > > File include/v8.h (right): > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1926863003/diff/1/include/v8.h#newcode5803 > > > > > > > include/v8.h:5803: void GetStackSample(RegisterState* state, void** > > > > frames, > > > > > > > Why do you need the second one? > > > > > > > Can it once being run under the simulator just ignore the state arg > > and > > > > > > extract > > > > > > > one from the simulator? > > > > > > > > > > > > The reason we need the second one is that Chrome is using this API, we > > > want > > > > to > > > > > > make another one and modify Chrome to use the new one, and then delete > > the > > > > old > > > > > > one. > > > > > > > > > > Hmm. What do you need to modify in chrome to make it use the new API? It > > > seems > > > > > to me that > > > > > old and new APIs are backward compatible, so if you don't change the > > > signature > > > > > you > > > > > wouldn't need to change the client in chrome. Am I missing something? > > > > > > > > The signature will be changed to 'void > > Isolate::GetStackSample(RegisterState* > > > > state, ...)' rather than the current one 'void > Isolate::GetStackSample(const > > > > RegisterState& state, ...)', because the const indicates that state won't > be > > > > changed. > > > > > > why do you need the API function change the input parameter 'state'? > > > > We don't need to change it, we can just make a copy of 'state' and modify it > if > > needed, but the const signature is confusing, since on simulator we are not > > const signature is not related to whether the parameters is used or not. > > > using the register state provided by clients to get stack sample. > > That's fine if a parameter is not used under certain circumstances. So you are suggesting we don't change the signature, but still modify the register state when needed, do I understand correctly?
On 2016/04/29 00:30:45, lpy wrote: > On 2016/04/29 00:25:32, alph wrote: > > On 2016/04/29 00:23:31, lpy wrote: > > > On 2016/04/29 00:20:28, alph wrote: > > > > On 2016/04/29 00:15:23, lpy wrote: > > > > > On 2016/04/29 00:12:35, alph wrote: > > > > > > On 2016/04/28 18:20:59, lpy wrote: > > > > > > > On 2016/04/27 22:29:20, alph wrote: > > > > > > > > https://codereview.chromium.org/1926863003/diff/1/include/v8.h > > > > > > > > File include/v8.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1926863003/diff/1/include/v8.h#newcode5803 > > > > > > > > include/v8.h:5803: void GetStackSample(RegisterState* state, > void** > > > > > frames, > > > > > > > > Why do you need the second one? > > > > > > > > Can it once being run under the simulator just ignore the state > arg > > > and > > > > > > > extract > > > > > > > > one from the simulator? > > > > > > > > > > > > > > The reason we need the second one is that Chrome is using this API, > we > > > > want > > > > > to > > > > > > > make another one and modify Chrome to use the new one, and then > delete > > > the > > > > > old > > > > > > > one. > > > > > > > > > > > > Hmm. What do you need to modify in chrome to make it use the new API? > It > > > > seems > > > > > > to me that > > > > > > old and new APIs are backward compatible, so if you don't change the > > > > signature > > > > > > you > > > > > > wouldn't need to change the client in chrome. Am I missing something? > > > > > > > > > > The signature will be changed to 'void > > > Isolate::GetStackSample(RegisterState* > > > > > state, ...)' rather than the current one 'void > > Isolate::GetStackSample(const > > > > > RegisterState& state, ...)', because the const indicates that state > won't > > be > > > > > changed. > > > > > > > > why do you need the API function change the input parameter 'state'? > > > > > > We don't need to change it, we can just make a copy of 'state' and modify it > > if > > > needed, but the const signature is confusing, since on simulator we are not > > > > const signature is not related to whether the parameters is used or not. > > > > > using the register state provided by clients to get stack sample. > > > > That's fine if a parameter is not used under certain circumstances. > > So you are suggesting we don't change the signature, but still modify the > register state when needed, do I understand correctly? Not modify, but pass another one if we're running under simulator. So the API function gets the current CPU state and based on this data retrieves JS stack, whether it's running native code or simulated. So the actual mode is not visible to the embedder.
On 2016/04/29 00:34:11, alph wrote: > On 2016/04/29 00:30:45, lpy wrote: > > On 2016/04/29 00:25:32, alph wrote: > > > On 2016/04/29 00:23:31, lpy wrote: > > > > On 2016/04/29 00:20:28, alph wrote: > > > > > On 2016/04/29 00:15:23, lpy wrote: > > > > > > On 2016/04/29 00:12:35, alph wrote: > > > > > > > On 2016/04/28 18:20:59, lpy wrote: > > > > > > > > On 2016/04/27 22:29:20, alph wrote: > > > > > > > > > https://codereview.chromium.org/1926863003/diff/1/include/v8.h > > > > > > > > > File include/v8.h (right): > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1926863003/diff/1/include/v8.h#newcode5803 > > > > > > > > > include/v8.h:5803: void GetStackSample(RegisterState* state, > > void** > > > > > > frames, > > > > > > > > > Why do you need the second one? > > > > > > > > > Can it once being run under the simulator just ignore the state > > arg > > > > and > > > > > > > > extract > > > > > > > > > one from the simulator? > > > > > > > > > > > > > > > > The reason we need the second one is that Chrome is using this > API, > > we > > > > > want > > > > > > to > > > > > > > > make another one and modify Chrome to use the new one, and then > > delete > > > > the > > > > > > old > > > > > > > > one. > > > > > > > > > > > > > > Hmm. What do you need to modify in chrome to make it use the new > API? > > It > > > > > seems > > > > > > > to me that > > > > > > > old and new APIs are backward compatible, so if you don't change the > > > > > signature > > > > > > > you > > > > > > > wouldn't need to change the client in chrome. Am I missing > something? > > > > > > > > > > > > The signature will be changed to 'void > > > > Isolate::GetStackSample(RegisterState* > > > > > > state, ...)' rather than the current one 'void > > > Isolate::GetStackSample(const > > > > > > RegisterState& state, ...)', because the const indicates that state > > won't > > > be > > > > > > changed. > > > > > > > > > > why do you need the API function change the input parameter 'state'? > > > > > > > > We don't need to change it, we can just make a copy of 'state' and modify > it > > > if > > > > needed, but the const signature is confusing, since on simulator we are > not > > > > > > const signature is not related to whether the parameters is used or not. > > > > > > > using the register state provided by clients to get stack sample. > > > > > > That's fine if a parameter is not used under certain circumstances. > > > > So you are suggesting we don't change the signature, but still modify the > > register state when needed, do I understand correctly? > > Not modify, but pass another one if we're running under simulator. > So the API function gets the current CPU state and based on this data retrieves > JS stack, whether it's running native code or simulated. So the actual mode is > not visible to the embedder. s/pass/use/
Description was changed from ========== Make GetStackSample API support simulator Currently GetStackSample doesn't support simulator, thus sampler is aware of simulator, but since we are moving it out, it shouldn't have knowledge of simulator. This patch moves the logic using simulator to fill register state into GetStackSample, so that it supports simulator. BUG=v8:4956 LOG=n ========== to ========== Make Isolate::GetStackSample API support simulator Currently GetStackSample doesn't support simulator, thus sampler is aware of simulator, but since we are moving it out, it shouldn't have knowledge of simulator. This patch moves the logic using simulator accessible to Isolate::GetStackSample, so that it supports simulator. BUG=v8:4956 LOG=n ==========
I updated the CL as we discussed. PTAL.
https://codereview.chromium.org/1926863003/diff/20001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1926863003/diff/20001/src/api.cc#newcode7526 src/api.cc:7526: regs.pc = state.pc; nit: you don't need to do the copy for non-simulator case. https://codereview.chromium.org/1926863003/diff/20001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1926863003/diff/20001/src/profiler/sampler.cc... src/profiler/sampler.cc:425: if (state.sp == 0 || state.fp == 0) return; you can move this part into FillRegisters. https://codereview.chromium.org/1926863003/diff/20001/src/profiler/sampler.h File src/profiler/sampler.h (right): https://codereview.chromium.org/1926863003/diff/20001/src/profiler/sampler.h#... src/profiler/sampler.h:148: inline static bool FillRegisters(Isolate* isolate, v8::RegisterState* state) { Please don't put such a big function definition in .h You can leave it in sampler.cc
Thanks, I made a few changes. PTAL. https://codereview.chromium.org/1926863003/diff/20001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1926863003/diff/20001/src/api.cc#newcode7526 src/api.cc:7526: regs.pc = state.pc; On 2016/04/29 18:55:06, alph wrote: > nit: you don't need to do the copy for non-simulator case. Done. https://codereview.chromium.org/1926863003/diff/20001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1926863003/diff/20001/src/profiler/sampler.cc... src/profiler/sampler.cc:425: if (state.sp == 0 || state.fp == 0) return; On 2016/04/29 18:55:06, alph wrote: > you can move this part into FillRegisters. Done. https://codereview.chromium.org/1926863003/diff/20001/src/profiler/sampler.h File src/profiler/sampler.h (right): https://codereview.chromium.org/1926863003/diff/20001/src/profiler/sampler.h#... src/profiler/sampler.h:148: inline static bool FillRegisters(Isolate* isolate, v8::RegisterState* state) { On 2016/04/29 18:55:06, alph wrote: > Please don't put such a big function definition in .h > You can leave it in sampler.cc Done.
great! thanks. https://codereview.chromium.org/1926863003/diff/60001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1926863003/diff/60001/src/api.cc#newcode7534 src/api.cc:7534: i::TickSample::GetStackSample(isolate, state, i::TickSample::kSkipCEntryFrame, You don't have to copy this line twice. Just make: #else const RegisterState& regs = state; #endif i::TickSample::GetStackSample... https://codereview.chromium.org/1926863003/diff/60001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1926863003/diff/60001/src/profiler/sampler.cc... src/profiler/sampler.cc:340: if (!simulator->has_bad_pc()) { not sure how did it work before. let's just return false for all archs if has_bad_pc. Otherwise we'd just return an uninitialized pc and it's impossible to detect in the client. https://codereview.chromium.org/1926863003/diff/60001/src/profiler/sampler.cc... src/profiler/sampler.cc:343: state->sp = reinterpret_cast<Address>(simulator->get_register( nit: should fit one line. run git cl format. https://codereview.chromium.org/1926863003/diff/60001/src/profiler/sampler.cc... src/profiler/sampler.cc:388: if (state->sp == 0 || state->fp == 0) return false; this seems to be already done at line 348. We probably want to do it for all archs, not just ARM64. as e.g. MIPS might have the same issue. https://codereview.chromium.org/1926863003/diff/60001/src/profiler/sampler.cc... src/profiler/sampler.cc:1020: if (!SimulatorHelper::FillRegisters(isolate(), &state)) return; you cannot return without resuming the thread.
great! thanks.
Thanks. Please take another look. https://codereview.chromium.org/1926863003/diff/60001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/1926863003/diff/60001/src/api.cc#newcode7534 src/api.cc:7534: i::TickSample::GetStackSample(isolate, state, i::TickSample::kSkipCEntryFrame, On 2016/04/29 20:59:57, alph wrote: > You don't have to copy this line twice. Just make: > > #else > const RegisterState& regs = state; > #endif > i::TickSample::GetStackSample... > Done. https://codereview.chromium.org/1926863003/diff/60001/src/profiler/sampler.cc File src/profiler/sampler.cc (right): https://codereview.chromium.org/1926863003/diff/60001/src/profiler/sampler.cc... src/profiler/sampler.cc:340: if (!simulator->has_bad_pc()) { On 2016/04/29 20:59:57, alph wrote: > not sure how did it work before. let's just return false for all archs if > has_bad_pc. Otherwise we'd just return an uninitialized pc and it's impossible > to detect in the client. an uninitialized register state has a pc value 0, let's keep it unless we know for sure that it is correct if we return false for all archs. https://codereview.chromium.org/1926863003/diff/60001/src/profiler/sampler.cc... src/profiler/sampler.cc:343: state->sp = reinterpret_cast<Address>(simulator->get_register( On 2016/04/29 20:59:57, alph wrote: > nit: should fit one line. run git cl format. Done. https://codereview.chromium.org/1926863003/diff/60001/src/profiler/sampler.cc... src/profiler/sampler.cc:388: if (state->sp == 0 || state->fp == 0) return false; On 2016/04/29 20:59:57, alph wrote: > this seems to be already done at line 348. We probably want to do it for all > archs, not just ARM64. as e.g. MIPS might have the same issue. Yes, line 348 is for ARM64 only, let's delete the one above instead. https://codereview.chromium.org/1926863003/diff/60001/src/profiler/sampler.cc... src/profiler/sampler.cc:1020: if (!SimulatorHelper::FillRegisters(isolate(), &state)) return; On 2016/04/29 20:59:57, alph wrote: > you cannot return without resuming the thread. Done.
lgtm https://codereview.chromium.org/1926863003/diff/80001/src/profiler/sampler.h File src/profiler/sampler.h (right): https://codereview.chromium.org/1926863003/diff/80001/src/profiler/sampler.h#... src/profiler/sampler.h:14: #include "src/simulator.h" leave it in sampler.cc https://codereview.chromium.org/1926863003/diff/80001/src/profiler/sampler.h#... src/profiler/sampler.h:148: // Returns false when there is no active simulator, or sp is zero, nit: you don't need to go into implementation details here. "Returns true if register values were successfully retrieved from the simulator, false otherwise." -- should be enough.
Thanks for the review. jochen@, PTAL. https://codereview.chromium.org/1926863003/diff/80001/src/profiler/sampler.h File src/profiler/sampler.h (right): https://codereview.chromium.org/1926863003/diff/80001/src/profiler/sampler.h#... src/profiler/sampler.h:14: #include "src/simulator.h" On 2016/04/29 22:07:33, alph wrote: > leave it in sampler.cc Done. https://codereview.chromium.org/1926863003/diff/80001/src/profiler/sampler.h#... src/profiler/sampler.h:148: // Returns false when there is no active simulator, or sp is zero, On 2016/04/29 22:07:33, alph wrote: > nit: you don't need to go into implementation details here. > "Returns true if register values were successfully retrieved from the simulator, > false otherwise." -- should be enough. 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/1926863003/#ps100001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1926863003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1926863003/100001
On 2016/05/02 16:18:16, jochen wrote: > lgtm Thanks.
Message was sent while issue was closed.
Description was changed from ========== Make Isolate::GetStackSample API support simulator Currently GetStackSample doesn't support simulator, thus sampler is aware of simulator, but since we are moving it out, it shouldn't have knowledge of simulator. This patch moves the logic using simulator accessible to Isolate::GetStackSample, so that it supports simulator. BUG=v8:4956 LOG=n ========== to ========== Make Isolate::GetStackSample API support simulator Currently GetStackSample doesn't support simulator, thus sampler is aware of simulator, but since we are moving it out, it shouldn't have knowledge of simulator. This patch moves the logic using simulator accessible to Isolate::GetStackSample, so that it supports simulator. BUG=v8:4956 LOG=n ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Make Isolate::GetStackSample API support simulator Currently GetStackSample doesn't support simulator, thus sampler is aware of simulator, but since we are moving it out, it shouldn't have knowledge of simulator. This patch moves the logic using simulator accessible to Isolate::GetStackSample, so that it supports simulator. BUG=v8:4956 LOG=n ========== to ========== Make Isolate::GetStackSample API support simulator Currently GetStackSample doesn't support simulator, thus sampler is aware of simulator, but since we are moving it out, it shouldn't have knowledge of simulator. This patch moves the logic using simulator accessible to Isolate::GetStackSample, so that it supports simulator. BUG=v8:4956 LOG=n Committed: https://crrev.com/b027b623dff557dc88b70a7cbe422a8c56b27164 Cr-Commit-Position: refs/heads/master@{#35944} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b027b623dff557dc88b70a7cbe422a8c56b27164 Cr-Commit-Position: refs/heads/master@{#35944} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
