On Fri, Apr 23, 2010 at 11:36 AM, Søren Gjesse <[email protected]> wrote:
> > > On Fri, Apr 23, 2010 at 17:17, <[email protected]> wrote: > >> 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. >> >> > All Runtime_XXX functions are normally called from either JavaScript using > the native %XXX syntax or from generated code. > > This was what I thought. > >> 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 >> :). >> >> > Any allocation can lead to a GC if the allocation request cannot be > fulfilled with the current heap state. It is all hidden in the > CALL_HEAP_FUNCTION macro. > > Gotcha. Makes sense. I would say thought that in practice, it doesn't happen very often. So in terms of disturbing the profile, it is "more correct" than using an approach that frequently lets GC run :). > >> > 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. >> >> > I have no problem with iterating while defining the API. However this is in > the public API, and other users of V8 might start to use it as well and we > might find ourselves in a situation where we need to support a number of > API. There is also the issue that when the WebKit usecase has been fulfilled > the API might not evolve into a more general one. While experimenting we > could consider adding an additional header include/v8-experimental.h, or > have use of experimental API's requiring defining V8_EXPERIMENTAL_API. > I agree with not landing unfinished APIs to the public headers. I will go back and revise the patch to expose C++ API for a general purpose grabbing of stack information that takes in flags parameterizing what information we wish to resolve. Do you want me to move this from the Debug namespace to the v8 namespace? > > >> 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. >> >> >> http://codereview.chromium.org/1694011/show >> > > -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
