Add performance tests for the proxy resolver implementations....

Add performance tests for the proxy resolver implementations.

I downloaded the test data "no-ads.pac" from <http://ap.coova.org/wpad.dat>;.

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

[eroman, 2009-03-05 02:07:20]

I ran this test, and the results were:

ProxyResolverWinHttp_no-ads.pac 1818    ms
ProxyResolverV8_no-ads.pac      70      ms

Which is showing the V8-based implementation to be *26* times faster than the
WinHTTP implementation!

The average latency per request (500 iterations) was:
ProxyResolverWinHttp: 3.6ms
ProxyResolverV8: 0.14ms

So we are shaving off 3+ ms per url request, for this particular PAC script.

[wtc, 2009-03-05 19:04:54]

LGTM.  A nice wrapup of your excellent work on the V8 proxy
resolver!

Is the net_perftests not being built on Mac and Linux?  You
didn't change their build files.

http://codereview.chromium.org/40149/diff/1/6
File chrome/chrome.sln (right):

http://codereview.chromium.org/40149/diff/1/6#newcode575
Line 575: {21E22961-22BF-4493-BD3A-868F93DA5179} =
{21E22961-22BF-4493-BD3A-868F93DA5179}
Do you know what these changes are?  There are no
files names, just long hex strings that look like GUIDs.

http://codereview.chromium.org/40149/diff/1/3
File net/proxy/proxy_resolver_perftest.cc (right):

http://codereview.chromium.org/40149/diff/1/3#newcode15
Line 15: // TODO(eroman): Maybe ssl_test_util.cc should be the one to include
this
Yes!  This pragma should be added to the file that includes
<wincrypt.h> and references functions like CertCloseStore.

http://codereview.chromium.org/40149/diff/1/3#newcode33
Line 33: // |queries| should be terminated by {NULL, NULL}. A sentinal is used
Nit: "sentinal" => "sentinel"

http://codereview.chromium.org/40149/diff/1/3#newcode39
Line 39: // Returns the actual number of entries in |queries| (assumes NULL
sentinal).
Nit: "sentinal" => "sentinel"

http://codereview.chromium.org/40149/diff/1/3#newcode40
Line 40: int num_queries() const;
Nit: NumQueries, since this is not a simple accessor?

http://codereview.chromium.org/40149/diff/1/3#newcode44
Line 44: static PacPerfTest kPerfTests[] = {
'static' => 'const'

http://codereview.chromium.org/40149/diff/1/3#newcode46
Line 46: // regular expression oriented, and has no dependencies on the current
Nit: "current" => "local"?

http://codereview.chromium.org/40149/diff/1/3#newcode53
Line 53: {"http://www.staples.com/", "DIRECT"},
Is this a test to see if I'm reviewing carefully?  Or do you
really buy office supplies that often? :-)

http://codereview.chromium.org/40149/diff/1/3#newcode67
Line 67: int count = 0;
Nit: remove 'count'.  Does VC++ not warn about this?

