Summary: | Minimize the amount of memcpy done for allocating Error stacks. | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | commit-queue, keith_miller, msaboff, saam | ||||
Priority: | P2 | ||||||
Version: | WebKit Local Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Mark Lam
2016-06-11 12:09:54 PDT
Created attachment 281106 [details]
proposed patch.
Comment on attachment 281106 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=281106&action=review > Source/JavaScriptCore/inspector/ScriptCallStackFactory.cpp:140 > + const Vector<StackFrame>& stackTrace = exception->stack(); I would prefer this: auto& stackTrace = exception->stack(); What do you think? > Source/JavaScriptCore/interpreter/StackVisitor.h:192 > +class StackDepthCountFunctor { > +public: > + StackVisitor::Status operator()(StackVisitor&) const > + { > + count++; > + return StackVisitor::Continue; > + } > + > + mutable size_t count { 0 }; > +}; I’m not sure this belongs in a header. Why not put it closer to where it’s used? Maybe it could even be a lambda that captures a reference to a local variable. Comment on attachment 281106 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=281106&action=review >> Source/JavaScriptCore/inspector/ScriptCallStackFactory.cpp:140 >> + const Vector<StackFrame>& stackTrace = exception->stack(); > > I would prefer this: > > auto& stackTrace = exception->stack(); > > What do you think? Sure. I'm fine with that. >> Source/JavaScriptCore/interpreter/StackVisitor.h:192 >> +}; > > I’m not sure this belongs in a header. Why not put it closer to where it’s used? Maybe it could even be a lambda that captures a reference to a local variable. Thanks. I'm so used to doing this the old way that I missed that we can use a lambda here. I put this functor class in this header because I think it can be useful for other uses other than just the current one e.g. in custom code added during debugging sessions. Rather than having to roll one every time, I thought I'd just put it here as a utility. But since the lambda I would have to write instead for this is almost trivial, I'll go with the lambda instead. Thanks for the review. Landed in r201976: <http://trac.webkit.org/r201976>. |