Bug 81358 - New PageCache histogram for tracking the highest leverage frame reject reasons.
Summary: New PageCache histogram for tracking the highest leverage frame reject reasons.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Peters
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-16 09:18 PDT by Gavin Peters
Modified: 2012-03-20 09:43 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.78 KB, patch)
2012-03-16 09:24 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (3.35 KB, patch)
2012-03-20 05:21 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
Patch (3.35 KB, patch)
2012-03-20 05:34 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Peters 2012-03-16 09:18:18 PDT
New PageCache histogram for tracking the highest leverage frame reject reasons.
Comment 1 Gavin Peters 2012-03-16 09:21:48 PDT
The current chromium FrameClientImpl always denies page cache; but that's easily fixable.  This histogram tracks which rejection causes occur alone with that cause.  I'm particularly interested in knowing how many more pages would work in the page cache if we could get plugins in.
Comment 2 Gavin Peters 2012-03-16 09:24:04 PDT
Created attachment 132295 [details]
Patch
Comment 3 Gavin Peters 2012-03-16 09:25:30 PDT
Brady, WDYT?
Comment 4 Gavin Peters 2012-03-19 13:30:10 PDT
Comment on attachment 132295 [details]
Patch

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

> Source/WebCore/history/PageCache.cpp:281
> +        ASSERT(frameRejectReasons & (1 << ClientDeniesCaching));

Should I add:
COMPILE_ASSERT(NumberOfReasonsPageCannotBeInPageCache <= 32, ReasonPageCannotBeInPageCacheDoesNotFitIn32Bits);

here?  I don't expect the code will live that long, but it seems a good idea.
Comment 5 Adam Barth 2012-03-19 13:32:22 PDT
Comment on attachment 132295 [details]
Patch

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

> Source/WebCore/history/PageCache.cpp:282
> +        const unsigned v = frameRejectReasons & ~(1 << ClientDeniesCaching);

v ?  Can we give this variable a name that explains what it is?  Perhaps reasonsForRejectingFrameOtherThanClientDeniesCaching?

> Source/WebCore/history/PageCache.cpp:294
> +        int index = 0;
> +        if (v & 0xFFFF0000)
> +            index = 16;
> +        if (v & 0xFF00FF00)
> +            index += 8;
> +        if (v & 0xF0F0F0F0)
> +            index += 4;
> +        if (v & 0xCCCCCCCC)
> +            index += 2;
> +        if (v & 0xAAAAAAAA)
> +            index += 1;

Can you factor this out into a helper function.  This logic seems pretty general.  That will help folks understand what this code is doing.
Comment 6 Gavin Peters 2012-03-20 05:21:55 PDT
Created attachment 132806 [details]
Patch
Comment 7 Gavin Peters 2012-03-20 05:22:29 PDT
Comment on attachment 132806 [details]
Patch

Here's the remediated patch; I'll let this bake on EWS for a bit, and cq+ it later today.
Comment 8 Build Bot 2012-03-20 05:30:25 PDT
Comment on attachment 132806 [details]
Patch

Attachment 132806 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11994271
Comment 9 Gavin Peters 2012-03-20 05:34:36 PDT
Created attachment 132808 [details]
Patch
Comment 10 WebKit Review Bot 2012-03-20 07:31:34 PDT
Comment on attachment 132808 [details]
Patch

Clearing flags on attachment: 132808

Committed r111391: <http://trac.webkit.org/changeset/111391>
Comment 11 WebKit Review Bot 2012-03-20 07:31:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Csaba Osztrogonác 2012-03-20 08:59:42 PDT
It broke debug builds:
cc1plus: warnings being treated as errors
../../../../Source/WebCore/history/PageCache.cpp:86: error: ‘int WebCore::indexOfSingleBit(int32_t)’ defined but not used

It seems some ifdef guard is missing somewhere.
Comment 13 Gavin Peters 2012-03-20 09:38:15 PDT
Yes, I did.  Patch coming.
Comment 14 Gavin Peters 2012-03-20 09:43:39 PDT
mitz fixed that failure in r111391.