Thanks! OK, I will migrate to using Addresses in TickSampler.
http://codereview.chromium.org/50052/diff/1/6 File src/frames.cc (right): http://codereview.chromium.org/50052/diff/1/6#newcode86 Line 86: if (use_top || fp != NULL) { On 2009/03/20 13:31:33, Søren Gjesse wrote: > Isn't fp != NULL sufficient? No. It is possible to have a case when neither Top, nor registers contain valid pointers. Then we do nothing. http://codereview.chromium.org/50052/diff/1/6#newcode208 Line 208: StackFrame* frame = iterator_.frame(); On 2009/03/20 13:31:33, Søren Gjesse wrote: > Maybe call this variable last_frame and prev_frame for frame. Done. http://codereview.chromium.org/50052/diff/1/6#newcode222 Line 222: bool SafeStackFrameIterator::IsValidFrame(StackFrame* frame) const { On 2009/03/20 13:31:33, Søren Gjesse wrote: > Add a comment on this check (JavaScript frame needs a valid function whereas > other frames does not) Done. http://codereview.chromium.org/50052/diff/1/6#newcode348 Line 348: (void)GetCallerState(state); On 2009/03/20 13:31:33, Søren Gjesse wrote: > Is the (void) cast required? No. Just to indicate that the result is thrown away. Ok, removed. http://codereview.chromium.org/50052/diff/1/7 File src/frames.h (right): http://codereview.chromium.org/50052/diff/1/7#newcode422 Line 422: inline Object* function_no_check() const; On 2009/03/20 13:31:33, Søren Gjesse wrote: > How about naming this function_slot_object instead? Done. http://codereview.chromium.org/50052/diff/1/7#newcode523 Line 523: explicit StackFrameIterator(bool use_top, Address fp, Address sp); On 2009/03/20 13:31:33, Søren Gjesse wrote: > explicit not needed anymore. Done. http://codereview.chromium.org/50052/diff/1/7#newcode545 Line 545: void (StackFrameIterator::*advance_)(void); On 2009/03/20 13:31:33, Søren Gjesse wrote: > (void) -> () Done. http://codereview.chromium.org/50052/diff/1/7#newcode621 Line 621: Address low_bound, Address high_bound); On 2009/03/20 13:31:33, Søren Gjesse wrote: > explicit not required. Done. http://codereview.chromium.org/50052/diff/1/8 File src/log.cc (right): http://codereview.chromium.org/50052/diff/1/8#newcode928 Line 928: msg.Append(",0x%x", reinterpret_cast<int>(sample->stack[i])); On 2009/03/20 13:31:33, Søren Gjesse wrote: > Change int to uint32_t. Btw. why does %p now work anymore? Done. Under Windows %p doesn't produce the "0x" prefix. http://codereview.chromium.org/50052/diff/1/10 File src/platform-win32.cc (right): http://codereview.chromium.org/50052/diff/1/10#newcode1763 Line 1763: if (sampler_->IsProfiling()) { On 2009/03/20 13:31:33, Søren Gjesse wrote: > Good move! Yes. Otherwise we are sampling the moving stack ;) http://codereview.chromium.org/50052/diff/1/11 File test/cctest/test-log-ia32.cc (right): http://codereview.chromium.org/50052/diff/1/11#newcode4 Line 4: On 2009/03/20 13:31:33, Søren Gjesse wrote: > How about changing unsigned int to Address where it is an address in this file? > Then use casting in the use of CHECK_GE macros, or add a CHECK_ADDR_GE macros. > In test-debug.cc I have added helpers for the CHECK_EQ macros for Address, but > these will not help as you need CHECK_GE unless we change these to call helpers > as well, which might be overkill for this. Let's make this change together with using Address in TickSample. http://codereview.chromium.org/50052/diff/1/11#newcode34 Line 34: StackTracer* tracer; On 2009/03/20 13:31:33, Søren Gjesse wrote: > Initialize tracer and sampler to NULL. Done. http://codereview.chromium.org/50052/diff/1/11#newcode49 Line 49: reinterpret_cast<unsigned int>(trace_env.sample) - 10240; On 2009/03/20 13:31:33, Søren Gjesse wrote: > Why is 100 not enough any more? The main change here is to use Sample's address as a base (remember, Sample's address is used as the lower bound), because FP can contain garbage. 100 then becomes insufficient. http://codereview.chromium.org/50052/diff/1/11#newcode210 Line 210: On 2009/03/20 13:31:33, Søren Gjesse wrote: > Please describe this function (e.g. create a JS function which calls another JS > function with the current fp value). Done. http://codereview.chromium.org/50052 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
