Bug 141345 - [WK2] Add logging to validate the network cache efficacy (Part 2)
Summary: [WK2] Add logging to validate the network cache efficacy (Part 2)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on: 141269 141464
Blocks:
  Show dependency treegraph
 
Reported: 2015-02-06 16:34 PST by Chris Dumez
Modified: 2015-02-11 16:13 PST (History)
4 users (show)

See Also:


Attachments
WIP Patch (7.02 KB, patch)
2015-02-06 16:36 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (23.55 KB, patch)
2015-02-09 11:06 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (23.56 KB, patch)
2015-02-09 15:08 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (24.13 KB, patch)
2015-02-10 17:47 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (24.20 KB, patch)
2015-02-11 11:45 PST, Chris Dumez
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2015-02-06 16:34:16 PST
Add diagnostic logging support from the NetworkProcess and use it to log information we need to validate the network cache efficacy.
Comment 1 Chris Dumez 2015-02-06 16:36:45 PST
Created attachment 246187 [details]
WIP Patch
Comment 2 Chris Dumez 2015-02-09 11:06:46 PST
Created attachment 246279 [details]
Patch
Comment 3 WebKit Commit Bot 2015-02-09 11:08:10 PST
Attachment 246279 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:221:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.h:67:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Antti Koivisto 2015-02-09 15:04:01 PST
Comment on attachment 246279 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cache/NetworkCache.h:67
> -    void retrieve(const WebCore::ResourceRequest&, std::function<void (std::unique_ptr<Entry>)>);
> +    void retrieve(uint64_t webPageID, const WebCore::ResourceRequest&, std::function<void (std::unique_ptr<Entry>)>);

Please make webPageID the seconds argument since ResourceRequest is the important one.
Comment 5 Chris Dumez 2015-02-09 15:08:08 PST
Created attachment 246294 [details]
Patch
Comment 6 Chris Dumez 2015-02-09 15:09:00 PST
Sam, can you please confirm you're OK with this? It should more or less match the feedback you gave me.
Comment 7 WebKit Commit Bot 2015-02-09 15:10:55 PST
Attachment 246294 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:221:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.h:67:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 WebKit Commit Bot 2015-02-10 16:56:22 PST
Comment on attachment 246294 [details]
Patch

Rejecting attachment 246294 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
leCachedEntryFailureKeyEv", referenced from:
      __ZN6WebKit22NetworkCacheStatistics26recordRetrievedCachedEntryEyRKNS_15NetworkCacheKeyERKN7WebCore15ResourceRequestEb in NetworkCacheStatisticsCocoa.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

** BUILD FAILED **


The following build commands failed:
	Ld /Volumes/Data/EWS/WebKit/WebKitBuild/Release/WebKit.framework/Versions/A/WebKit normal x86_64
(1 failure)

Full output: http://webkit-queues.appspot.com/results/5455962949287936
Comment 9 Chris Dumez 2015-02-10 17:47:55 PST
Created attachment 246358 [details]
Patch
Comment 10 WebKit Commit Bot 2015-02-10 17:49:20 PST
Attachment 246358 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:221:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.h:67:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 WebKit Commit Bot 2015-02-10 18:31:51 PST
Comment on attachment 246358 [details]
Patch

Clearing flags on attachment: 246358

Committed r179910: <http://trac.webkit.org/changeset/179910>
Comment 12 WebKit Commit Bot 2015-02-10 18:31:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Alexey Proskuryakov 2015-02-10 23:51:05 PST
This caused a number of assertion failures on debug bots, I'm going to roll out.

https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/r179910%20(2024)/results.html
Comment 14 WebKit Commit Bot 2015-02-10 23:56:38 PST
Re-opened since this is blocked by bug 141464
Comment 15 Chris Dumez 2015-02-11 09:45:20 PST
Comment on attachment 246358 [details]
Patch

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

> Source/WebKit2/UIProcess/Network/NetworkProcessProxy.cpp:236
> +    MESSAGE_CHECK(page);

So this is the assertion we are sometimes hitting on debug bots. This is likely because by the time we make the logging decision in our background thread, the page is gone.
Comment 16 Antti Koivisto 2015-02-11 10:15:47 PST
> So this is the assertion we are sometimes hitting on debug bots. This is
> likely because by the time we make the logging decision in our background
> thread, the page is gone.

Moving page related code to NetworkResourceLoader level may be a good idea. Those get canceled when the page dies.

It is somewhat unfortunate that logging facility has to know about pages at all. Cache is not associated with any page.
Comment 17 Chris Dumez 2015-02-11 11:32:41 PST
(In reply to comment #16)
> > So this is the assertion we are sometimes hitting on debug bots. This is
> > likely because by the time we make the logging decision in our background
> > thread, the page is gone.
> 
> Moving page related code to NetworkResourceLoader level may be a good idea.
> Those get canceled when the page dies.
> 
> It is somewhat unfortunate that logging facility has to know about pages at
> all. Cache is not associated with any page.

Agreed. I have just discussed this with Sam and he'll think about how we should refactor this. In the mean time though, I'll re-upload a patch that drops IPC messages for pages that are no longer in the map. We lose some logging for some rare cases but this is OK for now.
Comment 18 Chris Dumez 2015-02-11 11:45:33 PST
Created attachment 246403 [details]
Patch
Comment 19 WebKit Commit Bot 2015-02-11 11:47:57 PST
Attachment 246403 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:221:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/Network/NetworkProcessProxy.cpp:54:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.h:67:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Sam Weinig 2015-02-11 15:48:43 PST
Comment on attachment 246403 [details]
Patch

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

> Source/WebKit2/UIProcess/Network/NetworkProcessProxy.cpp:58
> +#define MESSAGE_CHECK_NO_ASSERT(condition) do \
> +    if (!(condition)) { \
> +        connection()->markCurrentlyDispatchedMessageAsInvalid(); \
> +        return; \
> +    } \
> +while (0)

I don't think you want this.  These messages are not invalid, it's just that the page is closed.  I would instead null-check the page, and put this comment near it.
Comment 21 Chris Dumez 2015-02-11 15:57:35 PST
Committed r179972: <http://trac.webkit.org/changeset/179972>
Comment 22 Chris Dumez 2015-02-11 16:13:08 PST
(In reply to comment #20)
> Comment on attachment 246403 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=246403&action=review
> 
> > Source/WebKit2/UIProcess/Network/NetworkProcessProxy.cpp:58
> > +#define MESSAGE_CHECK_NO_ASSERT(condition) do \
> > +    if (!(condition)) { \
> > +        connection()->markCurrentlyDispatchedMessageAsInvalid(); \
> > +        return; \
> > +    } \
> > +while (0)
> 
> I don't think you want this.  These messages are not invalid, it's just that
> the page is closed.  I would instead null-check the page, and put this
> comment near it.

Ok, replaced with a simple null-check before landing.