RESOLVED FIXED Bug 81358
New PageCache histogram for tracking the highest leverage frame reject reasons. 
https://bugs.webkit.org/show_bug.cgi?id=81358
Summary New PageCache histogram for tracking the highest leverage frame reject reasons.
Gavin Peters
Reported 2012-03-16 09:18:18 PDT
New PageCache histogram for tracking the highest leverage frame reject reasons.
Attachments
Patch (2.78 KB, patch)
2012-03-16 09:24 PDT, Gavin Peters 		no flags
DetailsFormatted DiffDiff
Patch (3.35 KB, patch)
2012-03-20 05:21 PDT, Gavin Peters 		no flags
DetailsFormatted DiffDiff
Patch (3.35 KB, patch)
2012-03-20 05:34 PDT, Gavin Peters 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Gavin Peters
Comment 1 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.
Gavin Peters
Comment 2 2012-03-16 09:24:04 PDT
Created attachment 132295 [details] Patch
Gavin Peters
Comment 3 2012-03-16 09:25:30 PDT
Brady, WDYT?
Gavin Peters
Comment 4 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.
Adam Barth
Comment 5 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.
Gavin Peters
Comment 6 2012-03-20 05:21:55 PDT
Created attachment 132806 [details] Patch
Gavin Peters
Comment 7 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.
Build Bot
Comment 8 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
Gavin Peters
Comment 9 2012-03-20 05:34:36 PDT
Created attachment 132808 [details] Patch
WebKit Review Bot
Comment 10 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>
WebKit Review Bot
Comment 11 2012-03-20 07:31:39 PDT
All reviewed patches have been landed. Closing bug.
Csaba Osztrogonác
Comment 12 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.
Gavin Peters
Comment 13 2012-03-20 09:38:15 PDT
Yes, I did. Patch coming.
Gavin Peters
Comment 14 2012-03-20 09:43:39 PDT
mitz fixed that failure in r111391.
Note You need to log in before you can comment on or make changes to this bug.