http://codereview.chromium.org/40149/diff/1/3#newcode86
Line 86: resolver_(resolver), resolver_name_(resolver_name) {
Nit: the colon (:) that precedes the initializer list should
be on this line:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Initializer_Lists

http://codereview.chromium.org/40149/diff/1/3#newcode105
Line 105: InitHttpServer();
Would it be better to call InitHttpServer() eagerly in the
constructor?

http://codereview.chromium.org/40149/diff/1/3#newcode159
Line 159: path = path.AppendASCII("net");
Just wondering: do we have to append one component at a time?

[eroman, 2009-03-06 00:07:46]

> Is the net_perftests not being built on Mac and Linux? 
> You didn't change their build files.

Unfortunately neither the try server nor chromium builders build net_perftests
(AFAICT).

Hence I am proceeding carefully to make sure I test the change on each platform
before adding.

This is a little more error prone process, so I would prefer to add as a second
CL. Is that OK?

http://codereview.chromium.org/40149/diff/1/6
File chrome/chrome.sln (right):

http://codereview.chromium.org/40149/diff/1/6#newcode575
Line 575: {21E22961-22BF-4493-BD3A-868F93DA5179} =
{21E22961-22BF-4493-BD3A-868F93DA5179}
On 2009/03/05 19:04:55, wtc wrote:
> Do you know what these changes are?  There are no
> files names, just long hex strings that look like GUIDs.

I had to add some project dependencies to appease the linker:

-----------
net.sln
-----------

Added the following dependencies to net_perftests (4):

+ bzip2
+ v8
+ v8_base
+ zlib

-----------
chrome.sln
-----------

Added the following dependencies to net_perftests (5):

+ bzip2
+ v8
+ v8_base
+ sdch
+ zlib

http://codereview.chromium.org/40149/diff/1/3
File net/proxy/proxy_resolver_perftest.cc (right):

http://codereview.chromium.org/40149/diff/1/3#newcode15
Line 15: // TODO(eroman): Maybe ssl_test_util.cc should be the one to include
this
On 2009/03/05 19:04:55, wtc wrote:
> Yes!  This pragma should be added to the file that includes
> <wincrypt.h> and references functions like CertCloseStore.

Done.

http://codereview.chromium.org/40149/diff/1/3#newcode33
Line 33: // |queries| should be terminated by {NULL, NULL}. A sentinal is used
On 2009/03/05 19:04:55, wtc wrote:
> Nit: "sentinal" => "sentinel"

Done.

http://codereview.chromium.org/40149/diff/1/3#newcode39
Line 39: // Returns the actual number of entries in |queries| (assumes NULL
sentinal).
On 2009/03/05 19:04:55, wtc wrote:
> Nit: "sentinal" => "sentinel"

Done.

http://codereview.chromium.org/40149/diff/1/3#newcode40
Line 40: int num_queries() const;
On 2009/03/05 19:04:55, wtc wrote:
> Nit: NumQueries, since this is not a simple accessor?

Done.

http://codereview.chromium.org/40149/diff/1/3#newcode53
Line 53: {"http://www.staples.com/", "DIRECT"},
On 2009/03/05 19:04:55, wtc wrote:
> Is this a test to see if I'm reviewing carefully?  Or do you
> really buy office supplies that often? :-)

I crafted these URLs in order to trace out different execution paths through the
PAC script.

In the case of "staples.com", this PAC script does both a domain check and a
path check, so I have different path variants.

Good spot though, I do have "/pixeltracker/x" twice -- I will go ahead and
change that to something else.

http://codereview.chromium.org/40149/diff/1/3#newcode67
Line 67: int count = 0;
On 2009/03/05 19:04:55, wtc wrote:
> Nit: remove 'count'.  Does VC++ not warn about this?

D'oh!

http://codereview.chromium.org/40149/diff/1/3#newcode86
Line 86: resolver_(resolver), resolver_name_(resolver_name) {
On 2009/03/05 19:04:55, wtc wrote:
> Nit: the colon (:) that precedes the initializer list should
> be on this line:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Initializer_Lists
> 

Done.

http://codereview.chromium.org/40149/diff/1/3#newcode105
Line 105: InitHttpServer();
On 2009/03/05 19:04:55, wtc wrote:
> Would it be better to call InitHttpServer() eagerly in the
> constructor?

I find it easier to read having that logic right here, as opposed to splitting.

Performance-wise doesn't matter much, since this happens before we start timing.

I can change this if you prefer.

http://codereview.chromium.org/40149/diff/1/3#newcode159
Line 159: path = path.AppendASCII("net");
On 2009/03/05 19:04:55, wtc wrote:
> Just wondering: do we have to append one component at a time?

Unfortunatly, AFAICT yes.

the FilePath object was designed for cross-platform portability, so it abstracts
away the path separator.

[wtc, 2009-03-06 00:18:49]

On 2009/03/06 00:07:46, eroman wrote:
> > Is the net_perftests not being built on Mac and Linux? 
> > You didn't change their build files.
> 
> Unfortunately neither the try server nor chromium builders build net_perftests
> (AFAICT).
> 
> Hence I am proceeding carefully to make sure I test the change on each
platform
> before adding.
> 
> This is a little more error prone process, so I would prefer to add as a
second
> CL. Is that OK?

Of course, it is.

LGTM!