I like this change very much as it will make a good foundation for both attributing the cost in IC's etc. to a JS function and locating cost of different call paths.
I think you should move the walking of the stack call into Ticker::Tick instead of having it in platform. That way you should be able to avoid copying the stack when calling Profiler::Insert. Maybe a second parameter to Profiler::Insert where an allocated array can be passed. Of cause with a dept of two thats not an issue but I don't like having the copy constructor for TickSample. For now just having a fixed size stack in TickSample might be sufficient. The code in platform-all.cc should then move to log.cc. When the stack walking code becomes architecture dependent we can split it in log-ia32.cc and log-arm.cc. The stack walking is going to be architecture dependent and not platform dependent. http://codereview.chromium.org/21403/diff/1/4 File src/platform-all.cc (right): http://codereview.chromium.org/21403/diff/1/4#newcode65 Line 65: // than 10,000 butes are bogus I don't think the constant 10000 is valid for our frames, as using Function.prototype.apply can create large frames. Try the following in the shell with --allow-natives-syntax (%DebugPrint prints the frame pointer) function f() { DebugPrint('f ' + arguments.length) } function g(x) { var a=new Array(x); DebugPrint('g'); f.apply(null, a); } g(100000) I think we just need to store a low-water-mart when V8 is initialized. http://codereview.chromium.org/21403/diff/1/5 File src/platform-freebsd.cc (right): http://codereview.chromium.org/21403/diff/1/5#newcode641 Line 641: unsigned int ebp = mcontext.mc_ebp; Maybe just add the frame pointer to the TickSample class? http://codereview.chromium.org/21403/diff/1/5#newcode644 Line 644: StackWalker::SampleStack(sample, ebp); Move stack walking to Ticker::Tick. http://codereview.chromium.org/21403/diff/1/6 File src/platform-linux.cc (right): http://codereview.chromium.org/21403/diff/1/6#newcode624 Line 624: unsigned int ebp = mcontext.gregs[REG_EBP]; Maybe just add the frame pointer to the TickSample class? http://codereview.chromium.org/21403/diff/1/6#newcode627 Line 627: StackWalker::SampleStack(sample, ebp); Move stack walking to Ticker::Tick. http://codereview.chromium.org/21403/diff/1/7 File src/platform-macos.cc (right): http://codereview.chromium.org/21403/diff/1/7#newcode590 Line 590: unsigned int ebp = mcontext->ss.ebp; Maybe just add the frame pointer to the TickSample class? http://codereview.chromium.org/21403/diff/1/7#newcode593 Line 593: StackWalker::SampleStack(sample, ebp); Move stack walking to Ticker::Tick. http://codereview.chromium.org/21403/diff/1/8 File src/platform-win32.cc (right): http://codereview.chromium.org/21403/diff/1/8#newcode1589 Line 1589: StackWalker::SampleStack(sample, context.Ebp); Move stack walking to Ticker::Tick. http://codereview.chromium.org/21403/diff/1/8#newcode1589 Line 1589: StackWalker::SampleStack(sample, context.Ebp); Maybe just add the frame pointer to the TickSample class? http://codereview.chromium.org/21403 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
