Remove the ConnCountImpact FieldTrial code.

Remove the ConnCountImpact FieldTrial code.

BUG=156873
TEST=No user visible changes.

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

[SteveT, 2012-10-19 18:59:55]

As discussed, starting to clean up various old Trials.

One question for you inline (feel free to chime in, Will, if you know the
answer).

Thanks!

https://codereview.chromium.org/11228007/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_field_trials.cc (left):

https://codereview.chromium.org/11228007/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_field_trials.cc:174: max_sockets = 6;
I'm guessing I need to set this as a constant somewhere in net/ code?

Could you point me to that code... save me a lot of digging :)

[willchan no longer on Chromium, 2012-10-19 19:04:27]

https://code.google.com/searchframe#OAMlx_jo-ck/src/net/socket/client_socket_...

I think that's it. It can probably be made constant then.

On Fri, Oct 19, 2012 at 11:59 AM,  <stevet@chromium.org> wrote:
> Reviewers: Matt Menke,
>
> Message:
> As discussed, starting to clean up various old Trials.
>
> One question for you inline (feel free to chime in, Will, if you know the
> answer).
>
> Thanks!
>
>
>
>
https://codereview.chromium.org/11228007/diff/1/chrome/browser/chrome_browser...
> File chrome/browser/chrome_browser_field_trials.cc (left):
>
>
https://codereview.chromium.org/11228007/diff/1/chrome/browser/chrome_browser...
> chrome/browser/chrome_browser_field_trials.cc:174: max_sockets = 6;
> I'm guessing I need to set this as a constant somewhere in net/ code?
>
> Could you point me to that code... save me a lot of digging :)
>
> Description:
> Remove the ConnCountImpact FieldTrial code.
>
> BUG=156873
> TEST=No user visible changes.
>
> Please review this at https://codereview.chromium.org/11228007/
>
> SVN Base: http://git.chromium.org/chromium/src.git@master
>
> Affected files:
>   M chrome/browser/chrome_browser_field_trials.h
>   M chrome/browser/chrome_browser_field_trials.cc
>   M chrome/renderer/page_load_histograms.cc
>   M net/http/http_network_transaction.cc
>
>

[mmenke, 2012-10-19 19:16:55]

It's already set to 6 by default, the function to set it just overrides the
defaults.

Looks like set_max_sockets_per_group is actually used in a couple unit tests. 
The calls look completely unnecessary to me, however, since those tests are
creating socket pools manually, with a specified max, anyways.

So looks to me like the function and all calls to it can safely be removed.

[SteveT, 2012-10-19 19:42:09]

On 2012/10/19 19:16:55, Matt Menke wrote:
> It's already set to 6 by default, the function to set it just overrides the
> defaults.

Just discovered that as you sent this email. Sounds like we don't have to set it
again elsewhere, then :)

> 
> Looks like set_max_sockets_per_group is actually used in a couple unit tests. 
> The calls look completely unnecessary to me, however, since those tests are
> creating socket pools manually, with a specified max, anyways.
> 
> So looks to me like the function and all calls to it can safely be removed.

Not as safe as we thought. I tried running the related unit test after removing
set_max_sockets_per_group and I hit a DCHECK:

[24285:24285:1019/152617:366863571222:FATAL:socket_test_util.h(522)] Check
failed: next_index_ < data_providers_.size() (1 vs. 1)
Backtrace:
	base::debug::StackTrace::StackTrace() [0x7f0898f27866]
	logging::LogMessage::~LogMessage() [0x7f0898f58fd1]
	net::SocketDataProviderArray<>::GetNext() [0xebb8ec]
	net::DeterministicMockClientSocketFactory::CreateTransportClientSocket()
