Add the capability to run multiple proxy PAC scripts in parallel....

Add the capability to run multiple proxy PAC scripts in parallel.

Refactors SingleThreadedProxyResolver into MultiThreadedProxyResolver.

New threads are created lazily on demand, up to a fixed maximum.

Note that this CL does NOT change the policy in Chrome -- it will continue to use a single thread for proxy resolving, but using the new code to do so.

BUG=11079
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=51924

[eroman, 2010-07-01 23:25:03]

I have updated the CL now with the file renames.

[willchan no longer on Chromium, 2010-07-06 16:55:30]

On 2010/07/01 23:25:03, eroman wrote:
> I have updated the CL now with the file renames.

I'm looking at this, but just to double check, are the files being removed
properly?  I see that multi_threaded_proxy_resolver* looks like it's moved
properly, but I would expect that the deleted files be marked as deleted instead
of merged.

[willchan no longer on Chromium, 2010-07-06 21:01:59]

It took me a few hours to get 90% of this into memory.  I think I understand it
fairly well now.  Overall, it looks pretty good and will be a good improvement. 
I have quite a number of minor comments for you to look at.

http://codereview.chromium.org/2822043/diff/49001/50003
File net/proxy/multi_threaded_proxy_resolver.cc (right):

http://codereview.chromium.org/2822043/diff/49001/50003#newcode18
net/proxy/multi_threaded_proxy_resolver.cc:18: // TODO(eroman): If you call
GetProxyForURL() while executing inside of the
I think this is not worth fixing, unless you have a good way of doing it.  It
will only create a new thread if we're below the max number of threads anyway.

http://codereview.chromium.org/2822043/diff/49001/50003#newcode115
net/proxy/multi_threaded_proxy_resolver.cc:115: protected:
You've got protected twice.

http://codereview.chromium.org/2822043/diff/49001/50003#newcode121
net/proxy/multi_threaded_proxy_resolver.cc:121: Type type_;
const

http://codereview.chromium.org/2822043/diff/49001/50003#newcode143
net/proxy/multi_threaded_proxy_resolver.cc:143: const string16& pac_script)
Overloading like this is against the style guide.  You should just use the first
one, and choose the type based on callback != NULL.

