Hand craft an A/B test of SDCH compression...

Hand craft an A/B test of SDCH compression

After we're sure we can do SDCH compression to a given URL, toss
a 50-50 coin each time we have a chance to advertise SDCH, and
either completely avoid advertisement, or advertise (including
the dictionary).  Histogram both compression download times, as
well as the download times for the "completely avoid" case.

http://crbug.com/11236
bug=11236
r=wtc,openvcdiff

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

[jar (doing other things), 2009-04-29 16:07:42]

I had asked Wan Teh to code review this, but I forgot to CC Ken (so this missive
takes care of that).  I would typically have also asked Lincoln to review... but
he mentioned he's *trying* to scale back, so I'm just cc'ing him in case he
feels like adding to the review.

Note that this change should theoretically not impact SDCH performance, but
really just moves around instrumentation (histogram recording) so that I can get
comparable stats when running/disabling SDCH (on a per transaction basis).

[Lincoln, 2009-04-29 21:42:40]

http://codereview.chromium.org/100004/diff/1148/1151
File net/base/filter.h (right):

http://codereview.chromium.org/100004/diff/1148/1151#newcode53
Line 53: enum STATISTIC_SELECTOR {
This looked at first like it should be made into two separate values or into a
bitfield (i.e. SDCH_DECODE | SDCH_EXPERIMENT).  Have you considered those
options?  If you're sure there will never be more than these four options then
the current approach is acceptable.

http://codereview.chromium.org/100004/diff/1148/1152
File net/base/load_flags.h (right):

http://codereview.chromium.org/100004/diff/1148/1152#newcode80
Line 80: // While doing SDCH latency tests, we'll need to flags to mark
participation
Possible typo: to flags to => two flags to

http://codereview.chromium.org/100004/diff/1148/1149
File net/base/sdch_filter.h (right):

http://codereview.chromium.org/100004/diff/1148/1149#newcode35
Line 35: ~SdchFilter();
"virtual" is correct.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Inheritance
"If your class has virtual methods, its destructor should be virtual. [...] When
redefining an inherited virtual function, explicitly declare it virtual in the
declaration of the derived class."

http://codereview.chromium.org/100004/diff/1148/1155
File net/base/sdch_manager.h (right):

http://codereview.chromium.org/100004/diff/1148/1155#newcode306
Line 306: // round trip before starting our comparitive tests.  If ever we
encounter
nit: comparitive => comparative

http://codereview.chromium.org/100004/diff/1148/1156
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/100004/diff/1148/1156#newcode224
Line 224: return 0 != (request_info_.load_flags &
net::LOAD_SDCH_DICTIONARY_ADVERTISED);
Isn't the "0 !=" understood, as in the previous revision?

http://codereview.chromium.org/100004/diff/1148/1156#newcode564
Line 564: // The AllowLaencyExperiment() is only true if we've successfully done
a
Typo: AllowLaencyExperiment => AllowLatencyExperiment

http://codereview.chromium.org/100004/diff/1148/1156#newcode572
Line 572: sdch_holdback = true;  // Don't fully support sdch this time.
You could just set advertise_sdch to false here and eliminate sdch_holdback
entirely.

http://codereview.chromium.org/100004/diff/1148/1156#newcode586
Line 586: request_info_.extra_headers += "Accept-Encoding:
gzip,deflate,bzip2\r\n";
You might wish to make "gzip,deflate,bzip2" a string constant to reduce
duplication.

http://codereview.chromium.org/100004/diff/1148/1156#newcode591
Line 591: if (avail_dictionaries.length() > 0) {
The previous version used !avail_dictionaries.empty().  Why the change, here and
on line 568?

http://codereview.chromium.org/100004/diff/1148/1158
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/100004/diff/1148/1158#newcode599
Line 599: UMA_HISTOGRAM_CLIPPED_TIMES("Sdch.Network_Decode_1st_To_Last_a",
In url_request_http_job.cc, EnablePacketTimes() is only called if the client is
going to participate in the latency experiment.  Here, the histogram data is
only sent if the client did *not* participate in the experiment.  Is it just me,
or is the logic backwards?

http://codereview.chromium.org/100004/diff/1148/1157
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/100004/diff/1148/1157#newcode357
Line 357: std::vector<base::Time> packet_times_;
Please add the #include corresponding to base::Time.

[wtc, 2009-04-29 21:57:40]

http://codereview.chromium.org/100004/diff/1148/1151
File net/base/filter.h (right):

http://codereview.chromium.org/100004/diff/1148/1151#newcode53
Line 53: enum STATISTIC_SELECTOR {
STATISTIC_SELECTOR doesn't conform to our naming convention
of types.  We use all caps for enumeration constants, but
the enumeration type should be named in CamelCaps
(StatisticSelector).

http://codereview.chromium.org/100004/diff/1148/1151#newcode199
Line 199: virtual void finalize() { if (next_filter_.get())
next_filter_->finalize(); }
The method name "finalize" should be capitalized.

We should write the if statement in two lines.
  if (next_filter_.get())
    next_filter_->finalize();

Question: Can we just destroy the filter chain when you
want to call the Finalize() method?

http://codereview.chromium.org/100004/diff/1148/1152
File net/base/load_flags.h (right):

http://codereview.chromium.org/100004/diff/1148/1152#newcode80
Line 80: // While doing SDCH latency tests, we'll need to flags to mark
participation
"to flags" => "two flags"

Load flags are set by the creator of a URLRequest.  Since
you set and test the LOAD_SDCH_TEST_xxx flags in
URLRequestHttpJob, you should just add two bool members to
URLRequestHttpJob.

This applies to the LOAD_SDCH_DICTIONARY_ADVERTISED flag
you added before.  It is also only set and tested in
URLRequestHttpJob, and therefore should also be a bool
member of URLRequestHttpJob.

http://codereview.chromium.org/100004/diff/1148/1149
File net/base/sdch_filter.h (left):

http://codereview.chromium.org/100004/diff/1148/1149#oldcode35
Line 35: virtual ~SdchFilter();
You deleted 'virtual' accidentally.  Please remmeber to add
it back.

http://codereview.chromium.org/100004/diff/1148/1149
File net/base/sdch_filter.h (right):

http://codereview.chromium.org/100004/diff/1148/1149#newcode48
Line 48: // Rceord histograms if needed.
Typo: "Rceord" => "Record"

http://codereview.chromium.org/100004/diff/1148/1154
File net/base/sdch_manager.cc (right):

http://codereview.chromium.org/100004/diff/1148/1154#newcode522
Line 522: if (enable)
Nit: in the net module we use braces {} when there is an
'else'.

http://codereview.chromium.org/100004/diff/1148/1155
File net/base/sdch_manager.h (right):

http://codereview.chromium.org/100004/diff/1148/1155#newcode136
Line 136: // defunct = 93,           // Over 10 minutes: No timing stats
recorded.
Nit: do you want to use the original enum constant name
OVER_10_MINUTES here, as you do above?

http://codereview.chromium.org/100004/diff/1148/1155#newcode306
Line 306: // round trip before starting our comparitive tests.  If ever we
encounter
Typo: comparitive => comparative

http://codereview.chromium.org/100004/diff/1148/1156
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/100004/diff/1148/1156#newcode555
Line 555: // probably an img or such. (and SDCH encoding is not likely).
Nit: remove the '.' after "such".

http://codereview.chromium.org/100004/diff/1148/1156#newcode562
Line 562: &avail_dictionaries);
Nit: over-indented.

http://codereview.chromium.org/100004/diff/1148/1156#newcode564
Line 564: // The AllowLaencyExperiment() is only true if we've successfully done
a
Typo: Laency

http://codereview.chromium.org/100004/diff/1148/1156#newcode570
Line 570: EnablePacketCounting(5);
Is this magic constant 5 linked to the magic constant 4 used
in URLRequestJob::RecordPacketStats?

http://codereview.chromium.org/100004/diff/1148/1158
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/100004/diff/1148/1158#newcode49
Line 49: filter_->finalize();
Why can't you just destroy the filter instead?
  filter_.reset();

Then we don't need to add the Finalize() method to Filter.

http://codereview.chromium.org/100004/diff/1148/1158#newcode136
Line 136: UpdatePacketReadTimes();
It's strange that you update packet read times *before*
reading.  Why don't you do that after reading?

http://codereview.chromium.org/100004/diff/1148/1158#newcode550
Line 550: void URLRequestJob::UpdatePacketReadTimes() {
Please have another person (mbelshe?) look at this function
also.

http://codereview.chromium.org/100004/diff/1148/1158#newcode672
Line 672: DCHECK(false);
Nit: use NOTREACHED() instead.

http://codereview.chromium.org/100004/diff/1148/1157
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/100004/diff/1148/1157#newcode333
Line 333: // used for histograms etc., and is not required.  It is optinally
gather
Typo: optinally

Change "gather" to "gathered"

http://codereview.chromium.org/100004/diff/1148/1157#newcode334
Line 334: // basde on the settings of several control varibles.
Typo: varibles.

http://codereview.chromium.org/100004/diff/1148/1157#newcode337
Line 337: void UpdatePacketReadTimes();
The style guide requires us to list the methods together,
so this UpdatePacketReadTimes method needs to be moved up.

http://codereview.chromium.org/100004/diff/1148/1157#newcode340
Line 340: // at a lower point in the network stack, assuring we have actual
packet
The issue is the HTTP cache and the SSL layers.

http://codereview.chromium.org/100004/diff/1148/1157#newcode368
Line 368: // We're ignoring overflow, as 1430 x 2^32 is a LOT of bytes.
Define 1430.

Nit: since you're using a signed int, it should be 2^31.

[jar (doing other things), 2009-04-30 03:41:09]

Excellent review by both of you (Wan Teh and Lincoln)!!! You both made some
excellent cateches.  Thank you very much.

All your suggestions were addressed.

Please have another look.

Jim

http://codereview.chromium.org/100004/diff/1148/1151
File net/base/filter.h (right):

http://codereview.chromium.org/100004/diff/1148/1151#newcode53
Line 53: enum STATISTIC_SELECTOR {
The selector is intended to be for one of N completely arbitrary histogram
emission groups.  I anticipate that new groups will be added, and called from
various places.  In this code, there are two that are part of experiments, and
two that are part of routine statistics gathering.


On 2009/04/29 21:42:40, Lincoln wrote:
> This looked at first like it should be made into two separate values or into a
> bitfield (i.e. SDCH_DECODE | SDCH_EXPERIMENT).  Have you considered those
> options?  If you're sure there will never be more than these four options then
> the current approach is acceptable.

http://codereview.chromium.org/100004/diff/1148/1151#newcode53
Line 53: enum STATISTIC_SELECTOR {
On 2009/04/29 21:57:40, wtc wrote:
> STATISTIC_SELECTOR doesn't conform to our naming convention
> of types.  We use all caps for enumeration constants, but
> the enumeration type should be named in CamelCaps
> (StatisticSelector).

Done.

http://codereview.chromium.org/100004/diff/1148/1151#newcode199
Line 199: virtual void finalize() { if (next_filter_.get())
next_filter_->finalize(); }
I hadn't thought of destroying the chain... and it is only now viable after all
the evolution this CL went through (before you saw it).  

I'll change to not using the finalize, and instead use reset as you suggested.


On 2009/04/29 21:57:40, wtc wrote:
> The method name "finalize" should be capitalized.
> 
> We should write the if statement in two lines.
>   if (next_filter_.get())
>     next_filter_->finalize();
> 
> Question: Can we just destroy the filter chain when you
> want to call the Finalize() method?

http://codereview.chromium.org/100004/diff/1148/1152
File net/base/load_flags.h (right):

http://codereview.chromium.org/100004/diff/1148/1152#newcode80
Line 80: // While doing SDCH latency tests, we'll need to flags to mark
participation
On 2009/04/29 21:42:40, Lincoln wrote:
> Possible typo: to flags to => two flags to

Switched to booleans in URLRequestHttpJob per wtc suggestions.

http://codereview.chromium.org/100004/diff/1148/1152#newcode80
Line 80: // While doing SDCH latency tests, we'll need to flags to mark
participation
On 2009/04/29 21:57:40, wtc wrote:
> "to flags" => "two flags"
> 
> Load flags are set by the creator of a URLRequest.  Since
> you set and test the LOAD_SDCH_TEST_xxx flags in
> URLRequestHttpJob, you should just add two bool members to
> URLRequestHttpJob.
> 
> This applies to the LOAD_SDCH_DICTIONARY_ADVERTISED flag
> you added before.  It is also only set and tested in
> URLRequestHttpJob, and therefore should also be a bool
> member of URLRequestHttpJob.

Done.

http://codereview.chromium.org/100004/diff/1148/1149
File net/base/sdch_filter.h (left):

http://codereview.chromium.org/100004/diff/1148/1149#oldcode35
Line 35: virtual ~SdchFilter();
On 2009/04/29 21:57:40, wtc wrote:
> You deleted 'virtual' accidentally.  Please remmeber to add
> it back.

Done.

http://codereview.chromium.org/100004/diff/1148/1149
File net/base/sdch_filter.h (right):

http://codereview.chromium.org/100004/diff/1148/1149#newcode35
Line 35: ~SdchFilter();
On 2009/04/29 21:42:40, Lincoln wrote:
> "virtual" is correct.
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Inheritance
> "If your class has virtual methods, its destructor should be virtual. [...]
When
> redefining an inherited virtual function, explicitly declare it virtual in the
> declaration of the derived class."

Done.

http://codereview.chromium.org/100004/diff/1148/1149#newcode48
Line 48: // Rceord histograms if needed.
On 2009/04/29 21:57:40, wtc wrote:
> Typo: "Rceord" => "Record"

Finalize was removed.

http://codereview.chromium.org/100004/diff/1148/1154
File net/base/sdch_manager.cc (right):

http://codereview.chromium.org/100004/diff/1148/1154#newcode522
Line 522: if (enable)
On 2009/04/29 21:57:40, wtc wrote:
> Nit: in the net module we use braces {} when there is an
> 'else'.

Done.

http://codereview.chromium.org/100004/diff/1148/1155
File net/base/sdch_manager.h (right):

http://codereview.chromium.org/100004/diff/1148/1155#newcode136
Line 136: // defunct = 93,           // Over 10 minutes: No timing stats
recorded.
On 2009/04/29 21:57:40, wtc wrote:
> Nit: do you want to use the original enum constant name
> OVER_10_MINUTES here, as you do above?

Done.

http://codereview.chromium.org/100004/diff/1148/1155#newcode306
Line 306: // round trip before starting our comparitive tests.  If ever we
encounter
On 2009/04/29 21:42:40, Lincoln wrote:
> nit: comparitive => comparative

Done.

http://codereview.chromium.org/100004/diff/1148/1155#newcode306
Line 306: // round trip before starting our comparitive tests.  If ever we
encounter
On 2009/04/29 21:57:40, wtc wrote:
> Typo: comparitive => comparative

Done.

http://codereview.chromium.org/100004/diff/1148/1156
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/100004/diff/1148/1156#newcode224
Line 224: return 0 != (request_info_.load_flags &
net::LOAD_SDCH_DICTIONARY_ADVERTISED);
On 2009/04/29 21:42:40, Lincoln wrote:
> Isn't the "0 !=" understood, as in the previous revision?

The result of the boolean operator produced a bool in the old code.  This new
code produced an int... which caused a warning... which precluded a compile.  So
I added the compare.

http://codereview.chromium.org/100004/diff/1148/1156#newcode555
Line 555: // probably an img or such. (and SDCH encoding is not likely).
On 2009/04/29 21:57:40, wtc wrote:
> Nit: remove the '.' after "such".

Done.

http://codereview.chromium.org/100004/diff/1148/1156#newcode562
Line 562: &avail_dictionaries);
On 2009/04/29 21:57:40, wtc wrote:
> Nit: over-indented.

Done.

http://codereview.chromium.org/100004/diff/1148/1156#newcode564
Line 564: // The AllowLaencyExperiment() is only true if we've successfully done
a
On 2009/04/29 21:42:40, Lincoln wrote:
> Typo: AllowLaencyExperiment => AllowLatencyExperiment

Done.

http://codereview.chromium.org/100004/diff/1148/1156#newcode564
Line 564: // The AllowLaencyExperiment() is only true if we've successfully done
a
On 2009/04/29 21:57:40, wtc wrote:
> Typo: Laency

Done.

http://codereview.chromium.org/100004/diff/1148/1156#newcode570
Line 570: EnablePacketCounting(5);
Yup.  Magic is the same.  I put in a constant, and added DCHECKs (in
RecordPacketStats) to ensure the constant is consistent.

On 2009/04/29 21:57:40, wtc wrote:
> Is this magic constant 5 linked to the magic constant 4 used
> in URLRequestJob::RecordPacketStats?

http://codereview.chromium.org/100004/diff/1148/1156#newcode572
Line 572: sdch_holdback = true;  // Don't fully support sdch this time.
On 2009/04/29 21:42:40, Lincoln wrote:
> You could just set advertise_sdch to false here and eliminate sdch_holdback
> entirely.

Done.

http://codereview.chromium.org/100004/diff/1148/1156#newcode586
Line 586: request_info_.extra_headers += "Accept-Encoding:
gzip,deflate,bzip2\r\n";
On 2009/04/29 21:42:40, Lincoln wrote:
> You might wish to make "gzip,deflate,bzip2" a string constant to reduce
> duplication.

Although I hate duplication... this seemed more readable, and avoided a run-time
cost of the string appends.

http://codereview.chromium.org/100004/diff/1148/1156#newcode591
Line 591: if (avail_dictionaries.length() > 0) {
On 2009/04/29 21:42:40, Lincoln wrote:
> The previous version used !avail_dictionaries.empty().  Why the change, here
and
> on line 568?

I can't recall how it got changed... so I switched it back.  thx!

I also changed 568 (above) to this non-empty format.

http://codereview.chromium.org/100004/diff/1148/1158
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/100004/diff/1148/1158#newcode49
Line 49: filter_->finalize();
On 2009/04/29 21:57:40, wtc wrote:
> Why can't you just destroy the filter instead?
>   filter_.reset();
> 
> Then we don't need to add the Finalize() method to Filter.

Done.

http://codereview.chromium.org/100004/diff/1148/1158#newcode136
Line 136: UpdatePacketReadTimes();
On 2009/04/29 21:57:40, wtc wrote:
> It's strange that you update packet read times *before*
> reading.  Why don't you do that after reading?

Excellent catch.  Thanks!

I moved the call to the point where the byte count was updated.

http://codereview.chromium.org/100004/diff/1148/1158#newcode599
Line 599: UMA_HISTOGRAM_CLIPPED_TIMES("Sdch.Network_Decode_1st_To_Last_a",
On 2009/04/29 21:42:40, Lincoln wrote:
> In url_request_http_job.cc, EnablePacketTimes() is only called if the client
is
> going to participate in the latency experiment.  Here, the histogram data is
> only sent if the client did *not* participate in the experiment.  Is it just
me,
> or is the logic backwards?

I don't think the logic is backwards, but I think I failed to enable
histogramming prior to the experiment (before we have done a round trip
success).  To fix this, I've added a call to enable timing whenever we advertise
a dicitonary (and flag this connection as one that expects an SDCH response).

The "passthrough" listed below is actually only histograms for a small error
case, where SDCH was expected, and never arrived.  When that happens, we get out
of the experiment in fact!

I've added a set of histograms for the inter-packet latency in the control
group, and I've changed some enum names to perhaps be more meaningful.

Thanks for bringing this (deficient) area to my attention!!!

http://codereview.chromium.org/100004/diff/1148/1158#newcode672
Line 672: DCHECK(false);
On 2009/04/29 21:57:40, wtc wrote:
> Nit: use NOTREACHED() instead.

Done.

http://codereview.chromium.org/100004/diff/1148/1157
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/100004/diff/1148/1157#newcode333
Line 333: // used for histograms etc., and is not required.  It is optinally
gather
On 2009/04/29 21:57:40, wtc wrote:
> Typo: optinally
> 
> Change "gather" to "gathered"

Done.

http://codereview.chromium.org/100004/diff/1148/1157#newcode334
Line 334: // basde on the settings of several control varibles.
On 2009/04/29 21:57:40, wtc wrote:
> Typo: varibles.

Done.

http://codereview.chromium.org/100004/diff/1148/1157#newcode337
Line 337: void UpdatePacketReadTimes();
On 2009/04/29 21:57:40, wtc wrote:
> The style guide requires us to list the methods together,
> so this UpdatePacketReadTimes method needs to be moved up.

Done.

http://codereview.chromium.org/100004/diff/1148/1157#newcode340
Line 340: // at a lower point in the network stack, assuring we have actual
packet
On 2009/04/29 21:57:40, wtc wrote:
> The issue is the HTTP cache and the SSL layers.

Done.

http://codereview.chromium.org/100004/diff/1148/1157#newcode357
Line 357: std::vector<base::Time> packet_times_;
On 2009/04/29 21:42:40, Lincoln wrote:
> Please add the #include corresponding to base::Time.

Done.

http://codereview.chromium.org/100004/diff/1148/1157#newcode368
Line 368: // We're ignoring overflow, as 1430 x 2^32 is a LOT of bytes.
On 2009/04/29 21:57:40, wtc wrote:
> Define 1430.
> 
> Nit: since you're using a signed int, it should be 2^31.

Done.

[Lincoln, 2009-04-30 15:47:06]

LGTM

http://codereview.chromium.org/100004/diff/1148/1156
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/100004/diff/1148/1156#newcode224
Line 224: return 0 != (request_info_.load_flags &
net::LOAD_SDCH_DICTIONARY_ADVERTISED);
On 2009/04/30 03:41:09, jar wrote:
> On 2009/04/29 21:42:40, Lincoln wrote:
> > Isn't the "0 !=" understood, as in the previous revision?
> 
> The result of the boolean operator produced a bool in the old code.  This new
> code produced an int... which caused a warning... which precluded a compile. 
So
> I added the compare.

Thanks for the explanation.  Sounds good.

http://codereview.chromium.org/100004/diff/1148/1156#newcode586
Line 586: request_info_.extra_headers += "Accept-Encoding:
gzip,deflate,bzip2\r\n";
On 2009/04/30 03:41:09, jar wrote:
> On 2009/04/29 21:42:40, Lincoln wrote:
> > You might wish to make "gzip,deflate,bzip2" a string constant to reduce
> > duplication.
> 
> Although I hate duplication... this seemed more readable, and avoided a
run-time
> cost of the string appends.
> 

Fair enough.  I'll note that a #define would save the cost of the string
appends, but the readability argument is a valid one.

http://codereview.chromium.org/100004/diff/1148/1158
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/100004/diff/1148/1158#newcode49
Line 49: filter_->finalize();
On 2009/04/30 03:41:09, jar wrote:
> On 2009/04/29 21:57:40, wtc wrote:
> > Why can't you just destroy the filter instead?
> >   filter_.reset();
> > 
> > Then we don't need to add the Finalize() method to Filter.
> 
> Done.

Nice change.

http://codereview.chromium.org/100004/diff/1148/1157
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/100004/diff/1148/1157#newcode340
Line 340: // at a lower point in the network stack, assuring we have actual
packet
On 2009/04/29 21:57:40, wtc wrote:
> The issue is the HTTP cache and the SSL layers.

SDCH is not supported in combination with HTTPS.  I presume you mean some other
type of SSL layer (secure VPN tunneling?)

http://codereview.chromium.org/100004/diff/152/1188
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/100004/diff/152/1188#newcode597
Line 597: EnablePacketCounting(kSdchPacketHistogramCount);
Why call this twice (on line 572 and here?)  In order to reach this line,
advertise_sdch must be true, meaning that we must have reached (if not entered)
the "if" statement on line 570.  The only case in which this is not redundant is
if the latency experiment is disabled.  Can you consolidate these into one call
so the reader doesn't have to untangle the logic?

http://codereview.chromium.org/100004/diff/152/1190
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/100004/diff/152/1190#newcode669
Line 669: // We already provided interpacket histograms above in the SDCH_DECODE
Oh, I see.  RecordPacketStats() can be called multiple times for the same
filter.  That wasn't clear to me before.  Thanks for adding the comment.

http://codereview.chromium.org/100004/diff/152/1189
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/100004/diff/152/1189#newcode284
Line 284: // Facilitate histograming by turning on packet counting.
Typo: histograming => histogramming (see line 347)

[jar (doing other things), 2009-04-30 16:52:49]

I responded to Lincoln's comments (see below), and also added an error code for
the situation when we *were* participating in an experiment, and then we were
forced to stop doing so (presumably because a problem with an SDCH transfer
surfaced).

http://codereview.chromium.org/100004/diff/152/1188
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/100004/diff/152/1188#newcode597
Line 597: EnablePacketCounting(kSdchPacketHistogramCount);
On 2009/04/30 15:47:06, Lincoln wrote:
> Why call this twice (on line 572 and here?)  In order to reach this line,
> advertise_sdch must be true, meaning that we must have reached (if not
entered)
> the "if" statement on line 570.  The only case in which this is not redundant
is
> if the latency experiment is disabled.  Can you consolidate these into one
call
> so the reader doesn't have to untangle the logic?

The two calls actually handle different cases, as in different calls to record
different selected statistics.  I'll clarify both cases with a comment at each
site.

http://codereview.chromium.org/100004/diff/152/1189
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/100004/diff/152/1189#newcode284
Line 284: // Facilitate histograming by turning on packet counting.
On 2009/04/30 15:47:06, Lincoln wrote:
> Typo: histograming => histogramming (see line 347)

Done.

[Lincoln, 2009-04-30 17:17:25]

LGTM

[wtc, 2009-04-30 18:19:25]

LGTM.  Just some nits.

http://codereview.chromium.org/100004/diff/1203/180
File net/base/sdch_manager.cc (right):

http://codereview.chromium.org/100004/diff/1203/180#newcode525
Line 525: if (AllowLatencyExperiment(url))
Nit: you can inline the find() call in AllowLatencyExperiment
here so that you can just pass the returned iterator to the
erase call below.

http://codereview.chromium.org/100004/diff/1203/181
File net/base/sdch_manager.h (right):

http://codereview.chromium.org/100004/diff/1203/181#newcode136
Line 136: // defunct = 93,           // OVER_10_MINUTES No timing stats
recorded.
Nit: I suggest that you add parentheses () around
"No timing stats recorded." so that this comment looks like
the "MISSING_TIME_STATS (Should never happen.)" comment
above.

http://codereview.chromium.org/100004/diff/1203/182
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/100004/diff/1203/182#newcode599
Line 599: request_info_.load_flags |= net::LOAD_SDCH_DICTIONARY_ADVERTISED;
I hope you will convert LOAD_SDCH_DICTIONARY_ADVERTISED to
a bool member of this class in a future CL.

http://codereview.chromium.org/100004/diff/1203/182#newcode601
Line 601: // definately employ an SDCH filter (or tentative sdch filter) when we
get
Typo: definately

http://codereview.chromium.org/100004/diff/1203/182#newcode602
Line 602: // get a response.  When done, we'll record histograms via SDCH_DECODE
or
Typo: delete the duplicate "get".

http://codereview.chromium.org/100004/diff/1203/184
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/100004/diff/1203/184#newcode533
Line 533: UpdatePacketReadTimes();  // Facilitate stats recording if it is
active.
Excellent!

http://codereview.chromium.org/100004/diff/1203/184#newcode605
Line 605: DCHECK(kSdchPacketHistogramCount > 4u);
Wow, is the 'u' suffix necessary to avoid a compiler warning?

http://codereview.chromium.org/100004/diff/1203/184#newcode606
Line 606: if (packet_times_.size() <= 4)
Would it be better to say
  if (packet_times_.size() < kSdchPacketHistogramCount)
?

If you make this change, please make the same change to
the tests in the cases below.

[jar (doing other things), 2009-04-30 19:26:50]

I made all suggestions proposed by Wan Teh.

Please diff with version 19 (where you made you comments) to see the relatively
minimal changes.  Note that I added a unit test for the slighltly more complex
setter you asked for, and also tossed in some lint fixes.

http://codereview.chromium.org/100004/diff/1203/180
File net/base/sdch_manager.cc (right):

http://codereview.chromium.org/100004/diff/1203/180#newcode525
Line 525: if (AllowLatencyExperiment(url))
On 2009/04/30 18:19:25, wtc wrote:
> Nit: you can inline the find() call in AllowLatencyExperiment
> here so that you can just pass the returned iterator to the
> erase call below.

Done.

http://codereview.chromium.org/100004/diff/1203/181
File net/base/sdch_manager.h (right):

http://codereview.chromium.org/100004/diff/1203/181#newcode136
Line 136: // defunct = 93,           // OVER_10_MINUTES No timing stats
recorded.
On 2009/04/30 18:19:25, wtc wrote:
> Nit: I suggest that you add parentheses () around
> "No timing stats recorded." so that this comment looks like
> the "MISSING_TIME_STATS (Should never happen.)" comment
> above.

Done.

http://codereview.chromium.org/100004/diff/1203/182
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/100004/diff/1203/182#newcode599
Line 599: request_info_.load_flags |= net::LOAD_SDCH_DICTIONARY_ADVERTISED;
On 2009/04/30 18:19:25, wtc wrote:
> I hope you will convert LOAD_SDCH_DICTIONARY_ADVERTISED to
> a bool member of this class in a future CL.

Done.

http://codereview.chromium.org/100004/diff/1203/182#newcode601
Line 601: // definately employ an SDCH filter (or tentative sdch filter) when we
get
On 2009/04/30 18:19:25, wtc wrote:
> Typo: definately

Done.

http://codereview.chromium.org/100004/diff/1203/182#newcode602
Line 602: // get a response.  When done, we'll record histograms via SDCH_DECODE
or
On 2009/04/30 18:19:25, wtc wrote:
> Typo: delete the duplicate "get".

Done.

http://codereview.chromium.org/100004/diff/1203/184
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/100004/diff/1203/184#newcode533
Line 533: UpdatePacketReadTimes();  // Facilitate stats recording if it is
active.
On 2009/04/30 18:19:25, wtc wrote:
> Excellent!

Done.

http://codereview.chromium.org/100004/diff/1203/184#newcode605
Line 605: DCHECK(kSdchPacketHistogramCount > 4u);
On 2009/04/30 18:19:25, wtc wrote:
> Wow, is the 'u' suffix necessary to avoid a compiler warning?

Nope.  I was gun shy... but didn't need it in this case, and pulled it out (in
all three places).

http://codereview.chromium.org/100004/diff/1203/184#newcode606
Line 606: if (packet_times_.size() <= 4)
On 2009/04/30 18:19:25, wtc wrote:
> Would it be better to say
>   if (packet_times_.size() < kSdchPacketHistogramCount)
> ?
> 
> If you make this change, please make the same change to
> the tests in the cases below.

I just wanted production code to be very defensive, since I hard code array
index 4 below.

[wtc, 2009-04-30 20:31:18]

LGTM.  Remember to add load_flags.h to this CL.

http://codereview.chromium.org/100004/diff/222/226
File net/base/sdch_filter.cc (right):

http://codereview.chromium.org/100004/diff/222/226#newcode151
Line 151: DCHECK_EQ(0u, dest_buffer_excess_index_);
In several DCHECKs in this file, you are using 0u,
which very few people use.  I would just use 0
unless 0u is necessary for the code to compile.

[Lincoln, 2009-04-30 20:42:38]

LGTM

http://codereview.chromium.org/100004/diff/222/226
File net/base/sdch_filter.cc (right):

http://codereview.chromium.org/100004/diff/222/226#newcode151
Line 151: DCHECK_EQ(0u, dest_buffer_excess_index_);
On 2009/04/30 20:31:18, wtc wrote:
> In several DCHECKs in this file, you are using 0u,
> which very few people use.  I would just use 0
> unless 0u is necessary for the code to compile.

I beg to differ.  Certain compilers will produce a warning about signed/unsigned
mismatch unless the U suffix is used.   For an example of code in which I have
had to take this approach in the past, please see:
http://open-vcdiff.googlecode.com/svn/trunk/src/rolling_hash_test.cc