Bug 39646 - Web Inspector: add Console API for retrieving memory stats
Summary: Web Inspector: add Console API for retrieving memory stats
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: Mikhail Naganov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-24 22:14 PDT by Mikhail Naganov
Modified: 2010-06-04 13:42 PDT (History)
12 users (show)

See Also:


Attachments
proposed patch (2.82 KB, patch)
2010-05-24 22:23 PDT, Mikhail Naganov
no flags Details | Formatted Diff | Diff
fixed lint error (2.82 KB, patch)
2010-05-25 08:18 PDT, Mikhail Naganov
no flags Details | Formatted Diff | Diff
Implemented as 'console.memory' object after chatting with xenon (4.34 KB, patch)
2010-05-27 07:11 PDT, Mikhail Naganov
no flags Details | Formatted Diff | Diff
now with updated test expectations! (7.33 KB, patch)
2010-05-28 06:08 PDT, Mikhail Naganov
pfeldman: review-
mnaganov: commit-queue-
Details | Formatted Diff | Diff
Introduce MemoryInfo interface to access memory statistics (23.58 KB, patch)
2010-06-02 02:35 PDT, Mikhail Naganov
mnaganov: commit-queue-
Details | Formatted Diff | Diff
Yury's comments addressed (23.49 KB, patch)
2010-06-02 06:58 PDT, Mikhail Naganov
yurys: review+
mnaganov: commit-queue-
Details | Formatted Diff | Diff
Fixed windows build and test expectations (26.60 KB, patch)
2010-06-02 09:05 PDT, Mikhail Naganov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Naganov 2010-05-24 22:14:14 PDT
The first thing that can be added is JS heap size stats. Monitoring them is useful for continuous builds for catching regressions.

My proposal is to add 'console.memoryStats()' method which returns an object. Currently it has two fields: totalHeapSize and usedHeapSize. Later, it can be extended for reporting total browser's memory consumption.
Comment 1 Mikhail Naganov 2010-05-24 22:23:18 PDT
Created attachment 56972 [details]
proposed patch
Comment 2 WebKit Review Bot 2010-05-24 22:24:46 PDT
Attachment 56972 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/page/Console.cpp:450:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Mikhail Naganov 2010-05-25 08:18:51 PDT
Created attachment 57021 [details]
fixed lint error
Comment 4 Pavel Feldman 2010-05-25 14:31:30 PDT
Comment on attachment 57021 [details]
fixed lint error

Please land manually once you get xenon's LG on this.
Comment 5 Mikhail Naganov 2010-05-27 07:11:14 PDT
Created attachment 57239 [details]
Implemented as 'console.memory' object after chatting with xenon
Comment 6 WebKit Commit Bot 2010-05-27 14:15:06 PDT
Comment on attachment 57239 [details]
Implemented as 'console.memory' object after chatting with xenon

Rejecting patch 57239 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1
Last 500 characters of output:
ree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Skipped list contained 'compositing/iframes/composited-iframe.html', but no file of that name could be found
Testing 19005 test cases.
fast/dom/Window/window-properties.html -> failed

Exiting early after 1 failures. 6751 tests run.
124.46s total testing time

6750 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/2554059
Comment 7 Mikhail Naganov 2010-05-28 06:08:12 PDT
Created attachment 57320 [details]
now with updated test expectations!
Comment 8 Pavel Feldman 2010-05-28 11:30:22 PDT
Comment on attachment 57320 [details]
now with updated test expectations!

Oh, my bad. As ggaren pointed out, we need to make sure client code works fine no matter whether dev extras / inspector are there or not. So we need to make sure that console.memory object presents at all times. It may have no properties in case instrumentation is not available.
Comment 9 Eric Seidel (no email) 2010-06-02 02:27:10 PDT
Comment on attachment 57021 [details]
fixed lint error

Cleared Pavel Feldman's review+ from obsolete attachment 57021 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 10 Eric Seidel (no email) 2010-06-02 02:27:16 PDT
Comment on attachment 57239 [details]
Implemented as 'console.memory' object after chatting with xenon

Cleared Pavel Feldman's review+ from obsolete attachment 57239 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 11 Mikhail Naganov 2010-06-02 02:35:00 PDT
Created attachment 57637 [details]
Introduce MemoryInfo interface to access memory statistics
Comment 12 Yury Semikhatsky 2010-06-02 05:18:31 PDT
Comment on attachment 57637 [details]
Introduce MemoryInfo interface to access memory statistics

WebCore/GNUmakefile.am:298
 +  	WebCore/page/MemoryInfo.idl \
Wrong ordering

WebCore/page/MemoryInfo.idl:35
 +          readonly attribute unsigned long totalHeapSize;
We may want to be more specific about what exactly heap size it is if we want at some point to provide total browser heap size.

WebCore/page/MemoryInfo.h:43
 +      unsigned long totalHeapSize() const { return m_totalHeapSize; }
Why is it unsigned ling and not size_t?
Comment 13 Mikhail Naganov 2010-06-02 06:58:11 PDT
Created attachment 57650 [details]
Yury's comments addressed
Comment 14 Mikhail Naganov 2010-06-02 07:31:32 PDT
Reverted r60563 for reason:

windows build failed

Committed r60565: <http://trac.webkit.org/changeset/60565>
Comment 15 WebKit Review Bot 2010-06-02 08:08:01 PDT
http://trac.webkit.org/changeset/60563 might have broken Leopard Intel Release (Tests)
Comment 16 Mikhail Naganov 2010-06-02 09:05:42 PDT
Created attachment 57662 [details]
Fixed windows build and test expectations
Comment 17 Mikhail Naganov 2010-06-03 01:13:00 PDT
Committed manually: http://trac.webkit.org/changeset/60568

    2010-06-02  Mikhail Naganov  <mnaganov@chromium.org>
    
            Reviewed by Yuri Semikhatsky.
    
            Web Inspector: add Console API for retrieving memory stats
    
            Add 'console.memory' property which returns an object. Currently
            it has two fields: totalJSHeapSize and usedJSHeapSize. Later, it can be
            extended for reporting total browser's memory consumption.
    
            https://bugs.webkit.org/show_bug.cgi?id=39646
    
            * CMakeLists.txt:
            * DerivedSources.cpp:
            * DerivedSources.make:
            * GNUmakefile.am:
            * WebCore.gypi:
            * WebCore.pri:
            * WebCore.pro:
            * WebCore.vcproj/WebCore.vcproj:
            * WebCore.xcodeproj/project.pbxproj:
            * bindings/js/JSBindingsAllInOne.cpp:
            * bindings/js/JSConsoleCustom.cpp:
            (WebCore::JSConsole::memory):
            * bindings/v8/custom/V8ConsoleCustom.cpp:
            (WebCore::V8Console::memoryAccessorGetter):
            * page/Console.h:
            * page/Console.idl:
            * page/MemoryInfo.cpp: Added.
            (WebCore::MemoryInfo::MemoryInfo):
            * page/MemoryInfo.h: Added.
            (WebCore::MemoryInfo::create):
            (WebCore::MemoryInfo::totalJSHeapSize):
            (WebCore::MemoryInfo::usedJSHeapSize):
            * page/MemoryInfo.idl: Added.
            * fast/dom/Window/window-properties-expected.txt:
            * fast/dom/prototype-inheritance-2-expected.txt:
            * platform/gtk/fast/dom/Window/window-properties-expected.txt:
            * platform/qt/fast/dom/Window/window-properties-expected.txt:
            * platform/win/fast/dom/prototype-inheritance-2-expected.txt: