Bug 80444 – Add a Setting to expose quantized, rate-limited MemoryInfo values

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

Add a Setting to expose quantized, rate-limited MemoryInfo values

[Adam Barth]

Created attachment 130449 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Early Warning System Bot]

Comment on attachment 130449 [details]
Patch

Attachment 130449 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11835602

[Darin Fisher (:fishd, Google)]

Comment on attachment 130449 [details]
Patch

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

> Source/WebKit/chromium/public/WebSettings.h:126
> +    virtual void setQuantizedMemoryInfoEnabled(bool) = 0;

WebKit API change LGTM

[Ian Fette]

Jim and I talked about this this morning, and I expressed a concern that because the buckets have a very large number of significant digits, it's totally non-obvious to a user of the API that you're getting quantized / discretized results at a glance. I expressed the same concern via email last week.

We debated for a good hour and a half back and forth about this patch and how to make this more obvious. What I thought Jim and I settled on was to round the result to three significant digits. With his logarithmic scaling factor, at any given bucket size you know the memory usage +/- ~10%. So rounding it to three significant digits won't actually change anything from a statistically meaningful perspective (we're not throwing away data nor are we giving more data), but at the same time, it makes it clearer to a developer that you're not getting an actual number, but rather an approximation. ("I got 541,000,000 -- clearly that's a rounded number" as opposed to "WTF do I see 541,234,563 all the time?" if you even notice that you're getting the same value).

Another aspect we discussed is that although there are 100 buckets, in practice there's actually only about 40 buckets. 

The scaling factor is  1.10410481