[0xeb968f]
	net::TransportConnectJob::DoTransportConnect() [0x7f0898039a75]
	net::TransportConnectJob::DoLoop() [0x7f08980397b7]
	net::TransportConnectJob::OnIOComplete() [0x7f0898039588]
	base::internal::RunnableAdapter<>::Run() [0x7f089803cdbe]
	base::internal::InvokeHelper<>::MakeItSo() [0x7f089803cc55]
	base::internal::Invoker<>::Run() [0x7f089803ca76]
	base::Callback<>::Run() [0x7f0897d9f492]
	net::SingleRequestHostResolver::OnResolveCompletion() [0x7f0897e2face]
	base::internal::RunnableAdapter<>::Run() [0x7f0897e2fe2c]
	base::internal::InvokeHelper<>::MakeItSo() [0x7f0897e2fd95]
	base::internal::Invoker<>::Run() [0x7f0897e2fd28]
	base::Callback<>::Run() [0x621b60]
	net::MockHostResolverBase::ResolveNow() [0xe9e046]
	base::internal::RunnableAdapter<>::Run() [0xea1158]
	base::internal::InvokeHelper<>::MakeItSo() [0xea0d52]
	base::internal::Invoker<>::Run() [0xea0518]
	base::Callback<>::Run() [0x7f0898f1fe91]
	MessageLoop::RunTask() [0x7f0898f5de81]
	MessageLoop::DeferOrRunPendingTask() [0x7f0898f5df9c]
	MessageLoop::DoWork() [0x7f0898f5e833]
	base::MessagePumpLibevent::Run() [0x7f0898f0a4d3]
	MessageLoop::RunInternal() [0x7f0898f5da85]
	MessageLoop::RunHandler() [0x7f0898f5d93c]
	base::RunLoop::Run() [0x7f0898f9114c]
	MessageLoop::Run() [0x7f0898f5d270]
	net::internal::TestCompletionCallbackBaseInternal::WaitForResult() [0xea1a05]
	net::internal::TestCompletionCallbackTemplate<>::WaitForResult() [0x518a8c]
	net::(anonymous
namespace)::HttpPipelinedNetworkTransactionTest::CompleteFourRequests()
[0x9b5741]
	net::(anonymous
namespace)::HttpPipelinedNetworkTransactionTest_ReusesOnSpaceAvailable_Test::TestBody()
[0x9b6592]
	testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0xe7fb65]
	testing::internal::HandleExceptionsInMethodIfSupported<>() [0xe7d133]
	testing::Test::Run() [0xe72f25]
	testing::TestInfo::Run() [0xe73502]
	testing::TestCase::Run() [0xe739f4]
	testing::internal::UnitTestImpl::RunAllTests() [0xe78002]
	testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0xe80981]
	testing::internal::HandleExceptionsInMethodIfSupported<>() [0xe7d932]
	testing::UnitTest::Run() [0xe77167]
	base::TestSuite::Run() [0xe86ecb]
	main [0x5eb26b]
	0x7f0895dba76d
	0x4cd2a9


Anyways - not actually asking you to debug it for me. :)  If you think know what
this issue is, I can fix it and remove the method in this CL. Otherwise, I might
just leave that method alone for now.  I could also try to debug, but then I
wouldn't expect much progress on this CL for a while...

[SteveT, 2012-10-19 19:42:45]

