Chromium Code Reviews
Help | Chromium Project | Sign in
(3)

Issue 15076: Clean up dns prefetch code, and also port it. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 2 months ago by Paweł Hajdan Jr.
Modified:
5 years, 9 months ago
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

Clean up dns prefetch code, and also port it. - remove slave threads and use HostResolver in asynchronous mode instead (while still limiting number of concurrent lookups) - make the implementation portable and make DnsMaster unit test compile and pass on Linux - add more tests to DnsMaster unit test to simulate various shutdown scenarios, concurrent lookups, and to verify that we don't exceed our limit of concurrent lookup requests) - remove some tests which relied on specifics of slaves' inner working - adjust initialization and shutdown of dns prefetching (now it relies on the IO message loop being present) Bonus: shutdown is almost instant now, no need to have a timeout. BUG=5687, 6683

Patch Set 1 #

Total comments: 2

Patch Set 2 : '' #

Patch Set 3 : '' #

Total comments: 7

Patch Set 4 : switching to HostResolver and resulting adjustments #

Patch Set 5 : post slave task in the right place #

Patch Set 6 : post slave task in the right place #

Patch Set 7 : work in progress, asking for help #

Total comments: 13

Patch Set 8 : just use HostResolver in async mode, no slaves #

Total comments: 7

Patch Set 9 : fixes #

Total comments: 2

Patch Set 10 : fixes #

Patch Set 11 : reduce flakiness (happened on linux trybot) #

Total comments: 6

Patch Set 12 : more fixes, more tests #

Patch Set 13 : sync with trunk, uploading latest version to be sure #

Patch Set 14 : wip; sync with trunk, still crashes windows #

Patch Set 15 : re-upload, previous try gave me http 502 #

Patch Set 16 : now with CancelableRequest #

Patch Set 17 : simplification, and it works! #

Patch Set 18 : WIP; changing startup/shutdown; using PostTask on IO thread #

Patch Set 19 : updated startup & shutdown #

Patch Set 20 : re-upload, had 502 Bad Gateway #

Total comments: 1

Patch Set 21 : earlier startup, fixed shutdown, register prefs on POSIX #

Total comments: 3

Patch Set 22 : minor fixes #

Patch Set 23 : use scoper for init & free #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+418 lines, -825 lines) Patch
M chrome/browser/browser.scons View 1 2 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +0 lines, -5 lines 0 comments Download
M chrome/browser/browser.vcproj View 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +0 lines, -8 lines 0 comments Download
M chrome/browser/browser_main.cc View 18 19 20 21 22 4 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/browser_prefs.cc View 4 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/browser_process_impl.cc View 18 19 20 21 2 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/chrome_plugin_host.cc View 18 19 20 21 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/net/dns_global.h View 18 19 20 21 22 2 chunks +12 lines, -2 lines 0 comments Download
M chrome/browser/net/dns_global.cc View 1 2 9 10 11 12 13 14 15 16 17 18 19 20 21 2 chunks +7 lines, -8 lines 0 comments Download
M chrome/browser/net/dns_host_info.h View 2 chunks +5 lines, -6 lines 0 comments Download
M chrome/browser/net/dns_host_info.cc View 10 11 12 13 14 15 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/net/dns_host_info_unittest.cc View 10 11 12 13 14 15 2 chunks +2 lines, -4 lines 0 comments Download
M chrome/browser/net/dns_master.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 5 chunks +56 lines, -101 lines 1 comment Download
M chrome/browser/net/dns_master.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 9 chunks +116 lines, -202 lines 0 comments Download
M chrome/browser/net/dns_master_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 14 chunks +192 lines, -285 lines 0 comments Download
M chrome/browser/net/dns_slave.h View 1 2 3 4 5 6 1 chunk +0 lines, -72 lines 0 comments Download
M chrome/browser/net/dns_slave.cc View 1 2 3 4 5 6 1 chunk +0 lines, -96 lines 0 comments Download
M chrome/chrome.xcodeproj/project.pbxproj View 19 20 5 chunks +6 lines, -0 lines 0 comments Download
M chrome/common/temp_scaffolding_stubs.cc View 18 19 20 4 chunks +3 lines, -23 lines 0 comments Download
M chrome/test/unit/unit_tests.scons View 1 2 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 2 chunks +1 line, -1 line 0 comments Download
M net/base/host_resolver.h View 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +3 lines, -4 lines 0 comments Download
Trybot results:
Commit queue not available (can’t edit this change).

Messages

