Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1694011:
Adds C++ API for retrieving a stack trace without running JavaScript (Closed)
|
Created:
10 years, 8 months ago by jaimeyap Modified:
9 years, 7 months ago CC:
v8-dev, Kelly Norton Visibility:
Public. |
DescriptionAdds C++ API for retrieving a stack trace without invoking JavaScript. This is important for browser profiling since grabbing a stack trace via the API in messages.js and debug-debugger.js frequently allows GC to run. This API can still allow GC to run, but only rarely in situations where the heap allocator is unable to service an allocation request.
This matters alot currently for the profiling infrastructure in InspectorTimelineAgent in WebCore, which is used by the WebKit inspector's timeline panel, and Google Speed Tracer.
This API is extensible, and parameterized with flags so that callers can specify what subset of information they want to capture for each stack frame.
Regarding duplication of code. The API in top.cc and messages.js is aimed mainly at string printing Stack frames, and not at exposing API for programatically reading information about the stack. Also, the API available in debug-debugger.js and mirror-debugger.js are aimed at mainly supporting the interactive debugging use case, and are actually quite slow.
I think I could possible port some of the code in messages.js to use this new StackTrace API, but I would preferrably do that in a follow up patch.
Patch Set 1 #
Total comments: 3
Patch Set 2 : '' #
Total comments: 14
Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 49
Patch Set 7 : '' #Patch Set 8 : '' #
Total comments: 22
Patch Set 9 : '' #
Messages
Total messages: 19 (0 generated)
Requesting feedback. Will cover with appropriate test cases once consensus is reached on what the API should look like.
On 2010/04/22 18:17:54, jaimeyap wrote:
> Requesting feedback. Will cover with appropriate test cases once consensus is
> reached on what the API should look like.
First of all it is fine to have an ability to get stack trace information from
the C++ API. Maybe it should be in the normal API instead of in the debug API.
As far as I can see the basic stack traversal is the same as is used in
Runtime_CollectStackTrace, but instead of just storing relevant information from
the frame some calculations are done along the way. The main purpose of
Runtime_CollectStackTrace is to be fast at collecting data as the information
will most likely be discarded.
Also you new API will not avoid garbage collection as is allocates new objects
and calls functions which might allocate objects as well.
I don't think you should mention any specific numbers on the speed of one API
over another. This may change over time, and each API provides different levels
of information.
From the API point of wiev I think the information returned should be presented
as a C++ objects and not just an array of arrays. Internally using an array is
fine, but I don't think it should be exposed. I think you should be able to
create objects which "wraps" the arrays generated just like the API objects
"wraps" internal handles. E.g. the class Message is an interface to an internal
JS Object, and likewise class StackTrace and class StackFrame could be
interfaces to a FixedArray (no need for a JS Array then).
Local<v8::StackTrace> GetStackTrace(<flags to indicate level of information>)
class StackTrace {
int GetFrameCount();
Local<v8::StackFrame> GetFrame(int index);
}
class StackFrame {
...
}
I also think that it should be possible to decide what information to get hold
of using some kind of flags, so that we are not ending up with a fixed API which
can only return two strings and two numbers for each frame.
Of cause patches to improve performance of the other ways of working with the
stack are always welcome.
Jaime, what usage scenario are you targeting on? The function you wrote captures the current stack, so to capture running JS code, this function needs to be called from inside it. How? And you can't call this function from another thread, without stopping the V8 thread. http://codereview.chromium.org/1694011/diff/1/3 File src/api.cc (right): http://codereview.chromium.org/1694011/diff/1/3#newcode4028 src/api.cc:4028: inline int LineFromPosition(int position, Please look at GetScriptLineNUmber in handles.cc. http://codereview.chromium.org/1694011/diff/1/3#newcode4098 src/api.cc:4098: i::Handle<i::JSArray> stackFrame = i::Factory::NewJSArray(4); Why JSArray, not FixedArray? http://codereview.chromium.org/1694011/diff/1/3#newcode4100 src/api.cc:4100: elements->set(0, script_name); I'm not sure it is OK to return internal handle types. I think you need to convert them.
On 2010/04/23 08:30:56, Søren Gjesse wrote:
> On 2010/04/22 18:17:54, jaimeyap wrote:
> > Requesting feedback. Will cover with appropriate test cases once consensus
is
> > reached on what the API should look like.
>
Thanks for the feedback. This is my first V8 patch so forgive me if I don't have
a full grasp of the system :).
> First of all it is fine to have an ability to get stack trace information from
> the C++ API. Maybe it should be in the normal API instead of in the debug API.
>
Sounds good.
> As far as I can see the basic stack traversal is the same as is used in
> Runtime_CollectStackTrace, but instead of just storing relevant information
from
> the frame some calculations are done along the way. The main purpose of
> Runtime_CollectStackTrace is to be fast at collecting data as the information
> will most likely be discarded.
>
Yes, however it stops resolving when grabbing the PC. Meaning in order to
resolve the stack trace, you need to do a second full walk of all the frames
after the fact. Probably not a big deal in practice, but definitely non-optimal.
Also, is Runtime_CollectStackTrace available via some C++ header? I was under
the impression that is was purely C++/JS glue functionality.
> Also you new API will not avoid garbage collection as is allocates new objects
> and calls functions which might allocate objects as well.
>
I was not aware that GC could run during object allocation. I was under the
impression that GC would only run when invoking/running JavaScript. I see no
calls to CollectGarbage or related functions anywhere along the allocation stack
in factory.cc or heap.cc for allocation of JSArrays. I also did not observe any
GC runs during my tests of this new function (tested grabbing stack traces for
almost every cross from JS into the DOM bindings when running Gmail and Wave).
Ofcourse, I am probably just getting lost somewhere following the macros :).
> I don't think you should mention any specific numbers on the speed of one API
> over another. This may change over time, and each API provides different
levels
> of information.
>
Makes sense. I just wanted some feedback. The comments will most likely be
streamlined after figuring out what the API should look like.
> From the API point of wiev I think the information returned should be
presented
> as a C++ objects and not just an array of arrays. Internally using an array is
> fine, but I don't think it should be exposed. I think you should be able to
> create objects which "wraps" the arrays generated just like the API objects
> "wraps" internal handles. E.g. the class Message is an interface to an
internal
> JS Object, and likewise class StackTrace and class StackFrame could be
> interfaces to a FixedArray (no need for a JS Array then).
>
> Local<v8::StackTrace> GetStackTrace(<flags to indicate level of information>)
>
> class StackTrace {
> int GetFrameCount();
> Local<v8::StackFrame> GetFrame(int index);
> }
>
> class StackFrame {
> ...
> }
>
This makes sense. I thought I was following precedent of other methods in
v8-debug.h that returned Handles to Value and Context objects. But a C++ API
would be a lot more flexible for the general use case.
> I also think that it should be possible to decide what information to get hold
> of using some kind of flags, so that we are not ending up with a fixed API
which
> can only return two strings and two numbers for each frame.
>
Totally in agreement :). I was hoping to do this iteratively so that I could
satisfy WebKit's usecase and then build a more general API. But it should not be
that much more code to get most of the way the first time around. I think I will
try wrapping things up in C++ API in my second iteration.
> Of cause patches to improve performance of the other ways of working with the
> stack are always welcome.
I think they are limited fundamentally. I first attempted to speed up the other
ways of grabbing stack traces. The current mechanisms provide clean JS API. But
this API crosses boundaries between JS and C++ frequently, and allocates lots of
wrapper JS objects. I tried to make them faster, but was unable to without
building a separate API all together. I got some wins with a separate JS API,
but then ran into the problem of GC running willy nilly which was a complete
non-starter. As a reference, my "faster" JS API for grabbing stack traces was
about 5x faster than the current mechanism. The native API in this patch is
about 150x faster than my "faster" JS API and does not seem to trigger GC.
On 2010/04/23 08:33:49, Michail Naganov wrote: > Jaime, what usage scenario are you targeting on? The function you wrote captures > the current stack, so to capture running JS code, this function needs to be > called from inside it. How? And you can't call this function from another > thread, without stopping the V8 thread. > To clarify, this API is intended to satisfy the use case of the InspectorTimelineAgent functionality in WebCore which feeds both the timelinepanel in the WebKit Inspector, and Google Speed Tracer. This infrastructure measures time spent in various portions of the browser code. Things like Parsing HTML, running JavaScript, performing layout, etc... This builds a tree structure that we serialize to a JavaScript object and send over to tools like the webkit inspector, or Speed Tracer. Essentially, whenever JavaScript does something in the DOM bindings layer that causes (synchronously) HTML to be parsed, Layout to run, or Styles to be rematched, the InspectorTimelineAgent grabs information about the top call frame (using the debug JS api) to see what JavaScript triggered it. This is very useful for web developers who want to write efficient web apps. The problem currently is that: 1. Even only grabbing information about the top stack frame is very slow using the current JS api. 2. Modern web apps use libraries to write their code. Thus the vast majority of the time the top stack frame is general library code and useless to the developer. More stack frames are needed. 3. Frequently allowing GC to run during things like layout or style rematching is totally unacceptable for a browser profiler. This is not just observer effect. This is an incorrect representation of what would happen in the browser during normal execution of the application. GC would never normally run during something like style rematching. > http://codereview.chromium.org/1694011/diff/1/3 > File src/api.cc (right): > > http://codereview.chromium.org/1694011/diff/1/3#newcode4028 > src/api.cc:4028: inline int LineFromPosition(int position, > Please look at GetScriptLineNUmber in handles.cc. > Thanks. Will take a look. > http://codereview.chromium.org/1694011/diff/1/3#newcode4098 > src/api.cc:4098: i::Handle<i::JSArray> stackFrame = i::Factory::NewJSArray(4); > Why JSArray, not FixedArray? > No reason. Was just following precedent from other parts of the code. I should just change to a fixedArray. > http://codereview.chromium.org/1694011/diff/1/3#newcode4100 > src/api.cc:4100: elements->set(0, script_name); > I'm not sure it is OK to return internal handle types. I think you need to > convert them. I'm not sure I follow. I return an external handle type from this function at the end. It is a 2D v8::Array, so the caller never actually deals with an internal Handle.
Before nailing things down with unit tests, I just wanted you guys to take a second look to see if this looks good API wise. Regarding duplication of code. The existing Stack Trace code in top.cc,runtime.cc and messages.js is aimed mainly at string printing Stack frames, and not at exposing API for programatically reading information about the stack. The API available in debug-debugger.js and mirror-debugger.js are aimed at mainly supporting the interactive debugging use case, and are actually quite slow. I think I could possible port some of the code in messages.js to use this new StackTrace API, but I would preferrably do that in a follow up patch. Also, I think they are different enough that the code duplication isn't so bad. http://codereview.chromium.org/1694011/diff/9001/10002 File include/v8.h (right): http://codereview.chromium.org/1694011/diff/9001/10002#newcode755 include/v8.h:755: static_cast<FrameOptions>(LINE_NUMBER | COLUMN_OFFSET | SCRIPT_NAME | FUNCTION_NAME); line length addressed locally. http://codereview.chromium.org/1694011/diff/9001/10002#newcode2784 include/v8.h:2784: * property is present an empty handle is returned. space removed locally.
http://codereview.chromium.org/1694011/diff/9001/10002 File include/v8.h (right): http://codereview.chromium.org/1694011/diff/9001/10002#newcode772 include/v8.h:772: * This method will return 0 if it is unable to retrieve the line number, It's better to use a named constant for the "unable to retrieve" case. E.g. look at the "Function" class. http://codereview.chromium.org/1694011/diff/9001/10002#newcode781 include/v8.h:781: * This method will return 0 if it is unable to retrieve the line number, The same here. http://codereview.chromium.org/1694011/diff/9001/10003 File src/top.cc (right): http://codereview.chromium.org/1694011/diff/9001/10003#newcode380 src/top.cc:380: Handle<JSArray> stackTrace = Factory::NewJSArray(frame_limit); JSArray can be a bit slow because of dynamic expansion support. An alternative approach is to count frames number first, then allocate a fixed array. http://codereview.chromium.org/1694011/diff/9001/10003#newcode408 src/top.cc:408: CStrVector(kColumnKeyData)); You could pre-allocate those strings as symbols (see SYMBOL_LIST in heap.h) to avoid creating them every time. Or, at least, move their allocation out of the loop.
Patch updated according to Michail's comments. Working on unit tests now. http://codereview.chromium.org/1694011/diff/9001/10002 File include/v8.h (right): http://codereview.chromium.org/1694011/diff/9001/10002#newcode772 include/v8.h:772: * This method will return 0 if it is unable to retrieve the line number, On 2010/04/30 10:01:22, Michail Naganov wrote: > It's better to use a named constant for the "unable to retrieve" case. E.g. look > at the "Function" class. Done. http://codereview.chromium.org/1694011/diff/9001/10002#newcode781 include/v8.h:781: * This method will return 0 if it is unable to retrieve the line number, On 2010/04/30 10:01:22, Michail Naganov wrote: > The same here. Done. http://codereview.chromium.org/1694011/diff/9001/10003 File src/top.cc (right): http://codereview.chromium.org/1694011/diff/9001/10003#newcode380 src/top.cc:380: Handle<JSArray> stackTrace = Factory::NewJSArray(frame_limit); On 2010/04/30 10:01:22, Michail Naganov wrote: > JSArray can be a bit slow because of dynamic expansion support. An alternative > approach is to count frames number first, then allocate a fixed array. I thought the dynamic expansion code path will only trigger if I exceed the initial size that I create it with. I pass in frame_limit which is guaranteed to be >= frames_seen. The reason why I use JSArray is that I want to be able to easily return the StackTrace as a v8::Array for external consumption in JavaScript. Also, If you notice, I only add elements to the JSArray via its elements(), which is a FixedArray :). http://codereview.chromium.org/1694011/diff/9001/10003#newcode408 src/top.cc:408: CStrVector(kColumnKeyData)); On 2010/04/30 10:01:22, Michail Naganov wrote: > You could pre-allocate those strings as symbols (see SYMBOL_LIST in heap.h) to > avoid creating them every time. Or, at least, move their allocation out of the > loop. I will move them out of the loop. Should I guard each key symbol allocation with a flag guard? I think allocating them all should be fine since there are not very many. The cost of the extra branches might outweigh the allocation cost.
http://codereview.chromium.org/1694011/diff/9001/10003 File src/top.cc (right): http://codereview.chromium.org/1694011/diff/9001/10003#newcode380 src/top.cc:380: Handle<JSArray> stackTrace = Factory::NewJSArray(frame_limit); On 2010/04/30 17:43:41, jaimeyap wrote: > On 2010/04/30 10:01:22, Michail Naganov wrote: > > JSArray can be a bit slow because of dynamic expansion support. An alternative > > approach is to count frames number first, then allocate a fixed array. > > I thought the dynamic expansion code path will only trigger if I exceed the > initial size that I create it with. > > I pass in frame_limit which is guaranteed to be >= frames_seen. > > The reason why I use JSArray is that I want to be able to easily return the > StackTrace as a v8::Array for external consumption in JavaScript. Also, If you > notice, I only add elements to the JSArray via its elements(), which is a > FixedArray :). > This looks a bit fishy to me, because elements can also be of type NumberDictionary (see JSObject::NormalizeElements). Although, now NormalizeElements isn't invoked in your case, someone could change the code in the future to do this. What V8 experts say about this? http://codereview.chromium.org/1694011/diff/9001/10003#newcode408 src/top.cc:408: CStrVector(kColumnKeyData)); On 2010/04/30 17:43:41, jaimeyap wrote: > On 2010/04/30 10:01:22, Michail Naganov wrote: > > You could pre-allocate those strings as symbols (see SYMBOL_LIST in heap.h) to > > avoid creating them every time. Or, at least, move their allocation out of the > > loop. > > I will move them out of the loop. > > Should I guard each key symbol allocation with a flag guard? I think allocating > them all should be fine since there are not very many. The cost of the extra > branches might outweigh the allocation cost. I would try to measure execution time of both cases to know for sure. BTW, another bonus of making them pre-allocated symbols, is that in this case they would be simply loaded from a snapshot, so allocation cost will be zero.
Adding tests and fixing lint errors. http://codereview.chromium.org/1694011/diff/9001/10003 File src/top.cc (right): http://codereview.chromium.org/1694011/diff/9001/10003#newcode380 src/top.cc:380: Handle<JSArray> stackTrace = Factory::NewJSArray(frame_limit); On 2010/04/30 21:29:19, Michail Naganov wrote: > On 2010/04/30 17:43:41, jaimeyap wrote: > > On 2010/04/30 10:01:22, Michail Naganov wrote: > > > JSArray can be a bit slow because of dynamic expansion support. An > alternative > > > approach is to count frames number first, then allocate a fixed array. > > > > I thought the dynamic expansion code path will only trigger if I exceed the > > initial size that I create it with. > > > > I pass in frame_limit which is guaranteed to be >= frames_seen. > > > > The reason why I use JSArray is that I want to be able to easily return the > > StackTrace as a v8::Array for external consumption in JavaScript. Also, If > you > > notice, I only add elements to the JSArray via its elements(), which is a > > FixedArray :). > > > > This looks a bit fishy to me, because elements can also be of type > NumberDictionary (see JSObject::NormalizeElements). Although, now > NormalizeElements isn't invoked in your case, someone could change the code in > the future to do this. What V8 experts say about this? There is precedent for this. See Runtime_CollectStackTrace in runtime.cc. I am pretty sure that JSArray will default to fast mode (backed by FixedArray) so long as it is used in the method I am using it. But I agree. V8 experts please chime in. http://codereview.chromium.org/1694011/diff/9001/10003#newcode408 src/top.cc:408: CStrVector(kColumnKeyData)); On 2010/04/30 21:29:19, Michail Naganov wrote: > On 2010/04/30 17:43:41, jaimeyap wrote: > > On 2010/04/30 10:01:22, Michail Naganov wrote: > > > You could pre-allocate those strings as symbols (see SYMBOL_LIST in heap.h) > to > > > avoid creating them every time. Or, at least, move their allocation out of > the > > > loop. > > > > I will move them out of the loop. > > > > Should I guard each key symbol allocation with a flag guard? I think > allocating > > them all should be fine since there are not very many. The cost of the extra > > branches might outweigh the allocation cost. > > I would try to measure execution time of both cases to know for sure. > > BTW, another bonus of making them pre-allocated symbols, is that in this case > they would be simply loaded from a snapshot, so allocation cost will be zero. I can work on moving these to be pre-allocated. But when I compare the time for this function to the time for using a 2D fixed array (see patchset 1), they are very close for real world usecases, and the 2D array case doesn't require allocating any symbols. I don't think performance is much of an issue with this implementation. I think the majority of the time will be spent in extracting the stack information and storing it in the dictionary mode JSObject for the frame, not in allocating the key symbols. I think I could circle around in the future to move these to pre-allocated symbols if performance is still an issue with this API.
Please take a look at the stack traces used for stack traces in JavaScript exceptions (Runtime_CollectStackTrace in runtime.cc and the related code in messages.js) so that we might use this methid for collecting stack traces in there as well. http://codereview.chromium.org/1694011/diff/35007/38002 File include/v8.h (right): http://codereview.chromium.org/1694011/diff/35007/38002#newcode710 include/v8.h:710: enum FrameOptions { Rename FrameOptions to StackTraceOptions. Then naming of enum values in v8.h is quite a mess. However according to http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumer... the preferred naming convention is the constant-like one kXXX, so we should use that going forward. http://codereview.chromium.org/1694011/diff/35007/38002#newcode711 include/v8.h:711: LINE_NUMBER = 0x0001, Please use shift to generate these flags, kLineNumber = 1 << 1, http://codereview.chromium.org/1694011/diff/35007/38002#newcode712 include/v8.h:712: COLUMN_OFFSET = 0x0003, // COLUMN_OFFSET implicitly includes LINE_NUMBER. Use already defined constants kColumn = 1 << 2 | kLine, Just use the name column, as that is what is used in the class Message. http://codereview.chromium.org/1694011/diff/35007/38002#newcode717 include/v8.h:717: // TODO(jaimeyap): Add ability to lookup the method name, No TODOs with name. Eiter remove it or create a bug and use the bug number. http://codereview.chromium.org/1694011/diff/35007/38002#newcode719 include/v8.h:719: // and function arguements. Please put the conbined flags here kOverview = kSourceLine | kSourceColumn | ... http://codereview.chromium.org/1694011/diff/35007/38002#newcode733 include/v8.h:733: * Returns StackTrace as a v8::Array. Please extend the comment with information that the array contain StackFrame objects. http://codereview.chromium.org/1694011/diff/35007/38002#newcode737 include/v8.h:737: /** I don't think this note is required. This is a prerequisite for most function in the API. http://codereview.chromium.org/1694011/diff/35007/38002#newcode751 include/v8.h:751: static const FrameOptions OVERVIEW = Please move this to the enum definition. http://codereview.chromium.org/1694011/diff/35007/38002#newcode758 include/v8.h:758: static const FrameOptions DETAILED = Ditto. http://codereview.chromium.org/1694011/diff/35007/38002#newcode779 include/v8.h:779: * This method will return kNotFound if it is unable to retrieve the column kNotFound -> Message::kNoLineNumberInfo http://codereview.chromium.org/1694011/diff/35007/38002#newcode808 include/v8.h:808: static const int kNotFound; In the profiler API this is called CpuProfileNode::kNoLineNumberInfo and the class Message can also return a line number. We should only have one constant for this, and I suggest you place it in the class Message and call it kNoLineNumberInfo. Then update the comment in class Message and make sure a consistent value is returned (either 0 or -1 - I am not sure what message does). http://codereview.chromium.org/1694011/diff/35007/38001 File src/api.cc (right): http://codereview.chromium.org/1694011/diff/35007/38001#newcode1535 src/api.cc:1535: i::Handle<i::JSObject> obj(i::JSObject::cast(self->GetElement(index))); Please add a range check on index and return undefined if out of bounds. http://codereview.chromium.org/1694011/diff/35007/38003 File src/top.cc (right): http://codereview.chromium.org/1694011/diff/35007/38003#newcode367 src/top.cc:367: These constants are only used in a single place, so why not put the constants directly in the Factory::LookupAsciiSymbol calls below. http://codereview.chromium.org/1694011/diff/35007/38003#newcode368 src/top.cc:368: static const char* kLineKeyData = "line"; line -> lineNumber to match the name of the names in the API. http://codereview.chromium.org/1694011/diff/35007/38003#newcode376 src/top.cc:376: Local<StackTrace> Top::CaptureCurrentStackTrace(int frame_limit, Break line just after ( to have both arguments on the next line if the second argument cannot be aligned with the first. http://codereview.chromium.org/1694011/diff/35007/38003#newcode385 src/top.cc:385: Handle<String> column_key = Factory::NewStringFromAscii( Use Factory::LookupAsciiSymbol(kXXX) here. http://codereview.chromium.org/1694011/diff/35007/38003#newcode416 src/top.cc:416: if (line_number == script_line_offset) { Two spaces after ==. http://codereview.chromium.org/1694011/diff/35007/38003#newcode447 src/top.cc:447: Handle<String> eval_key = Factory::NewStringFromAscii( Use Factory::LookupAsciiSymbol(kXXX). Maybe move it up to where the other symbols are initialized. http://codereview.chromium.org/1694011/diff/35007/38003#newcode452 src/top.cc:452: if (options & StackTrace::IS_CONSTRUCTOR) { The frame has an IsConstructor() method, does that not give the expected result? http://codereview.chromium.org/1694011/diff/35007/38005 File src/top.h (right): http://codereview.chromium.org/1694011/diff/35007/38005#newcode269 src/top.h:269: // NOTE: You must enter a Context::Scope before calling this function! No need for this comment here. http://codereview.chromium.org/1694011/diff/35007/38006 File test/cctest/test-api.cc (right): http://codereview.chromium.org/1694011/diff/35007/38006#newcode9607 test/cctest/test-api.cc:9607: const char* origin = "capture-stack-trace-test"; koverview -> kOverview, kdetailed -> kDetailed http://codereview.chromium.org/1694011/diff/35007/38006#newcode9619 test/cctest/test-api.cc:9619: checkStackFrame(origin, "bar", 2, 3, false, false, frame0); Change frame0 -> stackTrace->GetFrame(0) and avoid the local variable? http://codereview.chromium.org/1694011/diff/35007/38006#newcode9661 test/cctest/test-api.cc:9661: "function bar() {\n" You can avoid dumplcating the test source by using AnalyzeStackInNativeCode(x), then set x from C++ code, and call a JavaScript function in the compiled code which does the "eval('new foo();')".
Thanks for the review. I will put up another patch set after we decide whether or not to pre-allocate the symbol keys so that I can use LookupAsciiSymbol. See my question/comment below for top.cc. http://codereview.chromium.org/1694011/diff/35007/38002 File include/v8.h (right): http://codereview.chromium.org/1694011/diff/35007/38002#newcode710 include/v8.h:710: enum FrameOptions { On 2010/05/03 07:59:21, Søren Gjesse wrote: > Rename FrameOptions to StackTraceOptions. > > Then naming of enum values in v8.h is quite a mess. However according to > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumer... > the preferred naming convention is the constant-like one kXXX, so we should use > that going forward. Done. http://codereview.chromium.org/1694011/diff/35007/38002#newcode711 include/v8.h:711: LINE_NUMBER = 0x0001, On 2010/05/03 07:59:21, Søren Gjesse wrote: > Please use shift to generate these flags, > > kLineNumber = 1 << 1, > Done. http://codereview.chromium.org/1694011/diff/35007/38002#newcode712 include/v8.h:712: COLUMN_OFFSET = 0x0003, // COLUMN_OFFSET implicitly includes LINE_NUMBER. On 2010/05/03 07:59:21, Søren Gjesse wrote: > Use already defined constants > > kColumn = 1 << 2 | kLine, > > Just use the name column, as that is what is used in the class Message. Done. http://codereview.chromium.org/1694011/diff/35007/38002#newcode717 include/v8.h:717: // TODO(jaimeyap): Add ability to lookup the method name, On 2010/05/03 07:59:21, Søren Gjesse wrote: > No TODOs with name. Eiter remove it or create a bug and use the bug number. I removed it. I will also create associated bugs after this patch goes in for adding the missing functionality and for porting the code in runtime.cc that is used by messages.js to use this API. http://codereview.chromium.org/1694011/diff/35007/38002#newcode719 include/v8.h:719: // and function arguements. On 2010/05/03 07:59:21, Søren Gjesse wrote: > Please put the conbined flags here > > kOverview = kSourceLine | kSourceColumn | ... Done. http://codereview.chromium.org/1694011/diff/35007/38002#newcode733 include/v8.h:733: * Returns StackTrace as a v8::Array. On 2010/05/03 07:59:21, Søren Gjesse wrote: > Please extend the comment with information that the array contain StackFrame > objects. Done. http://codereview.chromium.org/1694011/diff/35007/38002#newcode737 include/v8.h:737: /** On 2010/05/03 07:59:21, Søren Gjesse wrote: > I don't think this note is required. This is a prerequisite for most function in > the API. Done. http://codereview.chromium.org/1694011/diff/35007/38002#newcode751 include/v8.h:751: static const FrameOptions OVERVIEW = On 2010/05/03 07:59:21, Søren Gjesse wrote: > Please move this to the enum definition. Done. http://codereview.chromium.org/1694011/diff/35007/38002#newcode758 include/v8.h:758: static const FrameOptions DETAILED = On 2010/05/03 07:59:21, Søren Gjesse wrote: > Ditto. Done. http://codereview.chromium.org/1694011/diff/35007/38002#newcode808 include/v8.h:808: static const int kNotFound; On 2010/05/03 07:59:21, Søren Gjesse wrote: > In the profiler API this is called CpuProfileNode::kNoLineNumberInfo and the > class Message can also return a line number. We should only have one constant > for this, and I suggest you place it in the class Message and call it > kNoLineNumberInfo. Then update the comment in class Message and make sure a > consistent value is returned (either 0 or -1 - I am not sure what message does). I think Message returns -1. But CpuProfileNode and this new StackTrace API return 0. Given that the indexes are all 1-based, I think returning 0 makes most sense. I changed them all to use Message::kNoLineNumberOffset, which ==0. All unit tests pass, so if anyone was depending on this being -1 then they may be broken :(. http://codereview.chromium.org/1694011/diff/35007/38001 File src/api.cc (right): http://codereview.chromium.org/1694011/diff/35007/38001#newcode1535 src/api.cc:1535: i::Handle<i::JSObject> obj(i::JSObject::cast(self->GetElement(index))); On 2010/05/03 07:59:21, Søren Gjesse wrote: > Please add a range check on index and return undefined if out of bounds. Doesn't GetElement() already return undefined if index doesn't match the range? http://codereview.chromium.org/1694011/diff/35007/38003 File src/top.cc (right): http://codereview.chromium.org/1694011/diff/35007/38003#newcode367 src/top.cc:367: On 2010/05/03 07:59:21, Søren Gjesse wrote: > These constants are only used in a single place, so why not put the constants > directly in the Factory::LookupAsciiSymbol calls below. Done. http://codereview.chromium.org/1694011/diff/35007/38003#newcode368 src/top.cc:368: static const char* kLineKeyData = "line"; On 2010/05/03 07:59:21, Søren Gjesse wrote: > line -> lineNumber to match the name of the names in the API. Done. http://codereview.chromium.org/1694011/diff/35007/38003#newcode376 src/top.cc:376: Local<StackTrace> Top::CaptureCurrentStackTrace(int frame_limit, On 2010/05/03 07:59:21, Søren Gjesse wrote: > Break line just after ( to have both arguments on the next line if the second > argument cannot be aligned with the first. Done. http://codereview.chromium.org/1694011/diff/35007/38003#newcode385 src/top.cc:385: Handle<String> column_key = Factory::NewStringFromAscii( On 2010/05/03 07:59:21, Søren Gjesse wrote: > Use Factory::LookupAsciiSymbol(kXXX) here. Hmm. But in order to use LookupAsciiSymbol I would need to pre-allocate all the key symbols and add them to the roots_ array in heap.h. Do we want to always allocate these Symbols for the common case? Seems like it would be better to only pay the penalty when you grab a stack trace. If you still think I should do this, let me know and I will put these symbols there. http://codereview.chromium.org/1694011/diff/35007/38003#newcode416 src/top.cc:416: if (line_number == script_line_offset) { On 2010/05/03 07:59:21, Søren Gjesse wrote: > Two spaces after ==. Done. http://codereview.chromium.org/1694011/diff/35007/38003#newcode447 src/top.cc:447: Handle<String> eval_key = Factory::NewStringFromAscii( On 2010/05/03 07:59:21, Søren Gjesse wrote: > Use Factory::LookupAsciiSymbol(kXXX). Maybe move it up to where the other > symbols are initialized. Done. http://codereview.chromium.org/1694011/diff/35007/38003#newcode452 src/top.cc:452: if (options & StackTrace::IS_CONSTRUCTOR) { On 2010/05/03 07:59:21, Søren Gjesse wrote: > The frame has an IsConstructor() method, does that not give the expected result? It should. Changed to use that. http://codereview.chromium.org/1694011/diff/35007/38005 File src/top.h (right): http://codereview.chromium.org/1694011/diff/35007/38005#newcode269 src/top.h:269: // NOTE: You must enter a Context::Scope before calling this function! On 2010/05/03 07:59:21, Søren Gjesse wrote: > No need for this comment here. Done. http://codereview.chromium.org/1694011/diff/35007/38006 File test/cctest/test-api.cc (right): http://codereview.chromium.org/1694011/diff/35007/38006#newcode9607 test/cctest/test-api.cc:9607: const char* origin = "capture-stack-trace-test"; On 2010/05/03 07:59:21, Søren Gjesse wrote: > koverview -> kOverview, kdetailed -> kDetailed Done. http://codereview.chromium.org/1694011/diff/35007/38006#newcode9619 test/cctest/test-api.cc:9619: checkStackFrame(origin, "bar", 2, 3, false, false, frame0); On 2010/05/03 07:59:21, Søren Gjesse wrote: > Change frame0 -> stackTrace->GetFrame(0) and avoid the local variable? Done. http://codereview.chromium.org/1694011/diff/35007/38006#newcode9661 test/cctest/test-api.cc:9661: "function bar() {\n" On 2010/05/03 07:59:21, Søren Gjesse wrote: > You can avoid dumplcating the test source by using AnalyzeStackInNativeCode(x), > then set x from C++ code, and call a JavaScript function in the compiled code > which does the "eval('new foo();')". I also wanted to have slightly different call stacks to test differences in column offsets and suck. I will change the test code to more clearly reflect this.
As mentioned in the comment there is no need to "pre-allocate" symbols for strings used as property names. http://codereview.chromium.org/1694011/diff/35007/38001 File src/api.cc (right): http://codereview.chromium.org/1694011/diff/35007/38001#newcode1535 src/api.cc:1535: i::Handle<i::JSObject> obj(i::JSObject::cast(self->GetElement(index))); On 2010/05/03 19:16:02, jaimeyap wrote: > On 2010/05/03 07:59:21, Søren Gjesse wrote: > > Please add a range check on index and return undefined if out of bounds. > > Doesn't GetElement() already return undefined if index doesn't match the range? > Thats right. http://codereview.chromium.org/1694011/diff/35007/38003 File src/top.cc (right): http://codereview.chromium.org/1694011/diff/35007/38003#newcode385 src/top.cc:385: Handle<String> column_key = Factory::NewStringFromAscii( On 2010/05/03 19:16:02, jaimeyap wrote: > On 2010/05/03 07:59:21, Søren Gjesse wrote: > > Use Factory::LookupAsciiSymbol(kXXX) here. > > Hmm. But in order to use LookupAsciiSymbol I would need to pre-allocate all the > key symbols and add them to the roots_ array in heap.h. > > Do we want to always allocate these Symbols for the common case? Seems like it > would be better to only pay the penalty when you grab a stack trace. > > If you still think I should do this, let me know and I will put these symbols > there. Thats not right. Factory::LookupAsciiSymbol will create the symbol if it is not found in the symbol table already. Also when using the string as a property name a symbol with this string will be created anyway. Using a symbol to begin with then saves SetProperty from looking it up each time.
http://codereview.chromium.org/1694011/diff/35007/38002 File include/v8.h (right): http://codereview.chromium.org/1694011/diff/35007/38002#newcode808 include/v8.h:808: static const int kNotFound; On 2010/05/03 19:16:02, jaimeyap wrote: > On 2010/05/03 07:59:21, Søren Gjesse wrote: > > In the profiler API this is called CpuProfileNode::kNoLineNumberInfo and the > > class Message can also return a line number. We should only have one constant > > for this, and I suggest you place it in the class Message and call it > > kNoLineNumberInfo. Then update the comment in class Message and make sure a > > consistent value is returned (either 0 or -1 - I am not sure what message > does). > > I think Message returns -1. But CpuProfileNode and this new StackTrace API > return 0. > > Given that the indexes are all 1-based, I think returning 0 makes most sense. I > changed them all to use Message::kNoLineNumberOffset, which ==0. > > All unit tests pass, so if anyone was depending on this being -1 then they may > be broken :(. > As for the profiler API, I chose 0 because WebKit uses unsigned ints for reporting line numbers, so -1 was turning into 2^32 - 1, that isn't acceptable.
Patch updated and ready for review. Sorry for uploading so many patches, it seems my presubmit check wasn't running the linter :). http://codereview.chromium.org/1694011/diff/35007/38002 File include/v8.h (right): http://codereview.chromium.org/1694011/diff/35007/38002#newcode808 include/v8.h:808: static const int kNotFound; On 2010/05/04 09:59:18, Michail Naganov wrote: > On 2010/05/03 19:16:02, jaimeyap wrote: > > On 2010/05/03 07:59:21, Søren Gjesse wrote: > > > In the profiler API this is called CpuProfileNode::kNoLineNumberInfo and the > > > class Message can also return a line number. We should only have one > constant > > > for this, and I suggest you place it in the class Message and call it > > > kNoLineNumberInfo. Then update the comment in class Message and make sure a > > > consistent value is returned (either 0 or -1 - I am not sure what message > > does). > > > > I think Message returns -1. But CpuProfileNode and this new StackTrace API > > return 0. > > > > Given that the indexes are all 1-based, I think returning 0 makes most sense. > I > > changed them all to use Message::kNoLineNumberOffset, which ==0. > > > > All unit tests pass, so if anyone was depending on this being -1 then they may > > be broken :(. > > > > As for the profiler API, I chose 0 because WebKit uses unsigned ints for > reporting line numbers, so -1 was turning into 2^32 - 1, that isn't acceptable. I standardized on 0 as the notfound return value for Messages, StackTraces and CpuProfiler.
LGTM http://codereview.chromium.org/1694011/diff/55001/56002 File include/v8.h (right): http://codereview.chromium.org/1694011/diff/55001/56002#newcode697 include/v8.h:697: static const int kNoLineNumberInfo = 0; Please remove empty line. http://codereview.chromium.org/1694011/diff/55001/56002#newcode704 include/v8.h:704: * A JavaScript stack trace. This will only include JavaScript associated Please revise this comment into soething like this "Representation of a JavaScript stack trace. The information collected is a snapshot of the execution stack and the information remains valid after execution continues." I don't think we should mention stuff about V8's native runtime.js here as embedders should not worry about that. http://codereview.chromium.org/1694011/diff/55001/56002#newcode741 include/v8.h:741: * Grabs the current JavaScript StackTrace. Grabs the current JavaScript StackTrace. -> Grab a snapshot of the the current JavaScript execution stack. http://codereview.chromium.org/1694011/diff/55001/56002#newcode760 include/v8.h:760: * This method will return kNotFound if it is unable to retrieve the line kNotFound -> Message::kNoLineNumberInfo http://codereview.chromium.org/1694011/diff/55001/56002#newcode761 include/v8.h:761: * number, or if LINE_NUMBER was not passed as an option when capturing the LINE_NUMBER -> kLineNumber http://codereview.chromium.org/1694011/diff/55001/56002#newcode769 include/v8.h:769: * This method will return kNotFound if it is unable to retrieve the column kNotFound -> Message::kNoLineNumberInfo http://codereview.chromium.org/1694011/diff/55001/56002#newcode770 include/v8.h:770: * number, or if COLUMN_OFFSET was not passed as an option when capturing the COLUMN_OFFSET -> kColumnOffset http://codereview.chromium.org/1694011/diff/55001/56002#newcode787 include/v8.h:787: * Returns whether or not the associated function was run via a call to was run -> is compiled http://codereview.chromium.org/1694011/diff/55001/56002#newcode793 include/v8.h:793: * Returns whther or not the associated function is being run as a being run -> called http://codereview.chromium.org/1694011/diff/55001/56008 File src/messages.js (right): http://codereview.chromium.org/1694011/diff/55001/56008#newcode205 src/messages.js:205: function GetLineNumber(message) { Please add the constant kNoLineNumberInfo to JavaScript somewhere. http://codereview.chromium.org/1694011/diff/55001/56004 File src/top.cc (right): http://codereview.chromium.org/1694011/diff/55001/56004#newcode399 src/top.cc:399: SetProperty(stackFrame, Factory::LookupAsciiSymbol("column"), You might consider moving the calls to Factory::LookupAsciiSymbol(xxx) to before the loop so there will only be one lookup for each.
Thanks for the review. I will need a committer to land this for me. http://codereview.chromium.org/1694011/diff/55001/56002 File include/v8.h (right): http://codereview.chromium.org/1694011/diff/55001/56002#newcode697 include/v8.h:697: static const int kNoLineNumberInfo = 0; On 2010/05/05 07:24:19, Søren Gjesse wrote: > Please remove empty line. Done. http://codereview.chromium.org/1694011/diff/55001/56002#newcode704 include/v8.h:704: * A JavaScript stack trace. This will only include JavaScript associated On 2010/05/05 07:24:19, Søren Gjesse wrote: > Please revise this comment into soething like this > > "Representation of a JavaScript stack trace. The information collected is a > snapshot of the execution stack and the information remains valid after > execution continues." > > I don't think we should mention stuff about V8's native runtime.js here as > embedders should not worry about that. Done. But keep in mind that the stack trace stuff in messages.js and runtime.cc does in fact look at frames from native.js I was thinking that we would, in a subsequent patch, make this API take in a flag that determines whether or not to look at frames from native.js so that we can support that use case. The default ofcourse would remain without that flag. For now though, I just removed the reference to it. http://codereview.chromium.org/1694011/diff/55001/56002#newcode741 include/v8.h:741: * Grabs the current JavaScript StackTrace. On 2010/05/05 07:24:19, Søren Gjesse wrote: > Grabs the current JavaScript StackTrace. -> Grab a snapshot of the the current > JavaScript execution stack. Done. http://codereview.chromium.org/1694011/diff/55001/56002#newcode760 include/v8.h:760: * This method will return kNotFound if it is unable to retrieve the line On 2010/05/05 07:24:19, Søren Gjesse wrote: > kNotFound -> Message::kNoLineNumberInfo Done. http://codereview.chromium.org/1694011/diff/55001/56002#newcode761 include/v8.h:761: * number, or if LINE_NUMBER was not passed as an option when capturing the On 2010/05/05 07:24:19, Søren Gjesse wrote: > LINE_NUMBER -> kLineNumber Done. http://codereview.chromium.org/1694011/diff/55001/56002#newcode769 include/v8.h:769: * This method will return kNotFound if it is unable to retrieve the column On 2010/05/05 07:24:19, Søren Gjesse wrote: > kNotFound -> Message::kNoLineNumberInfo You mean Message::kNoColumnInfo http://codereview.chromium.org/1694011/diff/55001/56002#newcode770 include/v8.h:770: * number, or if COLUMN_OFFSET was not passed as an option when capturing the On 2010/05/05 07:24:19, Søren Gjesse wrote: > COLUMN_OFFSET -> kColumnOffset Done. http://codereview.chromium.org/1694011/diff/55001/56002#newcode787 include/v8.h:787: * Returns whether or not the associated function was run via a call to On 2010/05/05 07:24:19, Søren Gjesse wrote: > was run -> is compiled Done. http://codereview.chromium.org/1694011/diff/55001/56002#newcode793 include/v8.h:793: * Returns whther or not the associated function is being run as a On 2010/05/05 07:24:19, Søren Gjesse wrote: > being run -> called Done. http://codereview.chromium.org/1694011/diff/55001/56008 File src/messages.js (right): http://codereview.chromium.org/1694011/diff/55001/56008#newcode205 src/messages.js:205: function GetLineNumber(message) { On 2010/05/05 07:24:19, Søren Gjesse wrote: > Please add the constant kNoLineNumberInfo to JavaScript somewhere. Done. http://codereview.chromium.org/1694011/diff/55001/56004 File src/top.cc (right): http://codereview.chromium.org/1694011/diff/55001/56004#newcode399 src/top.cc:399: SetProperty(stackFrame, Factory::LookupAsciiSymbol("column"), On 2010/05/05 07:24:19, Søren Gjesse wrote: > You might consider moving the calls to Factory::LookupAsciiSymbol(xxx) to before > the loop so there will only be one lookup for each. Done.
Thanks for addressing the final comments. I will land the change.
On 2010/05/05 16:01:25, Søren Gjesse wrote: > Thanks for addressing the final comments. I will land the change. Landed through http://codereview.chromium.org/2028001, http://code.google.com/p/v8/source/detail?r=4597. Closing issue. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