The first bucket is 1M. The 33rd bucket is ~26M. A very simple text-only page (jim's homepage) used 26M in Chrome. about:blank is 17M (bucket 29).

So, basically, I think the first 29 buckets you will never see in practice.

Bucket 77 is around 2GB. It's not clear we will ever have memory >2GB on a 32bit platform. 

So, for most of our users, I contend the only answers are between buckets 33 and 77, or a total of 45 buckets. This is better than nothing, but it's not really the same as having 100 buckets... in practice, we have 45 buckets, which I think Jim and I both agreed was too low.

I think Jim was going to either start the buckets higher (20MB?) and/or increase the number of buckets, such that we actually had more buckets in the usable range, as well as rather than just returning an arbitrary number of significant digits, round it to 3 significant digits so that it's clear to a developer that they're getting an approximation, without actually discretizing to an arbitrary level (e.g. if your discretization algorithm were just '3 significant digits' then as soon as you cross 100MB you go from having 0.1MB granularity to 1MB granularity, the bucketing scheme Jim has at least makes this loss of precision more gradual and avoids such step functions.)

[Adam Barth]

I'm happy to make those changes.  How many buckets would you like?

[Ian Fette]

(In reply to comment #6)
> I'm happy to make those changes.  How many buckets would you like?

A good question -- ideally, I'd like to have at least 100 buckets in the "interesting" range, e.g. between 20MB and 200GB. Personally, I feel more is always merrier, but I would like at least 100 buckets to lie within that range, and the result that gets returned to the user to be limited to 3 significant digits so that it's clear you're getting discrertized results. I will leave the final number to you and/or jim provided that it meets those constraints :)

[Adam Barth]

Created attachment 130482 [details]
Patch

[Adam Barth]

Created attachment 130487 [details]
Patch

[Adam Barth]

@tonyg: Would you be willing to review this patch?

[Tony Gentilcore]

Comment on attachment 130487 [details]
Patch

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

I like the approach. Clever.

A few minor questions and comments. In particular, it would be great if we could test this. r- for this version until the comments are addressed.

> Source/WebCore/ChangeLog:32
> +        information to web pages.  Web pages can only learn new data every 20

Does this restriction really do anything? Can't I just create as many iframes as I want and check once in each?

> Source/WebCore/page/MemoryInfo.cpp:44
> +class HeapSizeCache {

Probably worth making this noncopyable.

Also, do you think it is sizable enough to warrant moving to its own file?

> Source/WebCore/page/MemoryInfo.cpp:54
> +    void getCachedHeapSize(size_t& usedJSHeapSize, size_t& totalJSHeapSize, size_t& jsHeapSizeLimit)

Even though this is only called in one place, the API makes it really easy to get wrong. Should we consider either a struct return value or three separate calls that return size_t's. I don't think the perf here is particularly critical.

> Source/WebCore/page/MemoryInfo.cpp:65
> +        const double TwentyMinutesInSeconds = 20 * 60;

Similar to the comment above the quantize() method, probably worth a comment explaining why we do this.

> Source/WebCore/page/MemoryInfo.cpp:96
> +            const float largestBucketSize = 4000000000.0; // Roughly 4GB.

This size is, quite unfortunately, not entirely unrealistic. Should we allow the largest to be a bit bigger in order to future-proof a little better.

> Source/WebCore/page/MemoryInfo.cpp:101
> +            size_t grainularity = nextPowerOfTen / 1000; // We want 3 signficant digits.

s/grainularity/granularity/

> Source/WebCore/page/MemoryInfo.cpp:103
> +            for (int i = 0; i < numberOfBuckets; ++i) {

The buckets could be layout tested pretty easily, right?

Just add a test with multiple iframes that each create a reference to a string of a different size, then print the heap size in each. Our expectations would just check that they fall into expected buckets.

I ask mostly because I didn't review the math super closely, and it seems best to just have a test do that for us.

> Source/WebCore/page/MemoryInfo.cpp:119
> +                // We cross 2GB around bucket 79.

I don't follow, thought the largest bucket was 4G.

> Source/WebCore/page/Settings.cpp:214
> +    , m_quantizedMemoryInfoEnabled(false)

Perhaps there should just be a tri-state:

m_memoryInfoAPI with possible values of MemoryInfoAPI::disabled MemoryInfoAPI::quantized MemoryInfoAPI::precise

[Kim Hazelwood]

If "memory info" is referring only to the JS heap sizes, then I think these buckets seem sufficient (with the same future scalability concerns raised by Tony). However, if we're talking about private memory (i.e. the new PrivateMemory and SharedMemory APIs that are currently under review) then we'll need the buckets to be scaled higher. We've personally seen private memory for a single tab grow to 9 GB (and have the screen shots to prove it), and have also heard reports of renderer processes growing to 17 GB.

[Adam Barth]

Comment on attachment 130487 [details]
Patch

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

>> Source/WebCore/page/MemoryInfo.cpp:96
>> +            const float largestBucketSize = 4000000000.0; // Roughly 4GB.
> 
> This size is, quite unfortunately, not entirely unrealistic. Should we allow the largest to be a bit bigger in order to future-proof a little better.

We can always revise it later if we run into this limit.

>> Source/WebCore/page/MemoryInfo.cpp:103
>> +            for (int i = 0; i < numberOfBuckets; ++i) {
> 
> The buckets could be layout tested pretty easily, right?
> 
> Just add a test with multiple iframes that each create a reference to a string of a different size, then print the heap size in each. Our expectations would just check that they fall into expected buckets.
> 
> I ask mostly because I didn't review the math super closely, and it seems best to just have a test do that for us.

The tests would take like 20 minutes per query because of the rate limiting.  :)

The testing of this code is less than ideal, but I've tested it in a stand-alone program.  This is really something that unit testing would be better at.

>> Source/WebCore/page/MemoryInfo.cpp:119
>> +                // We cross 2GB around bucket 79.
> 
> I don't follow, thought the largest bucket was 4G.

This comment was for some old bucket parameters.

[Adam Barth]

Comment on attachment 130487 [details]
Patch

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

>> Source/WebCore/page/Settings.cpp:214
>> +    , m_quantizedMemoryInfoEnabled(false)
> 
> Perhaps there should just be a tri-state:
> 
> m_memoryInfoAPI with possible values of MemoryInfoAPI::disabled MemoryInfoAPI::quantized MemoryInfoAPI::precise

That would be better, but setMemoryInfoEnabled() is already exposed as API in a bunch of ports.  I'm not sure how to handle changing that to an enum...

[Adam Barth]

Created attachment 148287 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 148287 [details]
Patch

It would be really really nice to unit-test this quantize function.  It's where this security comes from, no?

Also, we could/should test that this is accessbile (but non-updating) in a LayoutTest, no?  I worry that if this is not tested it will be broken/insecure. :(

[Adam Barth]

Created attachment 149845 [details]
Patch

[WebKit Review Bot]

Comment on attachment 149845 [details]
Patch

Attachment 149845 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13093956

[Adam Barth]

Created attachment 149860 [details]
Patch

[WebKit Review Bot]

Comment on attachment 149860 [details]
Patch

Attachment 149860 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13103985

New failing tests:
MemoryInfo.quantizeMemorySize

[WebKit Review Bot]

Created attachment 149890 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Adam Barth]

Created attachment 149891 [details]
Patch

[Eric Seidel (no email)]

So this API is only for tracking memory usage in long-running pages?  It doesn't seem useful for say getting memory usage at page load time and then again after loading JavaScript?

Do we want to make origins with universal access be able to call this API at any time?

This API seems a little odd because the heap can be shared cross-origin, no?  Certainly in non-chrome browsers?  it seems this data could easily be polluted by having some other large site in your web process?

[Adam Barth]

> So this API is only for tracking memory usage in long-running pages?

Correct.  It's mostly useful for catching memory leaks.

> It doesn't seem useful for say getting memory usage at page load time and then again after loading JavaScript?

Correct.

> Do we want to make origins with universal access be able to call this API at any time?

There's already a setting for enabling full fidelity.  We use that setting in Chrome to make that feature available as a command line flag.

> This API seems a little odd because the heap can be shared cross-origin, no?

Correct.

> Certainly in non-chrome browsers?

And in Chrome.

> it seems this data could easily be polluted by having some other large site in your web process?

Right.  It's mostly useful in aggregate over a large number of users so that effect averages out.  Imagine you run a web site like IRCCloud.  This API would let you see that you're leaking memory as people leave their IRC sessions open.

[Eric Seidel (no email)]

Comment on attachment 149891 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 149891 [details]
Patch

Rejecting attachment 149891 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 #1 succeeded at 242 (offset 12 lines).
patching file Source/WebCore/page/Settings.h
Hunk #1 succeeded at 453 (offset 10 lines).
Hunk #2 succeeded at 734 (offset 14 lines).
patching file Source/WebKit/chromium/WebKit.gypi
Hunk #1 succeeded at 125 with fuzz 2 (offset 1 line).
patching file Source/WebKit/chromium/tests/MemoryInfo.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Eric Seidel']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/13362512

[Adam Barth]

Created attachment 154876 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 154876 [details]
Patch for landing

Clearing flags on attachment: 154876

Committed r123856: <http://trac.webkit.org/changeset/123856>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Patrick R. Gansterer]

(In reply to comment #28)
> (From update of attachment 154876 [details])
> Clearing flags on attachment: 154876
> 
> Committed r123856: <http://trac.webkit.org/changeset/123856>

Broke !ENABLE(INSPECTOR) build.

[Kentaro Hara]

(In reply to comment #30)
> (In reply to comment #28)
> > (From update of attachment 154876 [details] [details])
> > Clearing flags on attachment: 154876
> > 
> > Committed r123856: <http://trac.webkit.org/changeset/123856>
> 
> Broke !ENABLE(INSPECTOR) build.

We've landed the fix in r123873.

[Patrick R. Gansterer]

(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #28)
> > > (From update of attachment 154876 [details] [details] [details])
> > > Clearing flags on attachment: 154876
> > > 
> > > Committed r123856: <http://trac.webkit.org/changeset/123856>
> > 
> > Broke !ENABLE(INSPECTOR) build.
> 
> We've landed the fix in r123873.

Sorry for the noice. (Adding comments about build fixes in the bugs usually helps in tracking such changes, but AFAIK we don't have any policy for it. ;-))

[Thiago Marcos P. Santos]

(In reply to comment #32)
> (In reply to comment #31)
> > (In reply to comment #30)
> > > (In reply to comment #28)
> > > > (From update of attachment 154876 [details] [details] [details] [details])
> > > > Clearing flags on attachment: 154876
> > > > 
> > > > Committed r123856: <http://trac.webkit.org/changeset/123856>
> > > 
> > > Broke !ENABLE(INSPECTOR) build.
> > 
> > We've landed the fix in r123873.
> 
> Sorry for the noice. (Adding comments about build fixes in the bugs usually helps in tracking such changes, but AFAIK we don't have any policy for it. ;-))

I added as a dependency. But agree, better add a comment as well.