Bug 80444

Created attachment 130449 [details] Patch

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

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

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

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.)

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

(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 :)

Created attachment 130482 [details] Patch

Created attachment 130487 [details] Patch

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

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

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.

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.

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...

Created attachment 148287 [details] Patch

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. :(

Created attachment 149845 [details] Patch

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

Created attachment 149860 [details] Patch

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

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

Created attachment 149891 [details] Patch

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?

> 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.

Comment on attachment 149891 [details] Patch LGTM.

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

Created attachment 154876 [details] Patch for landing

Comment on attachment 154876 [details] Patch for landing Clearing flags on attachment: 154876 Committed r123856: <http://trac.webkit.org/changeset/123856>

All reviewed patches have been landed. Closing bug.

(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.

(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.

(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. ;-))

(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.