Bug 89889 - Web Inspector: replace recursion with a stack in DOM nodes snapshot traversal.
Summary: Web Inspector: replace recursion with a stack in DOM nodes snapshot traversal.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexei Filippov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-25 09:59 PDT by Alexei Filippov
Modified: 2012-07-02 07:18 PDT (History)
11 users (show)

See Also:


Attachments
Patch (5.29 KB, patch)
2012-06-25 10:20 PDT, Alexei Filippov
no flags Details | Formatted Diff | Diff
Patch (5.90 KB, patch)
2012-06-26 08:23 PDT, Alexei Filippov
no flags Details | Formatted Diff | Diff
Patch (5.95 KB, patch)
2012-06-27 03:55 PDT, Alexei Filippov
no flags Details | Formatted Diff | Diff
Patch (6.38 KB, patch)
2012-07-02 05:48 PDT, Alexei Filippov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexei Filippov 2012-06-25 09:59:52 PDT
Number of DOM nodes native snapshots can handle is currently limited by the process stack size because of recursion used to traverse the nodes.
Change the recursion to a stack based algorithm.
Comment 1 Alexei Filippov 2012-06-25 10:20:34 PDT
Created attachment 149315 [details]
Patch
Comment 2 Yury Semikhatsky 2012-06-26 05:49:55 PDT
Comment on attachment 149315 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149315&action=review

> Source/WebCore/dom/MemoryInstrumentation.h:40
> +class MemoryInstrumentedPointerBase;

Forward declarations should go in alphabetic order.

> Source/WebCore/dom/MemoryInstrumentation.h:111
> +    void countObjectSize()

Please inline this method instead.

> Source/WebCore/dom/MemoryInstrumentation.h:134
> +    void processInstrumentedPointer(MemoryInstrumentation* memoryInstrumentation)

missing virtual and OVERRIDE

> Source/WebCore/inspector/InspectorMemoryAgent.cpp:451
> +            delete pointer;

Please use queue of OwnPtr's to avoid manual delete.

> Source/WebCore/inspector/InspectorMemoryAgent.cpp:475
> +    Vector<MemoryInstrumentedPointerBase*> m_worklist;

m_instrumentedPointers?
Comment 3 Alexei Filippov 2012-06-26 08:23:26 PDT
Created attachment 149536 [details]
Patch
Comment 4 Alexei Filippov 2012-06-26 08:25:15 PDT
Comment on attachment 149315 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149315&action=review

>> Source/WebCore/dom/MemoryInstrumentation.h:40
>> +class MemoryInstrumentedPointerBase;
> 
> Forward declarations should go in alphabetic order.

done

>> Source/WebCore/dom/MemoryInstrumentation.h:111
>> +    void countObjectSize()
> 
> Please inline this method instead.

done

>> Source/WebCore/dom/MemoryInstrumentation.h:134
>> +    void processInstrumentedPointer(MemoryInstrumentation* memoryInstrumentation)
> 
> missing virtual and OVERRIDE

done

>> Source/WebCore/inspector/InspectorMemoryAgent.cpp:451
>> +            delete pointer;
> 
> Please use queue of OwnPtr's to avoid manual delete.

done

>> Source/WebCore/inspector/InspectorMemoryAgent.cpp:475
>> +    Vector<MemoryInstrumentedPointerBase*> m_worklist;
> 
> m_instrumentedPointers?

m_deferredInstrumentedPointers
Comment 5 Ilya Tikhonovsky 2012-06-26 12:53:59 PDT
Comment on attachment 149536 [details]
Patch

lgtm
Comment 6 Yury Semikhatsky 2012-06-27 00:43:08 PDT
Comment on attachment 149536 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149536&action=review

> Source/WebCore/dom/MemoryInstrumentation.h:120
> +class MemoryInstrumentedPointerBase {

I'd move these classes into MemoryInstrumentation as they are just implementation details. It would also allow us to give them shorter names.

> Source/WebCore/inspector/InspectorMemoryAgent.cpp:476
> +    Vector<PassOwnPtr<MemoryInstrumentedPointerBase> > m_deferredInstrumentedPointers;

PassOwnPtr is supposed to be used for passing objects ownership only, you should use Vector<OwnPtr<...> > here.
Comment 7 Alexei Filippov 2012-06-27 03:55:55 PDT
Created attachment 149720 [details]
Patch
Comment 8 Alexei Filippov 2012-07-02 05:48:16 PDT
Created attachment 150405 [details]
Patch
Comment 9 Alexei Filippov 2012-07-02 05:53:22 PDT
Comment on attachment 149536 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149536&action=review

>> Source/WebCore/dom/MemoryInstrumentation.h:120
>> +class MemoryInstrumentedPointerBase {
> 
> I'd move these classes into MemoryInstrumentation as they are just implementation details. It would also allow us to give them shorter names.

done

>> Source/WebCore/inspector/InspectorMemoryAgent.cpp:476
>> +    Vector<PassOwnPtr<MemoryInstrumentedPointerBase> > m_deferredInstrumentedPointers;
> 
> PassOwnPtr is supposed to be used for passing objects ownership only, you should use Vector<OwnPtr<...> > here.

done
Comment 10 WebKit Review Bot 2012-07-02 07:18:42 PDT
Comment on attachment 150405 [details]
Patch

Clearing flags on attachment: 150405

Committed r121677: <http://trac.webkit.org/changeset/121677>
Comment 11 WebKit Review Bot 2012-07-02 07:18:46 PDT
All reviewed patches have been landed.  Closing bug.