Total messages: 49 (0 generated)
Paweł Hajdan Jr.
There are some bad things in this patch I don't quite know what do to ...
8 years, 2 months ago (2008-12-19 10:47:09 UTC) #1
Dean McNamee
Yeah, ok, this patch is too hacky/slashy, and too much at once to review. Could ...
8 years, 2 months ago (2008-12-19 10:58:21 UTC) #2
Evan Martin
+agl (who's been doing some hack'n'slash as well, where Dean's comments might help)
8 years, 2 months ago (2008-12-19 17:51:48 UTC) #3
Evan Martin
For my change to make render_process_host build, I also had to remove an include of ...
8 years, 2 months ago (2008-12-19 17:53:21 UTC) #4
Paweł Hajdan Jr.
Submitted http://codereview.chromium.org/14919 .
8 years, 2 months ago (2008-12-20 10:17:01 UTC) #5
Paweł Hajdan Jr.
On 2008/12/19 10:58:21, Dean McNamee wrote: > Yeah, ok, this patch is too hacky/slashy, and ...
8 years, 1 month ago (2009-01-02 20:01:17 UTC) #6
Evan Martin
Just making sure this patch isn't lost -- I think Dean remains the right person ...
8 years, 1 month ago (2009-01-05 21:23:52 UTC) #7
Dean McNamee
This looks mostly OK to me, but this is Jar's code so you're going to ...
8 years, 1 month ago (2009-01-06 12:19:23 UTC) #8
jar (doing other things)
http://codereview.chromium.org/15076/diff/601/802 File chrome/browser/net/dns_master.cc (left): http://codereview.chromium.org/15076/diff/601/802#oldcode437 Line 437: ResumeThread(handle); // WINAPI call. I don't see the ...
8 years, 1 month ago (2009-01-06 18:37:39 UTC) #9
Paweł Hajdan Jr.
On 2009/01/06 18:37:39, jar wrote: > ...on the other hand... if the ported call actually ...
8 years, 1 month ago (2009-01-06 18:44:47 UTC) #10
Paweł Hajdan Jr.
ping. Darin, do you have some ideas about MessageLoop and the discussion above?
8 years, 1 month ago (2009-01-12 18:01:37 UTC) #11
darin (slow to review)
I'm not sure I understood the question about MessageLoops... http://codereview.chromium.org/15076/diff/601/802 File chrome/browser/net/dns_master.cc (right): http://codereview.chromium.org/15076/diff/601/802#newcode455 Line ...
8 years, 1 month ago (2009-01-12 18:31:50 UTC) #12
Paweł Hajdan Jr.
On 2009/01/12 18:31:50, darin wrote: > I'm not sure I understood the question about MessageLoops... ...
8 years, 1 month ago (2009-01-12 18:41:11 UTC) #13
darin (slow to review)
OK, I understand now. Taking a step back, I think the right answer is to ...
8 years, 1 month ago (2009-01-12 18:50:51 UTC) #14
jar (doing other things)
Sorry I didn't send this sooner. I wrote it up over a week ago... but ...
8 years, 1 month ago (2009-01-15 20:37:26 UTC) #15
Paweł Hajdan Jr.
On 2009/01/12 18:50:51, darin wrote: > OK, I understand now. Taking a step back, I ...
8 years, 1 month ago (2009-01-20 07:59:44 UTC) #16
darin (slow to review)
I'm a little confused by the usage of Thread and HostResolver. You can use HostResolver ...
8 years, 1 month ago (2009-01-20 17:45:30 UTC) #17
darin (slow to review)
http://codereview.chromium.org/15076/diff/1022/1024 File chrome/browser/net/dns_master.cc (right): http://codereview.chromium.org/15076/diff/1022/1024#newcode22 Line 22: class DnsMaster::LookupCallback : public net::CompletionCallback, the more canonical ...
8 years, 1 month ago (2009-01-20 21:09:13 UTC) #18
jar (doing other things)
http://codereview.chromium.org/15076/diff/1022/1024 File chrome/browser/net/dns_master.cc (right): http://codereview.chromium.org/15076/diff/1022/1024#newcode63 Line 63: AutoLock auto_lock(lock_); All the results_[] contents could be ...
8 years, 1 month ago (2009-01-20 21:19:46 UTC) #19
Paweł Hajdan Jr.
This is the version with no slaves, and just async HostResolvers. It limits the number ...
8 years, 1 month ago (2009-01-21 10:05:33 UTC) #20
Dean McNamee
Just some minor style nits. I will leave the real review up to darin/jar. Btw, ...
8 years, 1 month ago (2009-01-21 13:23:30 UTC) #21
darin (slow to review)
http://codereview.chromium.org/15076/diff/1434/1436 File chrome/browser/net/dns_master.cc (right): http://codereview.chromium.org/15076/diff/1434/1436#newcode368 Line 368: net::HostResolver* resolver = new net::HostResolver(); nit: it might ...
8 years, 1 month ago (2009-01-21 16:08:20 UTC) #22
darin (slow to review)
> > the more canonical way to use CompletionCallback is to have a member variable ...
8 years, 1 month ago (2009-01-21 16:11:13 UTC) #23
darin (slow to review)
Taking both of my suggestion together, you might just end up with a Lookup class ...
8 years, 1 month ago (2009-01-21 16:12:41 UTC) #24
Paweł Hajdan Jr.
Dean: Thanks for your comments. I hopefully fixed the nits. I also tweaked my .emacs ...
8 years, 1 month ago (2009-01-21 18:00:22 UTC) #25
darin (slow to review)
http://codereview.chromium.org/15076/diff/1451/1455 File chrome/browser/net/dns_master.cc (right): http://codereview.chromium.org/15076/diff/1451/1455#newcode39 Line 39: if (revoked()) even when revoked, we probably still ...
8 years, 1 month ago (2009-01-21 18:09:41 UTC) #26
Paweł Hajdan Jr.
On 2009/01/21 18:09:41, darin wrote: > http://codereview.chromium.org/15076/diff/1451/1455 > File chrome/browser/net/dns_master.cc (right): > > http://codereview.chromium.org/15076/diff/1451/1455#newcode39 > ...
8 years, 1 month ago (2009-01-21 18:42:36 UTC) #27
jar (doing other things)
Here are a pile of comments. One of the big ones discusses how to go ...
8 years, 1 month ago (2009-01-21 20:51:18 UTC) #28
jar (doing other things)
Nice direction! I look forward to seeing if you can complete the transition to using ...
8 years, 1 month ago (2009-01-22 01:03:00 UTC) #29
Paweł Hajdan Jr.
On 2009/01/22 01:03:00, jar wrote: > http://codereview.chromium.org/15076/diff/1480/1487 > File chrome/browser/net/dns_master.cc (right): > > http://codereview.chromium.org/15076/diff/1480/1487#newcode369 > ...
8 years, 1 month ago (2009-01-22 16:21:26 UTC) #30
Paweł Hajdan Jr.
ping.
8 years, 1 month ago (2009-01-23 06:25:08 UTC) #31
darin (slow to review)
LGTM Please keep an eye on the purify bots after checking in. We might need ...
8 years, 1 month ago (2009-01-23 18:04:55 UTC) #32
Paweł Hajdan Jr.
Switched to CancelableRequest. It made earlier crashes also appear consistently on Linux, which I fixed ...
8 years ago (2009-02-02 21:02:12 UTC) #33
Paweł Hajdan Jr.
This is code after simplification we talked about on irc. No leaks, and passes trybot ...
8 years ago (2009-02-03 12:44:27 UTC) #34
Dean McNamee
I'm going to leave this to Darin/Jim. I looked over it briefly and it seems ...
8 years ago (2009-02-03 12:51:09 UTC) #35
Dean McNamee
On 2009/02/03 12:51:09, Dean McNamee wrote: > I'm going to leave this to Darin/Jim. I ...
8 years ago (2009-02-03 13:36:48 UTC) #36
Paweł Hajdan Jr.
Dean: thanks for good suggestions. I updated the description. I tested the BUG syntax in ...
8 years ago (2009-02-04 10:59:03 UTC) #37
darin (slow to review)
I have no further comments. This LGTM !
8 years ago (2009-02-05 22:42:03 UTC) #38
Paweł Hajdan Jr.
I started changing the init/shutdown and PostTask things we discussed on irc (this is not ...
8 years ago (2009-02-10 17:47:32 UTC) #39
darin (slow to review)
it looks like you are using PostTask to get over to the io thread properly. ...
8 years ago (2009-02-10 18:26:20 UTC) #40
Evan Stade
ping what's the status on this patch? (just curious)
8 years ago (2009-02-13 20:20:29 UTC) #41
Evan Stade
nevermind, I'm dumb.
8 years ago (2009-02-13 20:21:35 UTC) #42
Paweł Hajdan Jr.
Patch updated. Tested on trybot, I couldn't test on windows compiled browser for now. Please ...
8 years ago (2009-02-16 19:00:32 UTC) #43
jar (doing other things)
I'm pretty wary of the relocation of the intialization code. I think it will give ...
8 years ago (2009-02-16 20:36:43 UTC) #44
Paweł Hajdan Jr.
On 2009/02/16 20:36:43, jar wrote: > I'm pretty wary of the relocation of the intialization ...
8 years ago (2009-02-16 20:54:33 UTC) #45
Paweł Hajdan Jr.
Darin, can you look again? I think the issues we discussed should be fixed now. ...
8 years ago (2009-02-18 16:36:09 UTC) #46
darin (slow to review)
LGTM http://codereview.chromium.org/15076/diff/3450/2519 File chrome/browser/browser_process_impl.cc (right): http://codereview.chromium.org/15076/diff/3450/2519#newcode178 Line 178: // Shutdown dns prefetching just before IO ...
8 years ago (2009-02-18 19:28:27 UTC) #47
Paweł Hajdan Jr.
On 2009/02/18 19:28:27, darin wrote: > // Shutdown DNS prefetching now to ensure that network ...
8 years ago (2009-02-18 21:28:20 UTC) #48
darin (slow to review)
8 years ago (2009-02-18 21:49:17 UTC) #49
LGTM

quick, ship it!

http://codereview.chromium.org/15076/diff/3473/2570
File chrome/browser/net/dns_master.h (right):

http://codereview.chromium.org/15076/diff/3473/2570#newcode145
Line 145: // Synchronize access to variables listed below.
looks like a good comment to me.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f8e48bd