Refactoring: Create per-connection packet writers in QuicDispatcher.

[This CL is abandoned.]

Refactoring: Create per-connection packet writers in QuicDispatcher.

To make porting the QUIC EndToEndTest to Chromium possible with fewer
Chromium-specific parts in shared code, I've made QuicDispatcher expose
and accept a QuicDispatcher::PacketWriterFactory which it uses to create
a new packet writer wrapper for every QuicConnection. I also changed
QuicConnection to accept a QuicConnection::PacketWriterFactory (a second
new type of factory) rather than the writer itself in its constructor,
since the per-connection packet writers need to be created with the
connection already existing.

Merge internal CL: 73064412

BUG=

[wtc, 2014-08-13 21:01:35]

Patch set 2 LGTM. Please make the suggested changes.

You can see some of the changes I suggested at
https://codereview.chromium.org/468373002/. Please ignore the changes to
quic_server.h and friends. I will take care of those.

Please also keep track of the changes that need to be ported back to the
internal tree.

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_connection...
File net/quic/quic_connection_test.cc (right):

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_connection...
net/quic/quic_connection_test.cc:429: writer()->set_is_server(is_server);

Nit: the change of |writer_| to |writer()| seems unnecessary -- we should be
able to just use |writer_|, right?

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_connection...
net/quic/quic_connection_test.cc:583: return
reinterpret_cast<TestPacketWriter*>(QuicConnection::writer());

IMPORTANT: please verify that it is fine to use reinterpret_cast here. The
original code from the internal CL uses down_cast.

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_dispatcher.h
File net/quic/quic_dispatcher.h (right):

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_dispatcher...
net/quic/quic_dispatcher.h:26: #define QUIC_BLOCKED_WRITER_INTERFACE_HASH

IMPORTANT: this change is not ideal. We should try to find a better solution.

After looking into this, I suggest we move this hash<> template specialization
into the net/quic/quic_blocked_writer_interface.h header. This seems to
recommended by the comment at the top of base/containers/hash_tables.h:

// NOTE: It is an explicit non-goal of this class to provide a generic hash
// function for pointers.  If you want to hash a pointers to a particular class,
// please define the template specialization elsewhere (for example, in its
// header file) and keep it specific to just pointers to that class.  This is
// because identity hashes are not desirable for all types that might show up
// in containers as pointers.

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_dispatcher...
net/quic/quic_dispatcher.h:31: return
hash<size_t>()(reinterpret_cast<size_t>(ptr));

Nit: we may want to switch to this implementation from the internal tree:

// Hash pointers as if they were int's, but bring more entropy to
// the lower bits.
template <>
struct hash<net::QuicBlockedWriterInterface*> {
  std::size_t operator()(const net::QuicBlockedWriterInterface* ptr) const {
    size_t k = reinterpret_cast<size_t>(ptr);
    return k + (k >> 6);
  }
};

The rationale is:

  ... pointers can have low entropy in the least significant bits, and
  those bits are important for efficiency in some uses ...

which seems reasonable to me.

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_dispatcher...
net/quic/quic_dispatcher.h:70: virtual QuicPacketWriter*
Create(QuicServerPacketWriter* writer,

IMPORTANT: please confirm that it is correct for these two Create() methods to
take a QuicServerPacketWriter* input and returns a QuicPacketWriter*. The
original code from the internal CL takes a QuicPacketWriter*.

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_http_strea...
File net/quic/quic_http_stream_test.cc (right):

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_http_strea...
net/quic/quic_http_stream_test.cc:218: TestPacketWriterFactory
writer_factory(socket);

Note my use of a TestPacketWriterFactory local variable. This implies
QuicConnection can only use the PacketWriterFactory during the constructor call.
This API contract of the QuicConnection constructor should be documented.

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_stream_fac...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_stream_fac...
net/quic/quic_stream_factory.cc:103: class ClientPacketWriterFactory : public
QuicConnection::PacketWriterFactory {

Note: I picked the "ClientPacketWriterFactory" name temporarily. Can you suggest
a better name? Perhaps "ChromiumPacketWriterFactory"? Or do you think
"ClientPacketWriterFactory" is a fine name?

https://codereview.chromium.org/467963002/diff/20001/net/quic/test_tools/quic...
File net/quic/test_tools/quic_test_utils.cc (right):

https://codereview.chromium.org/467963002/diff/20001/net/quic/test_tools/quic...
net/quic/test_tools/quic_test_utils.cc:251:
*NiceMockPacketWriterFactory::GetInstance(),

IMPORTANT: if we can rely on the API contract that QuicConnection will only use
the PacketWriterFactory during the constructor call, we can even pass a
temporary NiceMockPacketWriterFactory variable here. NiceMockPacketWriterFactory
doesn't have any data members, so it doesn't seem necessary to use it as a
singleton.

https://codereview.chromium.org/467963002/diff/20001/net/tools/quic/quic_clie...
File net/tools/quic/quic_client.h (right):

https://codereview.chromium.org/467963002/diff/20001/net/tools/quic/quic_clie...
net/tools/quic/quic_client.h:197: virtual QuicPacketWriter*
Create(QuicConnection* connection) const override;

Change "override" to "OVERRIDE".

https://codereview.chromium.org/467963002/diff/20001/net/tools/quic/quic_disp...
File net/tools/quic/quic_dispatcher_test.cc (right):

https://codereview.chromium.org/467963002/diff/20001/net/tools/quic/quic_disp...
net/tools/quic/quic_dispatcher_test.cc:16: #include
"net/quic/test_tools/quic_test_utils.h"

Aha!

Note that in a few files I removed the "net/quic/test_tools/quic_test_utils.h"
inclusion if the files also include
"net/tools/quic/test_tools/quic_test_utils.h". This is an attempt to fix the
problem of including both copies of quic_dispatcher.h. I suspect this also
introduced other problems, but you may have fix those other problems. (I no
longer remember what the problems were.)

https://codereview.chromium.org/467963002/diff/20001/net/tools/quic/quic_per_...
File net/tools/quic/quic_per_connection_packet_writer.cc (right):

https://codereview.chromium.org/467963002/diff/20001/net/tools/quic/quic_per_...
net/tools/quic/quic_per_connection_packet_writer.cc:1: #include
"net/tools/quic/quic_per_connection_packet_writer.h"

Please write the copyright and license header.

https://codereview.chromium.org/467963002/diff/20001/net/tools/quic/quic_serv...
File net/tools/quic/quic_server.h (right):

https://codereview.chromium.org/467963002/diff/20001/net/tools/quic/quic_serv...
net/tools/quic/quic_server.h:18: #include "net/tools/quic/quic_dispatcher.h"

Why is the forward declaration of QuicDispatcher not enough?

UPDATE: I tried this and found that we also need to
1) forward-declare class ProcessPacketInterface, and
2) include net/tools/quic/quic_dispatcher.h in net/tools/quic/quic_server.cc and
net/tools/quic/test_tools/server_thread.cc.

I can give this a try in the internal source tree, so you don't need to do this.

[dmz, 2014-08-14 17:22:42]

PTAL

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_connection...
File net/quic/quic_connection_test.cc (right):

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_connection...
net/quic/quic_connection_test.cc:429: writer()->set_is_server(is_server);
On 2014/08/13 21:01:33, wtc wrote:
> 
> Nit: the change of |writer_| to |writer()| seems unnecessary -- we should be
> able to just use |writer_|, right?

I don't think |writer_| exists anymore, right?

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_connection...
net/quic/quic_connection_test.cc:583: return
reinterpret_cast<TestPacketWriter*>(QuicConnection::writer());
On 2014/08/13 21:01:34, wtc wrote:
> 
> IMPORTANT: please verify that it is fine to use reinterpret_cast here. The
> original code from the internal CL uses down_cast.

I believe static_cast would be better style, but it all does the same thing.
down_cast just crashes earlier if it's the wrong type :P.

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_dispatcher.h
File net/quic/quic_dispatcher.h (right):

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_dispatcher...
net/quic/quic_dispatcher.h:26: #define QUIC_BLOCKED_WRITER_INTERFACE_HASH
On 2014/08/13 21:01:34, wtc wrote:
> 
> IMPORTANT: this change is not ideal. We should try to find a better solution.
> 
> After looking into this, I suggest we move this hash<> template specialization
> into the net/quic/quic_blocked_writer_interface.h header. This seems to
> recommended by the comment at the top of base/containers/hash_tables.h:
> 
> // NOTE: It is an explicit non-goal of this class to provide a generic hash
> // function for pointers.  If you want to hash a pointers to a particular
class,
> // please define the template specialization elsewhere (for example, in its
> // header file) and keep it specific to just pointers to that class.  This is
> // because identity hashes are not desirable for all types that might show up
> // in containers as pointers.

Done.

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_dispatcher...
net/quic/quic_dispatcher.h:31: return
hash<size_t>()(reinterpret_cast<size_t>(ptr));
On 2014/08/13 21:01:34, wtc wrote:
> 
> Nit: we may want to switch to this implementation from the internal tree:
> 
> // Hash pointers as if they were int's, but bring more entropy to
> // the lower bits.
> template <>
> struct hash<net::QuicBlockedWriterInterface*> {
>   std::size_t operator()(const net::QuicBlockedWriterInterface* ptr) const {
>     size_t k = reinterpret_cast<size_t>(ptr);
>     return k + (k >> 6);
>   }
> };
> 
> The rationale is:
> 
>   ... pointers can have low entropy in the least significant bits, and
>   those bits are important for efficiency in some uses ...
> 
> which seems reasonable to me.

Done.

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_dispatcher...
net/quic/quic_dispatcher.h:70: virtual QuicPacketWriter*
Create(QuicServerPacketWriter* writer,
On 2014/08/13 21:01:34, wtc wrote:
> 
> IMPORTANT: please confirm that it is correct for these two Create() methods to
> take a QuicServerPacketWriter* input and returns a QuicPacketWriter*. The
> original code from the internal CL takes a QuicPacketWriter*.

Yes, that's right.

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_http_strea...
File net/quic/quic_http_stream_test.cc (right):

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_http_strea...
net/quic/quic_http_stream_test.cc:218: TestPacketWriterFactory
writer_factory(socket);
On 2014/08/13 21:01:34, wtc wrote:
> 
> Note my use of a TestPacketWriterFactory local variable. This implies
> QuicConnection can only use the PacketWriterFactory during the constructor
call.
> This API contract of the QuicConnection constructor should be documented.

The PacketWriterFactory is passed in as const& -- isn't that documentation
enough? It's theoretically possible to store a const& member or something
similar, but it's certainly against style. I think const& implicitly implies
that it should not be stored.

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_stream_fac...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_stream_fac...
net/quic/quic_stream_factory.cc:103: class ClientPacketWriterFactory : public
QuicConnection::PacketWriterFactory {
On 2014/08/13 21:01:34, wtc wrote:
> 
> Note: I picked the "ClientPacketWriterFactory" name temporarily. Can you
suggest
> a better name? Perhaps "ChromiumPacketWriterFactory"? Or do you think
> "ClientPacketWriterFactory" is a fine name?

I probably would have gone with DefaultPacketWriterFactory, but I think
ClientPacketWriterFactory is perfectly good.

https://codereview.chromium.org/467963002/diff/20001/net/quic/test_tools/quic...
File net/quic/test_tools/quic_test_utils.cc (right):

https://codereview.chromium.org/467963002/diff/20001/net/quic/test_tools/quic...
net/quic/test_tools/quic_test_utils.cc:251:
*NiceMockPacketWriterFactory::GetInstance(),
On 2014/08/13 21:01:34, wtc wrote:
> 
> IMPORTANT: if we can rely on the API contract that QuicConnection will only
use
> the PacketWriterFactory during the constructor call, we can even pass a
> temporary NiceMockPacketWriterFactory variable here.
NiceMockPacketWriterFactory
> doesn't have any data members, so it doesn't seem necessary to use it as a
> singleton.

Good point, that would be simpler. I'll put this in the internal CL as well.

https://codereview.chromium.org/467963002/diff/20001/net/tools/quic/quic_clie...
File net/tools/quic/quic_client.h (right):

https://codereview.chromium.org/467963002/diff/20001/net/tools/quic/quic_clie...
net/tools/quic/quic_client.h:197: virtual QuicPacketWriter*
Create(QuicConnection* connection) const override;
On 2014/08/13 21:01:34, wtc wrote:
> 
> Change "override" to "OVERRIDE".

Done.

https://codereview.chromium.org/467963002/diff/20001/net/tools/quic/quic_disp...
File net/tools/quic/quic_dispatcher_test.cc (right):

https://codereview.chromium.org/467963002/diff/20001/net/tools/quic/quic_disp...
net/tools/quic/quic_dispatcher_test.cc:16: #include
"net/quic/test_tools/quic_test_utils.h"
On 2014/08/13 21:01:34, wtc wrote:
> 
> Aha!
> 
> Note that in a few files I removed the "net/quic/test_tools/quic_test_utils.h"
> inclusion if the files also include
> "net/tools/quic/test_tools/quic_test_utils.h". This is an attempt to fix the
> problem of including both copies of quic_dispatcher.h. I suspect this also
> introduced other problems, but you may have fix those other problems. (I no
> longer remember what the problems were.)

Yeah, I noticed that and undid it because of the problems it caused. Are there
any #includes that I missed?

https://codereview.chromium.org/467963002/diff/20001/net/tools/quic/quic_per_...
File net/tools/quic/quic_per_connection_packet_writer.cc (right):

https://codereview.chromium.org/467963002/diff/20001/net/tools/quic/quic_per_...
net/tools/quic/quic_per_connection_packet_writer.cc:1: #include
"net/tools/quic/quic_per_connection_packet_writer.h"
On 2014/08/13 21:01:34, wtc wrote:
> 
> Please write the copyright and license header.

Done.

https://codereview.chromium.org/467963002/diff/20001/net/tools/quic/quic_serv...
File net/tools/quic/quic_server.h (right):

https://codereview.chromium.org/467963002/diff/20001/net/tools/quic/quic_serv...
net/tools/quic/quic_server.h:18: #include "net/tools/quic/quic_dispatcher.h"
On 2014/08/13 21:01:34, wtc wrote:
> 
> Why is the forward declaration of QuicDispatcher not enough?
> 
> UPDATE: I tried this and found that we also need to
> 1) forward-declare class ProcessPacketInterface, and
> 2) include net/tools/quic/quic_dispatcher.h in net/tools/quic/quic_server.cc
and
> net/tools/quic/test_tools/server_thread.cc.
> 
> I can give this a try in the internal source tree, so you don't need to do
this.

Acknowledged.

[wtc, 2014-08-14 20:05:01]

Patch set 3 LGTM.

1. Please make the two suggested changes to net/quic/quic_stream_factory.cc and
quic_blocked_writer_interface.h.

2. Before you commit, please go over the CL's description and make sure it
reflects the final code.

Thanks!

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_connection...
File net/quic/quic_connection_test.cc (right):

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_connection...
net/quic/quic_connection_test.cc:429: writer()->set_is_server(is_server);

On 2014/08/14 17:22:41, dmz wrote:
>
> I don't think |writer_| exists anymore, right?

You are right. You deleted the |writer_| member from this class. The |writer_|
member of the base class QuicConnection is private, so it's not accessible.

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_http_strea...
File net/quic/quic_http_stream_test.cc (right):

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_http_strea...
net/quic/quic_http_stream_test.cc:218: TestPacketWriterFactory
writer_factory(socket);

On 2014/08/14 17:22:42, dmz wrote:
>
> The PacketWriterFactory is passed in as const& -- isn't that documentation
> enough?

Hmm... you're right that is a very strong hint. I am worried that the
QuicConnection constructor may store that (const) reference in a data member and
use it later.

> I think const& implicitly implies that it should not be stored.

I searched for "reference" in the Style Guide. I found what supports your
statement:

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

Under "Decision", it says:

 However, there are some instances where using const T* is preferable to const
T& for input parameters. For example:

    * You want to pass in a null pointer.
    * The function saves a pointer or reference to the input.

I think if the constructor may save a pointer or reference to the input
parameter, then we should signal that by using const T*.

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_stream_fac...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_stream_fac...
net/quic/quic_stream_factory.cc:103: class ClientPacketWriterFactory : public
QuicConnection::PacketWriterFactory {

On 2014/08/14 17:22:42, dmz wrote:
>
> I probably would have gone with DefaultPacketWriterFactory, ...

Ah, I now also prefer "Default" because this factory creates
QuicDefaultPacketWriter. Can you make this change?

https://codereview.chromium.org/467963002/diff/20001/net/tools/quic/quic_disp...
File net/tools/quic/quic_dispatcher_test.cc (right):

https://codereview.chromium.org/467963002/diff/20001/net/tools/quic/quic_disp...
net/tools/quic/quic_dispatcher_test.cc:16: #include
"net/quic/test_tools/quic_test_utils.h"

On 2014/08/14 17:22:42, dmz wrote:
>
> Yeah, I noticed that and undid it because of the problems it caused. Are there
> any #includes that I missed?

Actually I don't know :-) But it's not worth getting to the bottom of this
because the net/tools/quic will be gone soon.

https://codereview.chromium.org/467963002/diff/40001/net/quic/quic_blocked_wr...
File net/quic/quic_blocked_writer_interface.h (right):

https://codereview.chromium.org/467963002/diff/40001/net/quic/quic_blocked_wr...
net/quic/quic_blocked_writer_interface.h:29: template <>

Please add the following comment before this line:

// Hash pointers as if they were int's, but bring more entropy to
// the lower bits.

[dmz, 2014-08-14 21:24:52]

Okay, Ryan, could you have a look? I imagine you'll just want to look at the
changes relative to the internal code (which is patch set 1).

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_stream_fac...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/467963002/diff/20001/net/quic/quic_stream_fac...
net/quic/quic_stream_factory.cc:103: class ClientPacketWriterFactory : public
QuicConnection::PacketWriterFactory {
On 2014/08/14 20:05:00, wtc wrote:
> 
> On 2014/08/14 17:22:42, dmz wrote:
> >
> > I probably would have gone with DefaultPacketWriterFactory, ...
> 
> Ah, I now also prefer "Default" because this factory creates
> QuicDefaultPacketWriter. Can you make this change?

Done.

https://codereview.chromium.org/467963002/diff/40001/net/quic/quic_blocked_wr...
File net/quic/quic_blocked_writer_interface.h (right):

https://codereview.chromium.org/467963002/diff/40001/net/quic/quic_blocked_wr...
net/quic/quic_blocked_writer_interface.h:29: template <>
On 2014/08/14 20:05:00, wtc wrote:
> 
> Please add the following comment before this line:
> 
> // Hash pointers as if they were int's, but bring more entropy to
> // the lower bits.

Done.

[wtc, 2014-08-14 22:23:38]

Patch set 4 LGTM.

On 2014/08/14 21:24:52, dmz wrote:
> Okay, Ryan, could you have a look? I imagine you'll just want to look at the
> changes relative to the internal code (which is patch set 1).

Note: Patch set 1 already contains my attempted changes to port the internal
code to Chromium. Some of those changes are incomplete or wrong.

[wtc, 2014-08-15 22:22:03]

Daniel: I have a comment for you below.

Ryan: I think it'd be good if you could check the new changes Daniel made on top
of the internal CL that you reviewed.

To help you do that, I created a companion CL at
https://codereview.chromium.org/475113005/. Patch set 1 of that CL was a
straightforward application of the internal patch to the Chromium tree. So you
can just view the diffs against patch set 1 in that CL. Thanks a lot.

https://codereview.chromium.org/467963002/diff/60001/net/quic/quic_connection...
File net/quic/quic_connection_test.cc (right):

https://codereview.chromium.org/467963002/diff/60001/net/quic/quic_connection...
net/quic/quic_connection_test.cc:3979:
EXPECT_TRUE(client.sent_packet_manager().using_pacing());

This line has changed to EXPECT_FALSE on the tip of the trunk. So this CL needs
to be rebased before it is committed.

[wtc, 2014-08-21 01:05:02]

Daniel,

Will you be able to rebase and commit this CL soon? If not, I can commit my
clone of this CL (https://codereview.chromium.org/475113005/ ).

[dmz, 2014-08-29 19:03:06]

On 2014/08/21 01:05:02, wtc wrote:
> Daniel,
> 
> Will you be able to rebase and commit this CL soon? If not, I can commit my
> clone of this CL (https://codereview.chromium.org/475113005/ ).

Hi Wan-Teh,

Sorry for the late response. I believe all my dmziegler@chromium.org email is
still being redirected to dmziegler@google.com, so I'm not getting any
notifications -- do you know how that can be fixed? Anyway, thanks for
committing it for me.

Daniel

[Ryan Hamilton, 2015-04-01 03:05:52]

rch@chromium.org changed reviewers:
- wtc@chromium.org

[Ryan Hamilton, 2015-04-01 03:05:53]

Removing myself as a reviewer because I believe this functionality has been
added in tip-of-tree.

[Ryan Hamilton, 2015-04-01 03:06:27]

rch@chromium.org changed reviewers:
+ wtc@chromium.org

[Ryan Hamilton, 2015-04-01 03:06:54]

rch@chromium.org changed reviewers:
- rch@chromium.org