Bug 80187 – Add Histograms for reporting on PageCache reject reasons

• Component: New Bugs
• Status: RESOLVED FIXED

[Gavin Peters]

add Histograms for reporting on PageCache reject reasons

[Gavin Peters]

Created attachment 129949 [details]
Patch

[Gavin Peters]

Created attachment 129950 [details]
Patch

[Gavin Peters]

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.

[WebKit Review Bot]

Comment on attachment 129950 [details]
Patch

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

[Gavin Peters]

Created attachment 129960 [details]
Patch

[Gavin Peters]

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?

[WebKit Review Bot]

Comment on attachment 129960 [details]
Patch

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

[Brady Eidson]

I'll come review this after it passes EWS  :)

[Gavin Peters]

Created attachment 129976 [details]
Patch

[Gavin Peters]

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.

[Gavin Peters]

Created attachment 129982 [details]
Patch

[Gavin Peters]

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

[Brady Eidson]

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

[Gavin Peters]

Comment on attachment 129982 [details]
Patch

I have another upload coming on this, I've thought a bit more about it.

[Gavin Peters]

Created attachment 130403 [details]
Patch

[Gavin Peters]

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?

[Adam Barth]

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.

[Adam Barth]

Those comments probably aren't overly helpful...  This patch looks fine.  We should just clean it up a bit style-wise.

[Gavin Peters]

Created attachment 130430 [details]
more stylish

[Gavin Peters]

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

[Adam Barth]

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

[Adam Barth]

Comment on attachment 130430 [details]
more stylish

This looks fine to me.  Please give Brady a chance to leave comments before landing.

[Gavin Peters]

Created attachment 130434 [details]
more stylish 2

[Gavin Peters]

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.

[Brady Eidson]

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/

[Gavin Peters]

Created attachment 130584 [details]
Patch

[Gavin Peters]

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

[Gavin Peters]

Created attachment 130588 [details]
fix date too

[Gavin Peters]

Comment on attachment 130588 [details]
fix date too

cq+, i'm in #webkit and on gtalk as [e-mail]

[WebKit Review Bot]

Comment on attachment 130588 [details]
fix date too

Clearing flags on attachment: 130588

Committed r110057: <http://trac.webkit.org/changeset/110057>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.