Prioritize which HTTP requests get a socket first by adding a priority level ...

Prioritize which HTTP requests get a socket first by adding a priority level to various methods and classes.  Fix lint errors along the way.

R=darin,wtc
BUG=8993
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=12470

[willchan no longer on Chromium, 2009-03-23 23:50:40]

I wasn't sure exactly where I should be adding the priority member variable, so
I figured I'd just add it in some places and ask for early comments.  I'm
probably doing something silly, so feel free to say so :)

The test also isn't exactly how I want it.  It's a bit overly complicated.  Once
the code is in the shape we want, I'll probably refactor the test to use a
different ClientSocketPool with a smaller max sockets per group and fewer
requests.

[darin (slow to review), 2009-03-24 16:35:16]

LGTM.  Thanks for the cleaning up the neighboring code!  I only have some nit
picks:

http://codereview.chromium.org/42541/diff/1014/1024
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/42541/diff/1014/1024#newcode330
Line 330: if (request_data.resource_type == ResourceType::MAIN_FRAME ||
nit: it'd be good to document the strategy being used here.

http://codereview.chromium.org/42541/diff/1014/1021
File net/base/client_socket_pool.cc (right):

http://codereview.chromium.org/42541/diff/1014/1021#newcode52
Line 52: while (it != pending_requests->end() && r.priority <= it->priority)
++it;
style-nit: please put the "++it" on a new line.  doing so may not be required by
the google style guide, but it is a chromium coding convention.

http://codereview.chromium.org/42541/diff/1014/1022
File net/base/client_socket_pool.h (right):

http://codereview.chromium.org/42541/diff/1014/1022#newcode116
Line 116: static void InsertRequestIntoWaitlist(const Request& r,
nit: "WaitList" since it is two words.. or maybe just call this function
InsertRequestIntoQueue since it takes a RequestQueue param?

http://codereview.chromium.org/42541/diff/1014/1026
File net/base/client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/42541/diff/1014/1026#newcode180
Line 180: << "Request " << i << " was not in order.";
style-nit: it is a chrome coding convention to put the operator on the preceding
line when wrapping an expression.  there's a few more of these in this file.

http://codereview.chromium.org/42541/diff/1014/1016
File net/http/http_request_info.h (right):

http://codereview.chromium.org/42541/diff/1014/1016#newcode8
Line 8: #include <string>
style-nit: we usually just include the minimal set of headers required (the idea
being that it helps compile time)

http://codereview.chromium.org/42541/diff/1014/1017
File net/url_request/url_request.h (right):

http://codereview.chromium.org/42541/diff/1014/1017#newcode432
Line 432: // Returns the priority level for this request.
nit: it'd be good to document how the priority value is interpreted.  are
negative values allowed?  does a larger value mean higher priority?

[wtc, 2009-03-24 17:35:29]

LGTM.  Just some nits below.

You added a 'priority' parameter to some methods that
have a 'callback' parameter.  It'd look nicer for the
'callback' parameter to remain the last parameter.

http://codereview.chromium.org/42541/diff/1014/1024
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/42541/diff/1014/1024#newcode332
Line 332: request->set_priority(1);
This means the default priority of 0 is significant, because
you are using a priority of 1 here to give main frames and
sub-frames a priority boost.  Would be nice to document
this.

http://codereview.chromium.org/42541/diff/1014/1025
File chrome/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/42541/diff/1014/1025#newcode181
Line 181: virtual ~Observer() { }
Nice!

http://codereview.chromium.org/42541/diff/1014/1025#newcode315
Line 315: ExtraRequestInfo* r =
down_cast<ExtraRequestInfo*>(request->user_data());
down_cast is probably a GCC extension or a very new C++
feature.  Let's not use it yet.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Casting

http://codereview.chromium.org/42541/diff/1014/1020
File net/base/client_socket_handle.h (right):

http://codereview.chromium.org/42541/diff/1014/1020#newcode47
Line 47: int Init(const std::string& group_name, CompletionCallback* callback,
Note: the style guide requires us to list only one parameter
a line if the parameters can't all fit on one line:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

But this guideline is not always followed in the net module.

http://codereview.chromium.org/42541/diff/1014/1021
File net/base/client_socket_pool.cc (right):

http://codereview.chromium.org/42541/diff/1014/1021#newcode45
Line 45: // priority.  Highest priorities are closest to the front.  Older
requests are
Nice!

http://codereview.chromium.org/42541/diff/1014/1021#newcode50
Line 50: const Request& r, RequestQueue* pending_requests) {
Formatting nit: see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

http://codereview.chromium.org/42541/diff/1014/1022
File net/base/client_socket_pool.h (right):

http://codereview.chromium.org/42541/diff/1014/1022#newcode119
Line 119: ~ClientSocketPool();
Nit: since you're reordering the code to follow the style
guide, the style guide recomends putting the destructor
before methods, including static methods:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

http://codereview.chromium.org/42541/diff/1014/1026
File net/base/client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/42541/diff/1014/1026#newcode18
Line 18: const int kPriorities[10] = { 1, 7, 9, 5, 6, 2, 8, 3, 4, 1 };
Perhaps the hardcoded constants 10 and 9 in the unit tests
below can be replaced by arraysize(kPriorities) and
arraysize(kPriorities) - 1.

http://codereview.chromium.org/42541/diff/1014/1016
File net/http/http_request_info.h (right):

http://codereview.chromium.org/42541/diff/1014/1016#newcode8
Line 8: #include <string>
On 2009/03/24 16:35:17, darin wrote:
> style-nit: we usually just include the minimal set of headers required (the
idea
> being that it helps compile time)

I think this is a change suggested by cpplint.py (include
what you use).

[willchan no longer on Chromium, 2009-03-24 18:14:14]

I think I've addressed all the comments.  I wish Rietveld had a UI for making
sure I did so.  Maybe I'll add that.

Thanks for the nitpicks on the chromium specific coding conventions, please
continue to point these out in future code reviews, since I'm sure I'll continue
to make these mistakes for at least awhile.

And thanks to wtc for the pointer on the function decls/defs.  I can't believe I
never noticed that the rule was different from function calls.

As an aside, can one of you explain to a newbie like me how I tag this
changelist as being relevant to the bug opened for this feature?  Thanks!

http://codereview.chromium.org/42541/diff/1014/1024
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/42541/diff/1014/1024#newcode330
Line 330: if (request_data.resource_type == ResourceType::MAIN_FRAME ||
On 2009/03/24 16:35:17, darin wrote:
> nit: it'd be good to document the strategy being used here.

Done.

http://codereview.chromium.org/42541/diff/1014/1024#newcode332
Line 332: request->set_priority(1);
On 2009/03/24 17:35:30, wtc wrote:
> This means the default priority of 0 is significant, because
> you are using a priority of 1 here to give main frames and
> sub-frames a priority boost.  Would be nice to document
> this.

Done.

http://codereview.chromium.org/42541/diff/1014/1025
File chrome/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/42541/diff/1014/1025#newcode315
Line 315: ExtraRequestInfo* r =
down_cast<ExtraRequestInfo*>(request->user_data());
On 2009/03/24 17:35:30, wtc wrote:
> down_cast is probably a GCC extension or a very new C++
> feature.  Let's not use it yet.
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Casting

Actually, it is defined in our own base/basictypes.h header.

http://codereview.chromium.org/42541/diff/1014/1021
File net/base/client_socket_pool.cc (right):

http://codereview.chromium.org/42541/diff/1014/1021#newcode50
Line 50: const Request& r, RequestQueue* pending_requests) {
On 2009/03/24 17:35:30, wtc wrote:
> Formatting nit: see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

Done.

http://codereview.chromium.org/42541/diff/1014/1021#newcode52
Line 52: while (it != pending_requests->end() && r.priority <= it->priority)
++it;
On 2009/03/24 16:35:17, darin wrote:
> style-nit: please put the "++it" on a new line.  doing so may not be required
by
> the google style guide, but it is a chromium coding convention.

Thanks for the notice!  I'll try to remember it, although it might take me a few
more mistakes before I internalize this rule in my head.

http://codereview.chromium.org/42541/diff/1014/1022
File net/base/client_socket_pool.h (right):

http://codereview.chromium.org/42541/diff/1014/1022#newcode116
Line 116: static void InsertRequestIntoWaitlist(const Request& r,
On 2009/03/24 16:35:17, darin wrote:
> nit: "WaitList" since it is two words.. or maybe just call this function
> InsertRequestIntoQueue since it takes a RequestQueue param?

Done.

http://codereview.chromium.org/42541/diff/1014/1022#newcode119
Line 119: ~ClientSocketPool();
On 2009/03/24 17:35:30, wtc wrote:
> Nit: since you're reordering the code to follow the style
> guide, the style guide recomends putting the destructor
> before methods, including static methods:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

Done.

http://codereview.chromium.org/42541/diff/1014/1026
File net/base/client_socket_pool_unittest.cc (right):

http://codereview.chromium.org/42541/diff/1014/1026#newcode18
Line 18: const int kPriorities[10] = { 1, 7, 9, 5, 6, 2, 8, 3, 4, 1 };
On 2009/03/24 17:35:30, wtc wrote:
> Perhaps the hardcoded constants 10 and 9 in the unit tests
> below can be replaced by arraysize(kPriorities) and
> arraysize(kPriorities) - 1.

I started doing that and then thought that arraysize(kPriorities) is similarly
unintuitive to readers.  So I added another constant kNumPendingRequests which
is equal to arraysize(kPriorities) and used that through the code.

http://codereview.chromium.org/42541/diff/1014/1026#newcode180
Line 180: << "Request " << i << " was not in order.";
On 2009/03/24 16:35:17, darin wrote:
> style-nit: it is a chrome coding convention to put the operator on the
preceding
> line when wrapping an expression.  there's a few more of these in this file.

Fixed, thanks for letting me know about the convention.  I wouldn't have known
otherwise.  One unfortunate aspect of this convention I noticed is that my
editor (VIM) with the default google vimrc settings does not indent the
following line correct in this case.  I can probably look into tweaking my vimrc
to accommodate the convention.

http://codereview.chromium.org/42541/diff/1014/1016
File net/http/http_request_info.h (right):

http://codereview.chromium.org/42541/diff/1014/1016#newcode8
Line 8: #include <string>
On 2009/03/24 17:35:30, wtc wrote:
> On 2009/03/24 16:35:17, darin wrote:
> > style-nit: we usually just include the minimal set of headers required (the
> idea
> > being that it helps compile time)
> 
> I think this is a change suggested by cpplint.py (include
> what you use).
> 
> 

Correct, I fixed this due to the lint warning.  In the main google code base, I
definitely agree with the lint rule, because if you don't include what you use,
and instead depend on some other header file that you include to transitively
pull in the dependency, then if that header file is refactored and drops its
#include, then this could introduce a compile error in the consumers.  This puts
an undue burden on library developers to fix all the consumers' code prior to
removing a #include in the library header file.

In our case, you can imagine a similar situation if we are transitively pulling
in certain system headers via webkit header files.  A webkit merge that included
a change that dropped the #include could result in a build breakage in chromium
code.  This is a much smaller problem than it is for main google repository
since the code base is so much smaller (relatively speaking).

That said, if chromium-dev understands these concerns and decides the build
advantage is worth it, I'm open to that.  I'm new here and don't want to be too
religious about my previous coding conventions.  It's annoying to have to ignore
lint errors though.

http://codereview.chromium.org/42541/diff/1014/1017
File net/url_request/url_request.h (right):

http://codereview.chromium.org/42541/diff/1014/1017#newcode432
Line 432: // Returns the priority level for this request.
On 2009/03/24 16:35:17, darin wrote:
> nit: it'd be good to document how the priority value is interpreted.  are
> negative values allowed?  does a larger value mean higher priority?

Done.

[darin (slow to review), 2009-03-24 18:29:39]

http://codereview.chromium.org/42541/diff/1014/1016
File net/http/http_request_info.h (right):

http://codereview.chromium.org/42541/diff/1014/1016#newcode8
Line 8: #include <string>
those are good points, and since cpplint complained, we should probably go with
the change that makes cpplint not complain.  that is easier for developers to
follow :)

[wtc, 2009-03-24 20:54:30]

LGTM.

To link this CL to the bug, click "Edit Issue" in Rietveld
and add the following to the bottom of the CL's Description:

R=darin,wtc
BUG=8993

http://codereview.chromium.org/42541/diff/1014/1025
File chrome/browser/renderer_host/resource_dispatcher_host.h (right):

http://codereview.chromium.org/42541/diff/1014/1025#newcode315
Line 315: ExtraRequestInfo* r =
down_cast<ExtraRequestInfo*>(request->user_data());
On 2009/03/24 18:14:14, willchan wrote:
>
> Actually, [down_cast] is defined in our own base/basictypes.h header.

You're right!  Chromium is not using down_cast, which is
why I didn't know it existed.

http://codereview.chromium.org/42541/diff/56/4025
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/42541/diff/56/4025#newcode335
Line 335: // boosting priorities for requests for the frontground tab.
Nit: foreground tab.

http://codereview.chromium.org/42541/diff/56/4021
File net/base/client_socket_handle.h (right):

http://codereview.chromium.org/42541/diff/56/4021#newcode47
Line 47: int Init(const std::string& group_name, CompletionCallback* callback,
Nit: 'callback' should remain the last parameter to be
consistent with all other functions that have a 'callback'
parameter.

http://codereview.chromium.org/42541/diff/56/4023
File net/base/client_socket_pool.h (right):

http://codereview.chromium.org/42541/diff/56/4023#newcode51
Line 51: int RequestSocket(ClientSocketHandle* handle, CompletionCallback*
callback,
Nit: 'callback' should remain the last parameter to be
consistent with other methods that have a 'callback'
parameter.

[wtc, 2009-03-24 21:07:51]

http://codereview.chromium.org/42541/diff/56/4022
File net/base/client_socket_pool.cc (right):

http://codereview.chromium.org/42541/diff/56/4022#newcode54
Line 54: pending_requests->insert(it, r);
One more thing: we may want to use a different STL container
for RequestQueue now that we replaced the push_back method
by the insert method.  You don't need to change this now.

[willchan no longer on Chromium, 2009-03-24 21:41:12]

Thanks for the explanation on the R= and BUG= stuff.  It seems like another
thing that Rietveld/gcl could do.  And sorry about missing your callback
parameter ordering nit earlier, that was silly of me.

http://codereview.chromium.org/42541/diff/56/4025
File chrome/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/42541/diff/56/4025#newcode335
Line 335: // boosting priorities for requests for the frontground tab.
On 2009/03/24 20:54:30, wtc wrote:
> Nit: foreground tab.

Woops, sorry for that brain fart.  Done.

http://codereview.chromium.org/42541/diff/56/4021
File net/base/client_socket_handle.h (right):

http://codereview.chromium.org/42541/diff/56/4021#newcode47
Line 47: int Init(const std::string& group_name, CompletionCallback* callback,
On 2009/03/24 20:54:30, wtc wrote:
> Nit: 'callback' should remain the last parameter to be
> consistent with all other functions that have a 'callback'
> parameter.

Doh, knew I missed a comment.  Fixed all these occurrences (please double check
for me, thanks!).

http://codereview.chromium.org/42541/diff/56/4022
File net/base/client_socket_pool.cc (right):

http://codereview.chromium.org/42541/diff/56/4022#newcode54
Line 54: pending_requests->insert(it, r);
On 2009/03/24 21:07:51, wtc wrote:
> One more thing: we may want to use a different STL container
> for RequestQueue now that we replaced the push_back method
> by the insert method.  You don't need to change this now.

Yep, that's part of the reason I typedef'd RequestQueue, since I wasn't sure if
we'd switch the actual STL container.  Do you have any thoughts on what's
better?  I was thinking of using the priority_queue<>, but there's no way to
CancelRequest() given the priority_queue<> interface.

http://codereview.chromium.org/42541/diff/56/4023
File net/base/client_socket_pool.h (right):

http://codereview.chromium.org/42541/diff/56/4023#newcode51
Line 51: int RequestSocket(ClientSocketHandle* handle, CompletionCallback*
callback,
On 2009/03/24 20:54:30, wtc wrote:
> Nit: 'callback' should remain the last parameter to be
> consistent with other methods that have a 'callback'
> parameter.

Done.

[wtc, 2009-03-24 21:59:45]

LGTM.

I'm not sure which STL container is right.  It seems that
with the current code, std::list would be the simplest
container for the job.  If the number of priorities is
known, we can also use that many std::deque's (one for each
prioriti level).