add Histograms for reporting on PageCache reject reasons
Created attachment 129949 [details] Patch
Created attachment 129950 [details] Patch
Here's patches that add histogram reporting to PageCache.cpp, so we can see what is causing pages to not enter the page cache in the field. I decided to put the logic into the debug-only code for logging page cache decisions. Histogram reporting is semantically very similar to logging, and the code was already traversing the tree. The bad news for debug builds in non-Chromium ports is that the histogram reporting section does a few loops in order to call what are ultimately NOPs. This could be guarded by #ifdefs, but I made a call. Let me know if I should reverse it.
Comment on attachment 129950 [details] Patch Attachment 129950 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11802138
Created attachment 129960 [details] Patch
Comment on attachment 129960 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129960&action=review > Source/WebCore/history/PageCache.cpp:89 > +#endif Is this the correct use of ASSERT_UNUSED?
Comment on attachment 129960 [details] Patch Attachment 129960 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11802177
I'll come review this after it passes EWS :)
Created attachment 129976 [details] Patch
Comment on attachment 129976 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129976&action=review > Source/WebCore/history/PageCache.cpp:88 > + UNUSED_PARAM(indentLevel); The answer to was I using it correctly was: No. This use here should be good, though the build test bots will know best.
Created attachment 129982 [details] Patch
For those of you scoring at home, that is now three uploads just to get a version that compiles. The lesson is that you should test as many builds as possible before upload; I knew I was doing conditional compilation that would particularly affect the Chromium release build, and I didn't do that. Brady, you're wise to wait on EWS. I'll email you out of band once it's passed, to save you coming back here every two minutes, as you currently must be. :)
(In reply to comment #12) > Brady, you're wise to wait on EWS. I'll email you out of band once it's passed, to save you coming back here every two minutes, as you currently must be. :) Nah, I'm just watching my bugzilla emails. It's unlikely I'll get to it this weekend but if I don't I'll look Monday morning.
Comment on attachment 129982 [details] Patch I have another upload coming on this, I've thought a bit more about it.
Created attachment 130403 [details] Patch
This latest upload is a merge to trunk (abarth removed the geolocation exclusion yesterday, yay!). As well, I have pared down and renamed the histograms. The previous patch had too much of a kitchen-sink approach to histograms; I would like to at least see some statistics uploaded and incrementally add more histograms as we learn more about what is causing PageCache to not be useful. Also, note the renaming of the "....ByPage" histograms. The intention there is to make them collate directly next to their earlier Frame related cousins; since the "about:histograms/PageCache" display in Chrome sorts alphabetically, this contributes a lot to making that data more immediately useful. beidson, abarth: WDYT?
Comment on attachment 130403 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130403&action=review > Source/WebCore/history/PageCache.cpp:65 > +enum FrameRejectCause { FrameRejectCause -> Are we really rejecting frames? I would try to pick some name that reflects the PageCache... > Source/WebCore/history/PageCache.cpp:81 > + frameRejectCause_count, frameRejectCause_count isn't really a WebKit-style name. Maybe numberOfFrameRejectCauses (or whatever you end up calling the type) > Source/WebCore/history/PageCache.cpp:175 > + for (int i = 0; i < frameRejectCause_count; ++i) Missing the { } around the body of this loop.
Those comments probably aren't overly helpful... This patch looks fine. We should just clean it up a bit style-wise.
Created attachment 130430 [details] more stylish
(In reply to comment #17) > (From update of attachment 130403 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=130403&action=review > > > Source/WebCore/history/PageCache.cpp:65 > > +enum FrameRejectCause { > > FrameRejectCause -> Are we really rejecting frames? I would try to pick some name that reflects the PageCache... Done. Behold ReasonFrameCannotBeInPageCache and ReasonPageCannotBeInPageCache. > > > Source/WebCore/history/PageCache.cpp:81 > > + frameRejectCause_count, > > frameRejectCause_count isn't really a WebKit-style name. Maybe numberOfFrameRejectCauses (or whatever you end up calling the type) Done. NumberOfReasonsFOOsCannotBeInPageCache. > > Source/WebCore/history/PageCache.cpp:175 > > + for (int i = 0; i < frameRejectCause_count; ++i) > > Missing the { } around the body of this loop. Done. There were two more of these. (In reply to comment #18) > Those comments probably aren't overly helpful... This patch looks fine. We should just clean it up a bit style-wise. Besides the enumerated style failures above; I also had violations of http://www.webkit.org/coding/coding-style.html#names-enum-members and http://www.webkit.org/coding/coding-style.html#spacing-binary-ternary-op , both fixed now.
Comment on attachment 130430 [details] more stylish View in context: https://bugs.webkit.org/attachment.cgi?id=130430&action=review > Source/WebCore/history/PageCache.cpp:64 > +// Used in histograms, please only add at the end, and do not remove elements. We'll probably end up changing this enum over time as we modify this file. I guess we'll end up with unused entries...
Comment on attachment 130430 [details] more stylish This looks fine to me. Please give Brady a chance to leave comments before landing.
Created attachment 130434 [details] more stylish 2
Comment on attachment 130434 [details] more stylish 2 Thanks Adam for your review. I am now waiting for a review from beidson before landing this patch. I've updated the comment above the histogram to be more explicit. My expectation is that I'll be watching (and sharing) the results of this histogram telemetry carefully over the next few weeks, so updates of the enums can be quick. But if these become longer term histograms, then the enums become rather slow to be updatable without collision.
Comment on attachment 130434 [details] more stylish 2 View in context: https://bugs.webkit.org/attachment.cgi?id=130434&action=review > Source/WebCore/ChangeLog:3 > + add Histograms for reporting on PageCache reject reasons Nit pick - s/add/Add/
Created attachment 130584 [details] Patch
Thanks Brady for the review. The patch I've uploaded fixes the capitalisation, and will go into cq when I get to my desk in a few hours (~9:00am EST).
Created attachment 130588 [details] fix date too
Comment on attachment 130588 [details] fix date too cq+, i'm in #webkit and on gtalk as gavinp@google.com
Comment on attachment 130588 [details] fix date too Clearing flags on attachment: 130588 Committed r110057: <http://trac.webkit.org/changeset/110057>
All reviewed patches have been landed. Closing bug.