http://codereview.chromium.org/2822043/diff/49001/50003#newcode157
net/proxy/multi_threaded_proxy_resolver.cc:157: origin_loop->PostTask(FROM_HERE,
style nit: argument indentation is off

http://codereview.chromium.org/2822043/diff/49001/50003#newcode172
net/proxy/multi_threaded_proxy_resolver.cc:172: MultiThreadedProxyResolver*
coordinator_;
Is this used?

http://codereview.chromium.org/2822043/diff/49001/50003#newcode173
net/proxy/multi_threaded_proxy_resolver.cc:173: CompletionCallback* callback_;
Is this used?

http://codereview.chromium.org/2822043/diff/49001/50003#newcode175
net/proxy/multi_threaded_proxy_resolver.cc:175: string16 pac_script_;
Should these be const?

http://codereview.chromium.org/2822043/diff/49001/50003#newcode179
net/proxy/multi_threaded_proxy_resolver.cc:179: MessageLoop* origin_loop_;
Is this used?

http://codereview.chromium.org/2822043/diff/49001/50003#newcode224
net/proxy/multi_threaded_proxy_resolver.cc:224: const size_t kLoadLogBound =
100u;
kNetLogBound.  And why is it so large?

http://codereview.chromium.org/2822043/diff/49001/50003#newcode280
net/proxy/multi_threaded_proxy_resolver.cc:280: thread_number_(thread_number) {
Maybe do DCHECKs on coordinator and resolver.

http://codereview.chromium.org/2822043/diff/49001/50003#newcode298
net/proxy/multi_threaded_proxy_resolver.cc:298:
thread_->message_loop()->PostTask(FROM_HERE,
Arguments on subsequent lines need to line up with the first argument.  You
should probably move FROM_HERE to the next line.

http://codereview.chromium.org/2822043/diff/49001/50003#newcode337
net/proxy/multi_threaded_proxy_resolver.cc:337:
thread_->message_loop()->PostTask(FROM_HERE,
style nit again on the argument indentation

http://codereview.chromium.org/2822043/diff/49001/50003#newcode375
net/proxy/multi_threaded_proxy_resolver.cc:375: << "Resolver is un-initiazlied.
Must call SetPacScript() first!";
initialized

http://codereview.chromium.org/2822043/diff/49001/50003#newcode387
net/proxy/multi_threaded_proxy_resolver.cc:387: if (executor) {
You should add a DCHECK here that pending_jobs_.empty()

http://codereview.chromium.org/2822043/diff/49001/50003#newcode397
net/proxy/multi_threaded_proxy_resolver.cc:397: // If we havent' already reached
the thread limit, provision a new thread to
haven't

http://codereview.chromium.org/2822043/diff/49001/50003#newcode428
net/proxy/multi_threaded_proxy_resolver.cc:428: DCHECK_EQ(0u,
pending_jobs_.size());
empty() is cheaper than size()

http://codereview.chromium.org/2822043/diff/49001/50003#newcode442
net/proxy/multi_threaded_proxy_resolver.cc:442: void
MultiThreadedProxyResolver::PurgeMemory() {
I wonder if you should free up idle executors here.  This could screw up your
thread ids though.

http://codereview.chromium.org/2822043/diff/49001/50003#newcode458
net/proxy/multi_threaded_proxy_resolver.cc:458: // being duplicated by each of
the resolver threads).
This is a bad assumption.  Newer compilers' std::string implementations don't
reference count anymore.  I don't think current versions of MSVC will, and gcc
4.5 or so switches to a non-ref counted impl, so when we switch to a new gcc
version for chrome releases, this will cause memory bloat there too.

http://codereview.chromium.org/2822043/diff/49001/50003#newcode478
net/proxy/multi_threaded_proxy_resolver.cc:478: CHECK_EQ(0u,
pending_jobs_.size());
empty() instead of size()

http://codereview.chromium.org/2822043/diff/49001/50003#newcode487
net/proxy/multi_threaded_proxy_resolver.cc:487: // has_user_callback() will be
false).
Isn't it supposed to also exclude jobs that aren't user initiated?

http://codereview.chromium.org/2822043/diff/49001/50003#newcode492
net/proxy/multi_threaded_proxy_resolver.cc:492: void
MultiThreadedProxyResolver::DeleteAllExecutors() {
My really minor preference is to rename the Delete/Destroy to Orphan()/Release()
or something, since they aren't immediately deleted afterward.  Up to you
though!

http://codereview.chromium.org/2822043/diff/49001/50004
File net/proxy/multi_threaded_proxy_resolver.h (right):

http://codereview.chromium.org/2822043/diff/49001/50004#newcode39
net/proxy/multi_threaded_proxy_resolver.h:39: bool resolvers_expect_pac_bytes_;
const?
DISALLOW_COPY_AND_ASSIGN? (don't forget basictypes.h for this)

http://codereview.chromium.org/2822043/diff/49001/50004#newcode44
net/proxy/multi_threaded_proxy_resolver.h:44: class MultiThreadedProxyResolver :
public ProxyResolver {
This is complicated enough that you should provide a class comment somewhere
that details the implementation at a high level.

http://codereview.chromium.org/2822043/diff/49001/50004#newcode44
net/proxy/multi_threaded_proxy_resolver.h:44: class MultiThreadedProxyResolver :
public ProxyResolver {
MultiThreadedProxyResolver shouldn't be thread safe, right?  Can we inherit from
NonThreadSafe?

http://codereview.chromium.org/2822043/diff/49001/50004#newcode46
net/proxy/multi_threaded_proxy_resolver.h:46: class Job;
Job should be private, right?

http://codereview.chromium.org/2822043/diff/49001/50004#newcode47
net/proxy/multi_threaded_proxy_resolver.h:47: friend class Job;
Do you find that you actually need this friend for our supported compiler
versions?  The C++ standard says "A nested class is a member and as such has the
same access rights as any other member."

http://www.open-std.org/JTC1/SC22/WG21/docs/cwg_defects.html#45

http://codereview.chromium.org/2822043/diff/49001/50004#newcode103
net/proxy/multi_threaded_proxy_resolver.h:103: scoped_ptr<ProxyResolverFactory>
resolver_factory_;
const

http://codereview.chromium.org/2822043/diff/49001/50004#newcode104
net/proxy/multi_threaded_proxy_resolver.h:104: size_t max_num_threads_;
const

http://codereview.chromium.org/2822043/diff/49001/50004#newcode115
net/proxy/multi_threaded_proxy_resolver.h:115: class
MultiThreadedProxyResolver::Executor
Can this class move into the .cc file?

http://codereview.chromium.org/2822043/diff/49001/50004#newcode150
net/proxy/multi_threaded_proxy_resolver.h:150: MultiThreadedProxyResolver*
coordinator_;
const pointer

http://codereview.chromium.org/2822043/diff/49001/50004#newcode151
net/proxy/multi_threaded_proxy_resolver.h:151: int thread_number_;
const

http://codereview.chromium.org/2822043/diff/49001/50004#newcode158
net/proxy/multi_threaded_proxy_resolver.h:158: scoped_ptr<ProxyResolver>
resolver_;
const scoped_ptr

http://codereview.chromium.org/2822043/diff/49001/50007
File net/proxy/proxy_resolver_js_bindings.cc (right):

http://codereview.chromium.org/2822043/diff/49001/50007#newcode116
net/proxy/proxy_resolver_js_bindings.cc:116: // Helper to execute a syncrhonous
DNS resolve, using the per-request
synchronous

http://codereview.chromium.org/2822043/diff/49001/50012
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/2822043/diff/49001/50012#newcode130
net/proxy/proxy_service.cc:130: sync_host_resolver_->Shutdown();
This is weird / breaks abstraction boundaries.  Isn't sync_host_resolver_ owned
by impl_?  Why not call impl_->Shutdown() and have DefaultJSBindings::Shutdown()
call Shutdown() on the host resolver?

[eroman, 2010-07-07 17:19:08]

Thanks for the careful review

http://codereview.chromium.org/2822043/diff/49001/50003
File net/proxy/multi_threaded_proxy_resolver.cc (right):

http://codereview.chromium.org/2822043/diff/49001/50003#newcode18
net/proxy/multi_threaded_proxy_resolver.cc:18: // TODO(eroman): If you call
GetProxyForURL() while executing inside of the
On 2010/07/06 21:01:59, willchan wrote:
> I think this is not worth fixing, unless you have a good way of doing it.  It
> will only create a new thread if we're below the max number of threads anyway.

Yeah I agree this isn't important, I have removed the TODO.

I may end up addressing this as a side-effect once I switch to using a priority
queue. (I decided against doing priority queueing as part of this CL to keep
things simpler, as there is a bunch of plumbing required).

http://codereview.chromium.org/2822043/diff/49001/50003#newcode115
net/proxy/multi_threaded_proxy_resolver.cc:115: protected:
On 2010/07/06 21:01:59, willchan wrote:
> You've got protected twice.

Done.

http://codereview.chromium.org/2822043/diff/49001/50003#newcode143
net/proxy/multi_threaded_proxy_resolver.cc:143: const string16& pac_script)
On 2010/07/06 21:01:59, willchan wrote:
> Overloading like this is against the style guide.  You should just use the
first
> one, and choose the type based on callback != NULL.

Done.

http://codereview.chromium.org/2822043/diff/49001/50003#newcode157
net/proxy/multi_threaded_proxy_resolver.cc:157: origin_loop->PostTask(FROM_HERE,
On 2010/07/06 21:01:59, willchan wrote:
> style nit: argument indentation is off

Done.

http://codereview.chromium.org/2822043/diff/49001/50003#newcode172
net/proxy/multi_threaded_proxy_resolver.cc:172: MultiThreadedProxyResolver*
coordinator_;
On 2010/07/06 21:01:59, willchan wrote:
> Is this used?

No its not. removed.

http://codereview.chromium.org/2822043/diff/49001/50003#newcode173
net/proxy/multi_threaded_proxy_resolver.cc:173: CompletionCallback* callback_;
On 2010/07/06 21:01:59, willchan wrote:
> Is this used?

No its not. removed.

http://codereview.chromium.org/2822043/diff/49001/50003#newcode175
net/proxy/multi_threaded_proxy_resolver.cc:175: string16 pac_script_;
On 2010/07/06 21:01:59, willchan wrote:
> Should these be const?

Done.

http://codereview.chromium.org/2822043/diff/49001/50003#newcode179
net/proxy/multi_threaded_proxy_resolver.cc:179: MessageLoop* origin_loop_;
On 2010/07/06 21:01:59, willchan wrote:
> Is this used?

Grr! Sorry about that, forgot to slim this down after extracting to the base
class.

http://codereview.chromium.org/2822043/diff/49001/50003#newcode224
net/proxy/multi_threaded_proxy_resolver.cc:224: const size_t kLoadLogBound =
100u;
On 2010/07/06 21:01:59, willchan wrote:
> kNetLogBound.  And why is it so large?

It is somewhat arbitrary. Basically it buffers the messages, so they can be
added to the netlog on completion on the IO thread (so the total bound will
actually be enforced when copying to the netlog).

That said, I have lowered this number to 50.

(Was just cut pasting the code from earlier).

http://codereview.chromium.org/2822043/diff/49001/50003#newcode280
net/proxy/multi_threaded_proxy_resolver.cc:280: thread_number_(thread_number) {
On 2010/07/06 21:01:59, willchan wrote:
> Maybe do DCHECKs on coordinator and resolver.

Done.

http://codereview.chromium.org/2822043/diff/49001/50003#newcode298
net/proxy/multi_threaded_proxy_resolver.cc:298:
thread_->message_loop()->PostTask(FROM_HERE,
On 2010/07/06 21:01:59, willchan wrote:
> Arguments on subsequent lines need to line up with the first argument.  You
> should probably move FROM_HERE to the next line.

Done.

http://codereview.chromium.org/2822043/diff/49001/50003#newcode337
net/proxy/multi_threaded_proxy_resolver.cc:337:
thread_->message_loop()->PostTask(FROM_HERE,
On 2010/07/06 21:01:59, willchan wrote:
> style nit again on the argument indentation

Copy-paste bites me all over on this one.

http://codereview.chromium.org/2822043/diff/49001/50003#newcode375
net/proxy/multi_threaded_proxy_resolver.cc:375: << "Resolver is un-initiazlied.
Must call SetPacScript() first!";
On 2010/07/06 21:02:00, willchan wrote:
> initialized

Done.

http://codereview.chromium.org/2822043/diff/49001/50003#newcode387
net/proxy/multi_threaded_proxy_resolver.cc:387: if (executor) {
On 2010/07/06 21:02:00, willchan wrote:
> You should add a DCHECK here that pending_jobs_.empty()

Done.

http://codereview.chromium.org/2822043/diff/49001/50003#newcode397
net/proxy/multi_threaded_proxy_resolver.cc:397: // If we havent' already reached
the thread limit, provision a new thread to
On 2010/07/06 21:02:00, willchan wrote:
> haven't

Done.

http://codereview.chromium.org/2822043/diff/49001/50003#newcode428
net/proxy/multi_threaded_proxy_resolver.cc:428: DCHECK_EQ(0u,
pending_jobs_.size());
On 2010/07/06 21:02:00, willchan wrote:
> empty() is cheaper than size()

I prefer sticking with size() unless you feel strongly about this.  (I like the
extra information of knowing the size on failure).

As far as performance goes, since |pending_jobs_| *should* be empty, it should
still be cheap :)

http://codereview.chromium.org/2822043/diff/49001/50003#newcode442
net/proxy/multi_threaded_proxy_resolver.cc:442: void
MultiThreadedProxyResolver::PurgeMemory() {
On 2010/07/06 21:02:00, willchan wrote:
> I wonder if you should free up idle executors here.  This could screw up your
> thread ids though.

Yeah I had the exact same thought :)

Ideally I would purge the idle threads, but it messes with some of my other
expectations. I will stick a TODO.

http://codereview.chromium.org/2822043/diff/49001/50003#newcode458
net/proxy/multi_threaded_proxy_resolver.cc:458: // being duplicated by each of
the resolver threads).
On 2010/07/06 21:02:00, willchan wrote:
> This is a bad assumption.  Newer compilers' std::string implementations don't
> reference count anymore.  I don't think current versions of MSVC will, and gcc
> 4.5 or so switches to a non-ref counted impl, so when we switch to a new gcc
> version for chrome releases, this will cause memory bloat there too.

I tested on our various platforms of chrome, and found they all reference count.
There are many places in chrome that will be hurt by having a non-reference
counted string implementation. Even just all the message passing we do with
strings.

Do you suggest I manually allocate the string to be resilient to this? Or add
some sort of compile-time assert (if so how).

There is another place in proxy_resolver_v8 which relies on this for its
external-string shim.

http://codereview.chromium.org/2822043/diff/49001/50003#newcode478
net/proxy/multi_threaded_proxy_resolver.cc:478: CHECK_EQ(0u,
pending_jobs_.size());
On 2010/07/06 21:02:00, willchan wrote:
> empty() instead of size()

Same comment as earlier -- I think the performance impact is negligeable when
the container is empty. Moreover, this is only called once per browser session
(typically).

http://codereview.chromium.org/2822043/diff/49001/50003#newcode487
net/proxy/multi_threaded_proxy_resolver.cc:487: // has_user_callback() will be
false).
On 2010/07/06 21:02:00, willchan wrote:
> Isn't it supposed to also exclude jobs that aren't user initiated?

Yes, that is exactly correct.

I have reworded the comment to be more broad:

// The "has_user_callback()" is to exclude jobs for which the callback
// has already been invoked, or was not user-initiated (as in the case of
// lazy thread provisions). User-initiated jobs may !has_user_callback()
// when the callback has already been run. (Since we only clear the
// outstanding job AFTER the callback has been invoked, it is possible
// for a new request to be started from within the callback).

http://codereview.chromium.org/2822043/diff/49001/50003#newcode492
net/proxy/multi_threaded_proxy_resolver.cc:492: void
MultiThreadedProxyResolver::DeleteAllExecutors() {
On 2010/07/06 21:02:00, willchan wrote:
> My really minor preference is to rename the Delete/Destroy to
Orphan()/Release()
> or something, since they aren't immediately deleted afterward.  Up to you
> though!

Good idea.

Then I can remove that awkward comment I have about it not being strictly a
"delete".

http://codereview.chromium.org/2822043/diff/49001/50004
File net/proxy/multi_threaded_proxy_resolver.h (right):

http://codereview.chromium.org/2822043/diff/49001/50004#newcode44
net/proxy/multi_threaded_proxy_resolver.h:44: class MultiThreadedProxyResolver :
public ProxyResolver {
On 2010/07/06 21:02:00, willchan wrote:
> MultiThreadedProxyResolver shouldn't be thread safe, right?  Can we inherit
from
> NonThreadSafe?

Done.

http://codereview.chromium.org/2822043/diff/49001/50004#newcode44
net/proxy/multi_threaded_proxy_resolver.h:44: class MultiThreadedProxyResolver :
public ProxyResolver {
On 2010/07/06 21:02:00, willchan wrote:
> This is complicated enough that you should provide a class comment somewhere
> that details the implementation at a high level.

Done.

http://codereview.chromium.org/2822043/diff/49001/50004#newcode46
net/proxy/multi_threaded_proxy_resolver.h:46: class Job;
On 2010/07/06 21:02:00, willchan wrote:
> Job should be private, right?

Done.

(I had originally made it public to avoid needing to forward declare the
subtypes as inner classes)

http://codereview.chromium.org/2822043/diff/49001/50004#newcode47
net/proxy/multi_threaded_proxy_resolver.h:47: friend class Job;
On 2010/07/06 21:02:00, willchan wrote:
> Do you find that you actually need this friend for our supported compiler
> versions?  The C++ standard says "A nested class is a member and as such has
the
> same access rights as any other member."
> 
> http://www.open-std.org/JTC1/SC22/WG21/docs/cwg_defects.html#45

Done.

http://codereview.chromium.org/2822043/diff/49001/50004#newcode103
net/proxy/multi_threaded_proxy_resolver.h:103: scoped_ptr<ProxyResolverFactory>
resolver_factory_;
On 2010/07/06 21:02:00, willchan wrote:
> const

Done.

http://codereview.chromium.org/2822043/diff/49001/50004#newcode104
net/proxy/multi_threaded_proxy_resolver.h:104: size_t max_num_threads_;
On 2010/07/06 21:02:00, willchan wrote:
> const

Done.

http://codereview.chromium.org/2822043/diff/49001/50004#newcode115
net/proxy/multi_threaded_proxy_resolver.h:115: class
MultiThreadedProxyResolver::Executor
On 2010/07/06 21:02:00, willchan wrote:
> Can this class move into the .cc file?

Done. Good idea.

http://codereview.chromium.org/2822043/diff/49001/50004#newcode150
net/proxy/multi_threaded_proxy_resolver.h:150: MultiThreadedProxyResolver*
coordinator_;
On 2010/07/06 21:02:00, willchan wrote:
> const pointer

I can't, since this is used to call coordinator_->OnExecutorReady(), which is
not const.

http://codereview.chromium.org/2822043/diff/49001/50004#newcode151
net/proxy/multi_threaded_proxy_resolver.h:151: int thread_number_;
On 2010/07/06 21:02:00, willchan wrote:
> const

Done.

http://codereview.chromium.org/2822043/diff/49001/50004#newcode158
net/proxy/multi_threaded_proxy_resolver.h:158: scoped_ptr<ProxyResolver>
resolver_;
On 2010/07/06 21:02:00, willchan wrote:
> const scoped_ptr

Can't, as I call resolver_.reset() during destroy.

http://codereview.chromium.org/2822043/diff/49001/50007
File net/proxy/proxy_resolver_js_bindings.cc (right):

http://codereview.chromium.org/2822043/diff/49001/50007#newcode116
net/proxy/proxy_resolver_js_bindings.cc:116: // Helper to execute a syncrhonous
DNS resolve, using the per-request
On 2010/07/06 21:02:00, willchan wrote:
> synchronous

Done.

http://codereview.chromium.org/2822043/diff/49001/50012
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/2822043/diff/49001/50012#newcode130
net/proxy/proxy_service.cc:130: sync_host_resolver_->Shutdown();
On 2010/07/06 21:02:00, willchan wrote:
> This is weird / breaks abstraction boundaries.  Isn't sync_host_resolver_
owned
> by impl_?  Why not call impl_->Shutdown() and have
DefaultJSBindings::Shutdown()
> call Shutdown() on the host resolver?

Yeah this is a mess. I had a layering problem making this fit together.

DefaultJSBindings::Shutdown() cannot call SyncHostResolverBridge::Shutdown()
since it takes a HostResolver* (and there is no HostResolver::Shutdown()).

There is a HostResolverImpl::Shutdown() method, but I am reluctant to push that
into the base class. I can if you want, but it would be formalizing the hack (I
guess that hack is never going away anyway :(

Otherwise, I think I can rationalize this a bit by smashing together some of the
existing concepts
(SyncHostResolverBridge/DefaultJSBindings/ProxyResolverJSBindingsUsingSyncHostResolver).

Basically what I am thinking, is to split out this class to its own file, and
give it a better name, like "ProxyResolverJSBindingsForV8". The existing
"DefaultJSBindings" stuff can be merged into this (just need to provide a simple
mock for the unit-tests now). And lastly, the SyncHostResolverBridge can be
internalized into ProxyResolverJSBindingsForV8, since that is all it is used for
anyway.

If you are OK with that plan, I can do it as a follow-up while addressing
crbug.com/47226 (I wanted my own subclass of the bindings anyway in order to
tackle issues like this).

[willchan no longer on Chromium, 2010-07-07 18:15:36]

http://codereview.chromium.org/2822043/diff/49001/50003
File net/proxy/multi_threaded_proxy_resolver.cc (right):

http://codereview.chromium.org/2822043/diff/49001/50003#newcode458
net/proxy/multi_threaded_proxy_resolver.cc:458: // being duplicated by each of
the resolver threads).
On 2010/07/07 17:19:08, eroman wrote:
> On 2010/07/06 21:02:00, willchan wrote:
> > This is a bad assumption.  Newer compilers' std::string implementations
don't
> > reference count anymore.  I don't think current versions of MSVC will, and
gcc
> > 4.5 or so switches to a non-ref counted impl, so when we switch to a new gcc
> > version for chrome releases, this will cause memory bloat there too.
> 
> I tested on our various platforms of chrome, and found they all reference
count.
> There are many places in chrome that will be hurt by having a non-reference
> counted string implementation. Even just all the message passing we do with
> strings.

I know that for Mac and Linux, our default compilers all reference count.  But
gcc is definitely switching, so when we upgrade, we won't have COW strings
anymore.  From my reading, visual studio 2008 doesn't use COW strings either
(http://stackoverflow.com/questions/637695/how-efficient-is-stdstring-compared...).
 Are you using 2005 or 2008?

> 
> Do you suggest I manually allocate the string to be resilient to this? Or add
> some sort of compile-time assert (if so how).

I suggest manually allocating the string.

> 
> There is another place in proxy_resolver_v8 which relies on this for its
> external-string shim.

http://codereview.chromium.org/2822043/diff/49001/50004
File net/proxy/multi_threaded_proxy_resolver.h (right):

http://codereview.chromium.org/2822043/diff/49001/50004#newcode150
net/proxy/multi_threaded_proxy_resolver.h:150: MultiThreadedProxyResolver*
coordinator_;
On 2010/07/07 17:19:08, eroman wrote:
> On 2010/07/06 21:02:00, willchan wrote:
> > const pointer
> 
> I can't, since this is used to call coordinator_->OnExecutorReady(), which is
> not const.

I meant MultithreadedProxyResolver* const coordinator_;

http://codereview.chromium.org/2822043/diff/49001/50012
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/2822043/diff/49001/50012#newcode130
net/proxy/proxy_service.cc:130: sync_host_resolver_->Shutdown();
On 2010/07/07 17:19:08, eroman wrote:
> On 2010/07/06 21:02:00, willchan wrote:
> > This is weird / breaks abstraction boundaries.  Isn't sync_host_resolver_
> owned
> > by impl_?  Why not call impl_->Shutdown() and have
> DefaultJSBindings::Shutdown()
> > call Shutdown() on the host resolver?
> 
> Yeah this is a mess. I had a layering problem making this fit together.
> 
> DefaultJSBindings::Shutdown() cannot call SyncHostResolverBridge::Shutdown()
> since it takes a HostResolver* (and there is no HostResolver::Shutdown()).
> 
> There is a HostResolverImpl::Shutdown() method, but I am reluctant to push
that
> into the base class. I can if you want, but it would be formalizing the hack
(I
> guess that hack is never going away anyway :(
> 
> Otherwise, I think I can rationalize this a bit by smashing together some of
the
> existing concepts
>
(SyncHostResolverBridge/DefaultJSBindings/ProxyResolverJSBindingsUsingSyncHostResolver).
> 
> Basically what I am thinking, is to split out this class to its own file, and
> give it a better name, like "ProxyResolverJSBindingsForV8". The existing
> "DefaultJSBindings" stuff can be merged into this (just need to provide a
simple
> mock for the unit-tests now). And lastly, the SyncHostResolverBridge can be
> internalized into ProxyResolverJSBindingsForV8, since that is all it is used
for
> anyway.
> 
> If you are OK with that plan, I can do it as a follow-up while addressing
> crbug.com/47226 (I wanted my own subclass of the bindings anyway in order to
> tackle issues like this).

Sorry, I don't understand how combining the classes solves the issue.  If it
does though, then I'm fine with it.  Otherwise, I think you should formalize the
Shutdown() hack and make it part of the interface class.

http://codereview.chromium.org/2822043/diff/48002/11007
File net/proxy/multi_threaded_proxy_resolver.h (right):

http://codereview.chromium.org/2822043/diff/48002/11007#newcode41
net/proxy/multi_threaded_proxy_resolver.h:41: bool resolvers_expect_pac_bytes_;
const

http://codereview.chromium.org/2822043/diff/48002/11008
File net/proxy/multi_threaded_proxy_resolver_unittest.cc (right):

http://codereview.chromium.org/2822043/diff/48002/11008#newcode658
net/proxy/multi_threaded_proxy_resolver_unittest.cc:658: 
It seems like it'd be nice to show that the multithreaded resolver does not
suffer from the same head of line blocking problem as the single threaded one. 
Can you have 2 requests and 2 threads, where the 1st request never completes,
but the 2nd one completes?

[eroman, 2010-07-07 20:57:21]

http://codereview.chromium.org/2822043/diff/49001/50003
File net/proxy/multi_threaded_proxy_resolver.cc (right):

http://codereview.chromium.org/2822043/diff/49001/50003#newcode458
net/proxy/multi_threaded_proxy_resolver.cc:458: // being duplicated by each of
the resolver threads).
On 2010/07/07 18:15:36, willchan wrote:
> On 2010/07/07 17:19:08, eroman wrote:
> > On 2010/07/06 21:02:00, willchan wrote:
> > > This is a bad assumption.  Newer compilers' std::string implementations
> don't
> > > reference count anymore.  I don't think current versions of MSVC will, and
> gcc
> > > 4.5 or so switches to a non-ref counted impl, so when we switch to a new
gcc
> > > version for chrome releases, this will cause memory bloat there too.
> > 
> > I tested on our various platforms of chrome, and found they all reference
> count.
> > There are many places in chrome that will be hurt by having a non-reference
> > counted string implementation. Even just all the message passing we do with
> > strings.
> 
> I know that for Mac and Linux, our default compilers all reference count.  But
> gcc is definitely switching, so when we upgrade, we won't have COW strings
> anymore.  From my reading, visual studio 2008 doesn't use COW strings either
>
(http://stackoverflow.com/questions/637695/how-efficient-is-stdstring-compared...).
>  Are you using 2005 or 2008?
> 
> > 
> > Do you suggest I manually allocate the string to be resilient to this? Or
add
> > some sort of compile-time assert (if so how).
> 
> I suggest manually allocating the string.
> 
> > 
> > There is another place in proxy_resolver_v8 which relies on this for its
> > external-string shim.
> 
> 

That will add some non-trivial other changes to this change, as I will need to
plumb some "refcounted string" parameter throughout the API (or do some extra
copies).

I will hold off on that, and in the meantime send out some feelers on
chromium-dev about this issue. I think it will be a pretty serious problem if
the underlying string implementation stops being COW, since we do loads of
passing of strings between message loops and containers, for largely immutable
content.

If we ever do pull in such change, we will need some sort of "refcounted
std::string / refcounted wstring / refcounted string16" as part of base; I don't
want to craft a weird one-off here.

http://codereview.chromium.org/2822043/diff/49001/50004
File net/proxy/multi_threaded_proxy_resolver.h (right):

http://codereview.chromium.org/2822043/diff/49001/50004#newcode150
net/proxy/multi_threaded_proxy_resolver.h:150: MultiThreadedProxyResolver*
coordinator_;
On 2010/07/07 18:15:36, willchan wrote:
> On 2010/07/07 17:19:08, eroman wrote:
> > On 2010/07/06 21:02:00, willchan wrote:
> > > const pointer
> > 
> > I can't, since this is used to call coordinator_->OnExecutorReady(), which
is
> > not const.
> 
> I meant MultithreadedProxyResolver* const coordinator_;

I have left as-is, since I also do |coordinator_ = NULL| during destroy.

http://codereview.chromium.org/2822043/diff/48002/11008
File net/proxy/multi_threaded_proxy_resolver_unittest.cc (right):

http://codereview.chromium.org/2822043/diff/48002/11008#newcode658
net/proxy/multi_threaded_proxy_resolver_unittest.cc:658: 
On 2010/07/07 18:15:36, willchan wrote:
> It seems like it'd be nice to show that the multithreaded resolver does not
> suffer from the same head of line blocking problem as the single threaded one.

> Can you have 2 requests and 2 threads, where the 1st request never completes,
> but the 2nd one completes?

Done: MultiThreadedProxyResolverTest.OneThreadBlocked

[willchan no longer on Chromium, 2010-07-07 21:30:25]

http://codereview.chromium.org/2822043/diff/49001/50003
File net/proxy/multi_threaded_proxy_resolver.cc (right):

http://codereview.chromium.org/2822043/diff/49001/50003#newcode458
net/proxy/multi_threaded_proxy_resolver.cc:458: // being duplicated by each of
the resolver threads).
On 2010/07/07 20:57:21, eroman wrote:
> On 2010/07/07 18:15:36, willchan wrote:
> > On 2010/07/07 17:19:08, eroman wrote:
> > > On 2010/07/06 21:02:00, willchan wrote:
> > > > This is a bad assumption.  Newer compilers' std::string implementations
> > don't
> > > > reference count anymore.  I don't think current versions of MSVC will,
and
> > gcc
> > > > 4.5 or so switches to a non-ref counted impl, so when we switch to a new
> gcc
> > > > version for chrome releases, this will cause memory bloat there too.
> > > 
> > > I tested on our various platforms of chrome, and found they all reference
> > count.
> > > There are many places in chrome that will be hurt by having a
non-reference
> > > counted string implementation. Even just all the message passing we do
with
> > > strings.
> > 
> > I know that for Mac and Linux, our default compilers all reference count. 
But
> > gcc is definitely switching, so when we upgrade, we won't have COW strings
> > anymore.  From my reading, visual studio 2008 doesn't use COW strings either
> >
>
(http://stackoverflow.com/questions/637695/how-efficient-is-stdstring-compared...).
> >  Are you using 2005 or 2008?
> > 
> > > 
> > > Do you suggest I manually allocate the string to be resilient to this? Or
> add
> > > some sort of compile-time assert (if so how).
> > 
> > I suggest manually allocating the string.
> > 
> > > 
> > > There is another place in proxy_resolver_v8 which relies on this for its
> > > external-string shim.
> > 
> > 
> 
> That will add some non-trivial other changes to this change, as I will need to
> plumb some "refcounted string" parameter throughout the API (or do some extra
> copies).

I'm fine with holding off on this change.

> 
> I will hold off on that, and in the meantime send out some feelers on
> chromium-dev about this issue. I think it will be a pretty serious problem if
> the underlying string implementation stops being COW, since we do loads of
> passing of strings between message loops and containers, for largely immutable
> content.

Don't we do const std::string& for passing of strings between message loops?  If
not, that sounds like a bug.  We should be doing string copies and relying on it
being ref-counted.

> 
> If we ever do pull in such change, we will need some sort of "refcounted
> std::string / refcounted wstring / refcounted string16" as part of base; I
don't
> want to craft a weird one-off here.

Fair enough.  Personally, I think we should just use shared_ptr<const
std::string>, but I think I've lost that battle.  Also, do you know what our
releases are built with?  If 2008 is the default, then perhaps we've already got
non-COW strings on Windows?  Perhaps we should check with maruel or someone.

http://codereview.chromium.org/2822043/diff/48002/11008
File net/proxy/multi_threaded_proxy_resolver_unittest.cc (right):

http://codereview.chromium.org/2822043/diff/48002/11008#newcode658
net/proxy/multi_threaded_proxy_resolver_unittest.cc:658: 
On 2010/07/07 20:57:21, eroman wrote:
> On 2010/07/07 18:15:36, willchan wrote:
> > It seems like it'd be nice to show that the multithreaded resolver does not
> > suffer from the same head of line blocking problem as the single threaded
one.
> 
> > Can you have 2 requests and 2 threads, where the 1st request never
completes,
> > but the 2nd one completes?
> 
> Done: MultiThreadedProxyResolverTest.OneThreadBlocked

Have you uploaded the new patch yet?

[eroman, 2010-07-07 21:48:44]

http://codereview.chromium.org/2822043/diff/48002/11008
File net/proxy/multi_threaded_proxy_resolver_unittest.cc (right):

http://codereview.chromium.org/2822043/diff/48002/11008#newcode658
net/proxy/multi_threaded_proxy_resolver_unittest.cc:658: 
On 2010/07/07 21:30:25, willchan wrote:
> On 2010/07/07 20:57:21, eroman wrote:
> > On 2010/07/07 18:15:36, willchan wrote:
> > > It seems like it'd be nice to show that the multithreaded resolver does
not
> > > suffer from the same head of line blocking problem as the single threaded
> one.
> > 
> > > Can you have 2 requests and 2 threads, where the 1st request never
> completes,
> > > but the 2nd one completes?
> > 
> > Done: MultiThreadedProxyResolverTest.OneThreadBlocked
> 
> Have you uploaded the new patch yet?

It had gotten stuck at upload. Should be up now.

[willchan no longer on Chromium, 2010-07-07 22:36:21]

LGTM