(I've uploaded the broken patch for context)

[mmenke, 2012-10-19 19:44:59]

I'd suggest you just put it back in, and land patch set 1 (Which LGTM).  I'll
take a look at it at some point.

On 2012/10/19 19:42:09, SteveT wrote:
> On 2012/10/19 19:16:55, Matt Menke wrote:
> > It's already set to 6 by default, the function to set it just overrides the
> > defaults.
> 
> Just discovered that as you sent this email. Sounds like we don't have to set
it
> again elsewhere, then :)
> 
> > 
> > Looks like set_max_sockets_per_group is actually used in a couple unit
tests. 
> > The calls look completely unnecessary to me, however, since those tests are
> > creating socket pools manually, with a specified max, anyways.
> > 
> > So looks to me like the function and all calls to it can safely be removed.
> 
> Not as safe as we thought. I tried running the related unit test after
removing
> set_max_sockets_per_group and I hit a DCHECK:
> 
> [24285:24285:1019/152617:366863571222:FATAL:socket_test_util.h(522)] Check
> failed: next_index_ < data_providers_.size() (1 vs. 1)
> Backtrace:
> 	base::debug::StackTrace::StackTrace() [0x7f0898f27866]
> 	logging::LogMessage::~LogMessage() [0x7f0898f58fd1]
> 	net::SocketDataProviderArray<>::GetNext() [0xebb8ec]
> 	net::DeterministicMockClientSocketFactory::CreateTransportClientSocket()
> [0xeb968f]
> 	net::TransportConnectJob::DoTransportConnect() [0x7f0898039a75]
> 	net::TransportConnectJob::DoLoop() [0x7f08980397b7]
> 	net::TransportConnectJob::OnIOComplete() [0x7f0898039588]
> 	base::internal::RunnableAdapter<>::Run() [0x7f089803cdbe]
> 	base::internal::InvokeHelper<>::MakeItSo() [0x7f089803cc55]
> 	base::internal::Invoker<>::Run() [0x7f089803ca76]
> 	base::Callback<>::Run() [0x7f0897d9f492]
> 	net::SingleRequestHostResolver::OnResolveCompletion() [0x7f0897e2face]
> 	base::internal::RunnableAdapter<>::Run() [0x7f0897e2fe2c]
> 	base::internal::InvokeHelper<>::MakeItSo() [0x7f0897e2fd95]
> 	base::internal::Invoker<>::Run() [0x7f0897e2fd28]
> 	base::Callback<>::Run() [0x621b60]
> 	net::MockHostResolverBase::ResolveNow() [0xe9e046]
> 	base::internal::RunnableAdapter<>::Run() [0xea1158]
> 	base::internal::InvokeHelper<>::MakeItSo() [0xea0d52]
> 	base::internal::Invoker<>::Run() [0xea0518]
> 	base::Callback<>::Run() [0x7f0898f1fe91]
> 	MessageLoop::RunTask() [0x7f0898f5de81]
> 	MessageLoop::DeferOrRunPendingTask() [0x7f0898f5df9c]
> 	MessageLoop::DoWork() [0x7f0898f5e833]
> 	base::MessagePumpLibevent::Run() [0x7f0898f0a4d3]
> 	MessageLoop::RunInternal() [0x7f0898f5da85]
> 	MessageLoop::RunHandler() [0x7f0898f5d93c]
> 	base::RunLoop::Run() [0x7f0898f9114c]
> 	MessageLoop::Run() [0x7f0898f5d270]
> 	net::internal::TestCompletionCallbackBaseInternal::WaitForResult() [0xea1a05]
> 	net::internal::TestCompletionCallbackTemplate<>::WaitForResult() [0x518a8c]
> 	net::(anonymous
> namespace)::HttpPipelinedNetworkTransactionTest::CompleteFourRequests()
> [0x9b5741]
> 	net::(anonymous
>
namespace)::HttpPipelinedNetworkTransactionTest_ReusesOnSpaceAvailable_Test::TestBody()
> [0x9b6592]
> 	testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0xe7fb65]
> 	testing::internal::HandleExceptionsInMethodIfSupported<>() [0xe7d133]
> 	testing::Test::Run() [0xe72f25]
> 	testing::TestInfo::Run() [0xe73502]
> 	testing::TestCase::Run() [0xe739f4]
> 	testing::internal::UnitTestImpl::RunAllTests() [0xe78002]
> 	testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0xe80981]
> 	testing::internal::HandleExceptionsInMethodIfSupported<>() [0xe7d932]
> 	testing::UnitTest::Run() [0xe77167]
> 	base::TestSuite::Run() [0xe86ecb]
> 	main [0x5eb26b]
> 	0x7f0895dba76d
> 	0x4cd2a9
> 
> 
> Anyways - not actually asking you to debug it for me. :)  If you think know
what
> this issue is, I can fix it and remove the method in this CL. Otherwise, I
might
> just leave that method alone for now.  I could also try to debug, but then I
> wouldn't expect much progress on this CL for a while...

[commit-bot: I haz the power, 2012-10-19 20:00:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/11228007/1

[commit-bot: I haz the power, 2012-10-19 20:00:33]

Presubmit check for 11228007-1 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.1s to calculate.

[SteveT, 2012-10-19 20:16:28]

R+Ben for OWNERS approval in chrome/.

[SteveT, 2012-10-22 16:53:08]

Ping.

[SteveT, 2012-10-23 22:24:50]

Ping - Ben, can you take a quick OWNERS look?

[Ben Goodger (Google), 2012-10-23 22:57:46]

lgtm

[commit-bot: I haz the power, 2012-10-23 23:48:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/11228007/1

[commit-bot: I haz the power, 2012-10-24 01:58:59]

Change committed as 163750