Bug 81358 – New PageCache histogram for tracking the highest leverage frame reject reasons.

• Component: New Bugs
• Status: RESOLVED FIXED

[Gavin Peters]

New PageCache histogram for tracking the highest leverage frame reject reasons.

[Gavin Peters]

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.

[Gavin Peters]

Created attachment 132295 [details]
Patch

[Gavin Peters]

Brady, WDYT?

[Gavin Peters]

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.

[Adam Barth]

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.

[Gavin Peters]

Created attachment 132806 [details]
Patch

[Gavin Peters]

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.

[Build Bot]

Comment on attachment 132806 [details]
Patch

Attachment 132806 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11994271

[Gavin Peters]

Created attachment 132808 [details]
Patch

[WebKit Review Bot]

Comment on attachment 132808 [details]
Patch

Clearing flags on attachment: 132808

Committed r111391: <http://trac.webkit.org/changeset/111391>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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.

[Gavin Peters]

Yes, I did.  Patch coming.

[Gavin Peters]

mitz fixed that failure in r111391.