[Webkit-unassigned] [Bug 37502] Web Inspector: Removes public callLocation API from ScriptCallStack and replaces with ability to get top 10 stack frames

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=37502





--- Comment #4 from jaimeyap at google.com  2010-04-14 07:02:27 PST ---
I forgot to reply on the bug itself. I for some reason thought replying via
email would post the response. Re-posting here:

(In reply to comment #2)
> (From update of attachment 53257 [details])
> > +    static bool stackTrace(String& stackTrace);
> 
> Should be String* as in other places returning values.
> 
> >  InspectorTimelineAgent::InspectorTimelineAgent(InspectorFrontend* frontend)
> > -    : m_frontend(frontend)
> > +    : m_frontend(frontend),
> > +      m_currentStackTrace(),
> 
> No need to init this one.
> 
> > +      m_hasStackTrace(false)
> 
> Is following true?
> 
> m_hasStackTrace == !m_currentStackTrace.isEmpty()

Not necessarily. Consider the case where the the method in the utility context
fails to set a value for the stack trace (in the case where there is no stack
trace). That is, we try to retrieve a stack trace, but for some reason the
returned value is the empty String.

We do not want to have to re-enter the utility context for each node if we have
already attempted to retrieve a stack trace, so we need to keep some separate
state reflecting the condition "I have checked for a stack trace, and failed to
get one.". This is separate from simply saying "The stack is invalid" (empty)
or "the stack is valid" (set to some value).

> 
> >  {
> >      ++s_instanceCount;
> >      ScriptGCEvent::addEventListener(this);
> > @@ -57,7 +60,7 @@ InspectorTimelineAgent::InspectorTimelin
> >  void InspectorTimelineAgent::pushGCEventRecords()
> >  {
> >      for (GCEvents::iterator i = m_gcEvents.begin(); i != m_gcEvents.end(); ++i) {
> > -        ScriptObject record = TimelineRecordFactory::createGenericRecord(m_frontend, i->startTime);
> > +        ScriptObject record = TimelineRecordFactory::createGenericRecord(m_frontend, i->startTime, getCurrentStackTrace());
> 
> Present stack has nothing to do with the stack while doing GC.
> 

Good point.

> > +    if (m_recordStack.isEmpty()) {
> >          m_frontend->addRecordToTimeline(record);
> > -    else {
> > +        invalidateStackTrace();
> > +    } else {
> >          TimelineRecordEntry parent = m_recordStack.last();
> >          parent.children.set(parent.children.length(), record);
> >      }
> 
> No need for { } in single-line statements.
> 
> > +String* InspectorTimelineAgent::getCurrentStackTrace()
> > +{
> > +    if (m_currentStackTrace.isEmpty()) {
> 
> I wonder if you covered all the cases where stack needs to be invalidated. 

I did miss a few cases. I will update the patch in the morning. I also need to
invalidate when popping function call and eval script.

How
> often do we get nested records where this cache is actually helping?

Alot! You should see Wave or GMail. The pattern of UI construction where using
innerHTML for lots of small widgets causes literally hundreds of parseHTML
nodes. For nearly all complicated traces, caching the trace  makes a very large
difference. It is literally the difference between a few milliseconds versus
sometimes several seconds of overhead.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.