Bug 53592 - Web Inspector: Add reporting of JS heap size limit to 'console.memory'
Summary: Web Inspector: Add reporting of JS heap size limit to 'console.memory'
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: 53694
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-02 06:53 PST by Mikhail Naganov
Modified: 2011-02-03 12:06 PST (History)
16 users (show)

See Also:


Attachments
patch (10.92 KB, patch)
2011-02-02 09:14 PST, Mikhail Naganov
mnaganov: commit-queue-
Details | Formatted Diff | Diff
fixed style (10.83 KB, patch)
2011-02-02 09:28 PST, Mikhail Naganov
mnaganov: commit-queue-
Details | Formatted Diff | Diff
return 'undefined' limit value for JSC (16.84 KB, patch)
2011-02-03 01:49 PST, Mikhail Naganov
mnaganov: commit-queue-
Details | Formatted Diff | Diff
fixed custom getter signature (16.78 KB, patch)
2011-02-03 04:17 PST, Mikhail Naganov
pfeldman: review+
mnaganov: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Naganov 2011-02-02 06:53:54 PST
This helps to evaluate memory impact of a web application.
Also it's good to know maximum available VM memory size, as it differs between browsers, and their versions (e.g. 32-bit vs. 64-bit).
Comment 1 Patrick Mueller 2011-02-02 07:12:12 PST
Any security aspects here?  Seems like I remember there was something similar that was pulled from console last year, due to security concerns.

I'd like to see this implemented though.
Comment 2 Mikhail Naganov 2011-02-02 07:15:23 PST
After discussions, we made memory info available only by explicit user request (preference in Safari, cmdline flag in Chrome). This resolves security concerns, I believe.
Comment 3 Mikhail Naganov 2011-02-02 09:14:27 PST
Created attachment 80919 [details]
patch

@JSC developers:
  The MAX_NUM_BLOCKS const (ex. maxNumBlocks) appears to be ridiculously big -- being multiplied by BLOCK_SIZE it exceeds ULONG_MAX. Does it need to be adjusted somehow?
  On a 64-bit platform MAX_NUM_BLOCKS value is 2^59 - 1 (as sizeof(PageAllocationAligned) = 16), and BLOCK_SIZE is 2^18.
Comment 4 WebKit Review Bot 2011-02-02 09:17:06 PST
Attachment 80919 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/runtime/MarkedSpace.cpp:43:  MAX_NUM_BLOCKS is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Mikhail Naganov 2011-02-02 09:28:48 PST
Created attachment 80921 [details]
fixed style

Renamed the constant back, as the new name introduces style violation.
Comment 6 Build Bot 2011-02-02 09:42:52 PST
Attachment 80919 [details] did not build on win:
Build output: http://queues.webkit.org/results/7683985
Comment 7 Mikhail Naganov 2011-02-02 09:50:12 PST
...and VC++ confirms that maxNumBlocks is uselessly big. Geoffrey, can you comment on this, please?
Comment 8 Build Bot 2011-02-02 10:06:27 PST
Attachment 80921 [details] did not build on win:
Build output: http://queues.webkit.org/results/7689214
Comment 9 Geoffrey Garen 2011-02-02 10:57:36 PST
What does "limit" mean in this context?

JavaScriptCore doesn't place a hard limit on the size of the heap -- it will use whatever the OS will give it.
Comment 10 Mikhail Naganov 2011-02-02 11:00:34 PST
But there can be other restrictions that prevent VM from allocating new space above a certain limit, e.g. control structures size limits.

Why this maxNumBlocks constant exist at all?
Comment 11 Geoffrey Garen 2011-02-02 11:08:11 PST
> Why this maxNumBlocks constant exist at all?

It seems to exist solely to check for overflow in multiplication. Whatever you're trying to do, this probably isn't the best way to do it.

What does "limit" mean in this context? What are you trying to do?
Comment 12 Mikhail Naganov 2011-02-02 13:05:59 PST
I introduced this change because in V8 there is actually a hard limit on the VM heap size, and its value differs on 32-bit and 64-bit versions, so I didn't want to hardcode it. But knowing the limit appears to be useful, because when web app memory consumption grows up to the limit, renderer crashes.

If JSC really doesn't have a limit, I can put something like undefined in this field. I wanted to make sure that both engines are aligned.
Comment 13 Geoffrey Garen 2011-02-02 16:36:26 PST
undefined sounds good to me.
Comment 14 Mikhail Naganov 2011-02-03 01:49:39 PST
Created attachment 81043 [details]
return 'undefined' limit value for JSC
Comment 15 Build Bot 2011-02-03 02:51:05 PST
Attachment 81043 [details] did not build on win:
Build output: http://queues.webkit.org/results/7692258
Comment 16 Early Warning System Bot 2011-02-03 03:29:54 PST
Attachment 81043 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7687838
Comment 17 Mikhail Naganov 2011-02-03 04:17:37 PST
Created attachment 81049 [details]
fixed custom getter signature
Comment 18 Mikhail Naganov 2011-02-03 08:34:25 PST
Manually committed http://trac.webkit.org/changeset/77495


2011-02-03  Mikhail Naganov  <mnaganov@chromium.org>

        Reviewed by Pavel Feldman.

        Web Inspector: Add reporting of JS heap size limit to 'console.memory'.
        https://bugs.webkit.org/show_bug.cgi?id=53592

        In JSC there is no limit, thus 'undefined' value is returned.
        For V8, the limit reported by the VM is returned.

        * Android.jscbindings.mk:
        * CMakeLists.txt:
        * GNUmakefile.am:
        * WebCore.gypi:
        * WebCore.pro:
        * WebCore.vcproj/WebCore.vcproj:
        * WebCore.xcodeproj/project.pbxproj:
        * bindings/js/JSBindingsAllInOne.cpp:
        * bindings/js/JSMemoryInfoCustom.cpp: Added.
        * bindings/js/ScriptGCEvent.cpp:
        (WebCore::ScriptGCEvent::getHeapSize):
        * bindings/js/ScriptGCEvent.h:
        * bindings/v8/ScriptGCEvent.cpp:
        (WebCore::ScriptGCEvent::getHeapSize):
        * bindings/v8/ScriptGCEvent.h:
        * inspector/InspectorTimelineAgent.cpp:
        (WebCore::InspectorTimelineAgent::setHeapSizeStatistic):
        * page/MemoryInfo.cpp:
        (WebCore::MemoryInfo::MemoryInfo):
        * page/MemoryInfo.h:
        (WebCore::MemoryInfo::jsHeapSizeLimit):
        * page/MemoryInfo.idl:
Comment 20 WebKit Review Bot 2011-02-03 12:06:03 PST
http://trac.webkit.org/changeset/77495 might have broken GTK Linux 32-bit Debug