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. http://codereview.chromium.org/1694011/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
