Make swapping the incoming queue with the work queue constant time....

Make swapping the incoming queue with the work queue constant time.

This is an alternative to the fix proposed here:
http://codereview.chromium.org/172101

Credit goes to Dean McNamee for discovering this performance issue!

R=deanm
BUG=20204
TEST=none


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

[eric.rannaud, 2009-09-02 18:37:46]

There is either a typo or some missing code in the Swap() method.

http://codereview.chromium.org/190006/diff/1/2
File base/message_loop.h (right):

http://codereview.chromium.org/190006/diff/1/2#newcode287
Line 287: c.swap(queue->c);
What is c?

[darin (slow to review), 2009-09-02 20:00:41]

c is a protected member of std::queue.  it reveals the underlying container
class, which is deque by default.  deque has a constant time swap method, which
i am using here.

[Dean McNamee, 2009-09-02 20:01:19]

LGTM.  I don't know if you should call it Swap or swap(), whatever you
think is fine.

(Also, I realized that the reason std::queue doesn't supply a swap()
because it takes the container type as a template parameter.  While
deque supports swap, you could use other sequence containers that
don't).

On 2009/09/02 18:37:46, eric.rannaud wrote:
> There is either a typo or some missing code in the Swap() method.
> 
> http://codereview.chromium.org/190006/diff/1/2
> File base/message_loop.h (right):
> 
> http://codereview.chromium.org/190006/diff/1/2#newcode287
> Line 287: c.swap(queue->c);
> What is c?

[darin (slow to review), 2009-09-02 20:04:18]

I went with Swap to conform to google style.  The name of the class similarly
conforms to google style.

I think you are probably correct about why swap is not implemented by
std::queue.  Given that deque is the default container type, I think we'll
always be OK with this code.

[Dean McNamee, 2009-09-02 20:06:18]

LGTM.  I don't know if you should call it Swap or swap(), whatever you
think is fine.

(Also, I realized that the reason std::queue doesn't supply a swap()
because it takes the container type as a template parameter.  While
deque supports swap, you could use other sequence containers that
don't).

On Wed, Sep 2, 2009 at 8:37 PM, <eric.rannaud@gmail.com> wrote:
> There is either a typo or some missing code in the Swap() method.
>
>
> http://codereview.chromium.org/190006/diff/1/2
> File base/message_loop.h (right):
>
> http://codereview.chromium.org/190006/diff/1/2#newcode287
> Line 287: c.swap(queue->c);
> What is c?
>
> http://codereview.chromium.org/190006
>

[brettw, 2009-09-02 20:37:16]

I think we should use Swap() as a member function to comply with Google style.

Sometimes you need to make a swap available for STL operations (it uses it as a
non-member function) in which case it should be lower case. STL will expect a
swap like
  void swap(T& a, T& b);
outside of your class.