TaskScheduler: Avoid Sequence refcount bump in GetWork()

TaskScheduler: Avoid Sequence refcount bump in GetWork()

vmpstr@ discovered an implicit copy constructor resulting
in a refcount bump in https://codereview.chromium.org/1895903003/
and made it explicit in https://codereview.chromium.org/1901223003/
this CL removes the need for it via a couple changes:

1) Make SequenceAndSortKey move-only
 - Allows it to be in std::priority_queue without std::unique_ptr's
  - Less pointer chasing == yay?!
 - Removing need for custom SequenceAndSortKeyComparator.
2) Add a take_sequence() method to it:
 - Allows to take its Sequence without refbump on Pop()
3) Make it a private implementation detail and make Transaction's API:
 - Push(sequence,sort_key), PeekSortKey(), PopSequence()
4) Do fdoray's TODO on adding IsEmpty() to Transaction's API:
 - Removes need for null SequenceAndSortKey (required change to allow for PeekSortKey).
5) Make SequenceSortKey copyable.
 - Required for SequenceAndSortKey's move constructor
 - Means it turns into a class with operator== for tests instead of custom
   test member verifiers.

Might have been able to split these up but they all line up fairly nicely
and don't amount to a crazy amount of code so here it is :-).

BUG=553459
Committed: https://crrev.com/eaec122b5f9b9f1689e9da9d32e53939921b6f58
Cr-Commit-Position: refs/heads/master@{#390451}

[gab, 2016-04-20 18:44:24]

Francois/Rob

Please don't thoroughly review the files just yet but instead let me know what
you think of the following items:

0) Make SequenceSortKey and SequenceAndSortKey into classes with private
non-const members:
 - Allows copy or move without crazy const-casting. Immutable property still
valid if all methods are const.

1) (0) imposes either getters or operator== on these classes for tests, which
one do we prefer? I originally shy'ed away from operator== but am now thinking
an inlined version right above the members could be nice? Especially since this
is in base::internal and its usage should thus not spread (like we were worried
for TaskTraits).

1) Make SequenceAndSortKey move-only:
  - Allows to get rid of std::unique_ptr usage in std::priority_queue and custom
comparator
  - Allows for Pop() with return value without copy -- more below.

2) Add SequenceAndSortKey::take_sequence() to allow for move of |sequence_|
without refbump (i.e. copy) as was recently done in
https://codereview.chromium.org/1901223003.

3) I was originally going to get rid of Peek() and only have Pop() with return
value but Francois pointed out we need Peek() when comparing multiple PQs, but
Peek() is ugly as its const& and we thus can't move the |sequence_| out but its
also confusing to have both Peek() and Pop() with similar return values, so how
about:

3.1) Change Peek()/Pop() to PeekPriority() and PopSequence()?

[fdoray, 2016-04-20 19:42:06]

On 2016/04/20 18:44:24, gab wrote:
> Francois/Rob
> 
> Please don't thoroughly review the files just yet but instead let me know what
> you think of the following items:
> 
> 0) Make SequenceSortKey and SequenceAndSortKey into classes with private
> non-const members:
>  - Allows copy or move without crazy const-casting. Immutable property still
> valid if all methods are const.

I agree to make SequenceAndSortKey move-only, but I think SequenceSortKey should
stay copyable (its move constructor/operator= would do a copy anyway).

SequenceSortKey will no longer be immutable. It will become possible to modify
it using operator=. However, I think that the benefits of no longer having a PQ
of std::unique_ptr are worth it.

Note that we don't do crazy const-casting in PriorityQueue. We do it in
DelayedTaskManager.

> 
> 1) (0) imposes either getters or operator== on these classes for tests, which
> one do we prefer? I originally shy'ed away from operator== but am now thinking
> an inlined version right above the members could be nice? Especially since
this
> is in base::internal and its usage should thus not spread (like we were
worried
> for TaskTraits).
> 

Why would we need operator== for tests? We don't need it currently.

> 1) Make SequenceAndSortKey move-only:
>   - Allows to get rid of std::unique_ptr usage in std::priority_queue and
custom
> comparator

Getting rid of std::unique_ptr in std::priority_queue will potentially improve
performance (less pointer chasing).

>   - Allows for Pop() with return value without copy -- more below.
> 
> 2) Add SequenceAndSortKey::take_sequence() to allow for move of |sequence_|
> without refbump (i.e. copy) as was recently done in
> https://codereview.chromium.org/1901223003.
> 

If we do (3), take_sequence() would only be used by
PriorityQueue::Transaction::PopSequence(). Do we really need it or could we just
std::move the Sequence out of its SequenceAndSortKey?

> 3) I was originally going to get rid of Peek() and only have Pop() with return
> value but Francois pointed out we need Peek() when comparing multiple PQs, but
> Peek() is ugly as its const& and we thus can't move the |sequence_| out but
its
> also confusing to have both Peek() and Pop() with similar return values, so
how
> about:
> 
> 3.1) Change Peek()/Pop() to PeekPriority() and PopSequence()?

I like this API. It allows SequenceAndSortKey to become private to PriorityQueue
and callers don't have to worry about extra ref bumps.

Unfortunately, I think that we will have to use a const_cast in PopSequence.
There is no way to std::move something out of a std::priority_queue without a
const_cast.

[robliao, 2016-04-20 20:22:43]

On 2016/04/20 19:42:06, fdoray wrote:
> On 2016/04/20 18:44:24, gab wrote:
> > Francois/Rob
> > 
> > Please don't thoroughly review the files just yet but instead let me know
what
> > you think of the following items:
> > 
> > 0) Make SequenceSortKey and SequenceAndSortKey into classes with private
> > non-const members:
> >  - Allows copy or move without crazy const-casting. Immutable property still
> > valid if all methods are const.
> 
> I agree to make SequenceAndSortKey move-only, but I think SequenceSortKey
should
> stay copyable (its move constructor/operator= would do a copy anyway).
> 
> SequenceSortKey will no longer be immutable. It will become possible to modify
> it using operator=. However, I think that the benefits of no longer having a
PQ
> of std::unique_ptr are worth it.
> 
> Note that we don't do crazy const-casting in PriorityQueue. We do it in
> DelayedTaskManager.
> 
> > 
> > 1) (0) imposes either getters or operator== on these classes for tests,
which
> > one do we prefer? I originally shy'ed away from operator== but am now
thinking
> > an inlined version right above the members could be nice? Especially since
> this
> > is in base::internal and its usage should thus not spread (like we were
> worried
> > for TaskTraits).
> > 
> 
> Why would we need operator== for tests? We don't need it currently.
> 
> > 1) Make SequenceAndSortKey move-only:
> >   - Allows to get rid of std::unique_ptr usage in std::priority_queue and
> custom
> > comparator
> 
> Getting rid of std::unique_ptr in std::priority_queue will potentially improve
> performance (less pointer chasing).
> 
> >   - Allows for Pop() with return value without copy -- more below.
> > 
> > 2) Add SequenceAndSortKey::take_sequence() to allow for move of |sequence_|
> > without refbump (i.e. copy) as was recently done in
> > https://codereview.chromium.org/1901223003.
> > 
> 
> If we do (3), take_sequence() would only be used by
> PriorityQueue::Transaction::PopSequence(). Do we really need it or could we
just
> std::move the Sequence out of its SequenceAndSortKey?
> 
> > 3) I was originally going to get rid of Peek() and only have Pop() with
return
> > value but Francois pointed out we need Peek() when comparing multiple PQs,
but
> > Peek() is ugly as its const& and we thus can't move the |sequence_| out but
> its
> > also confusing to have both Peek() and Pop() with similar return values, so
> how
> > about:
> > 
> > 3.1) Change Peek()/Pop() to PeekPriority() and PopSequence()?
> 
> I like this API. It allows SequenceAndSortKey to become private to
PriorityQueue
> and callers don't have to worry about extra ref bumps.
> 
> Unfortunately, I think that we will have to use a const_cast in PopSequence.
> There is no way to std::move something out of a std::priority_queue without a
> const_cast.

0) I've no qualms turning this into a class. I actually prefer this design
pattern :-) and it gives us copy-constructable copy-assignable with const
guarantees if we need it.
1) Agreed. Do we need operator== for these classes? AFAIK SeqeunceSortKey has
all the comparators it needs.
2) In an exposed member world, this would normally be a move-assign, so sgtm.
3) I think it's fine to have peek and pop both return the same type. This is
well established in other implementations and would help remove some Peek/Pop
combinations.

[gab, 2016-04-25 16:52:36]

Sorry for the delay, replies to some points below:


On 2016/04/20 20:22:43, robliao wrote:
> On 2016/04/20 19:42:06, fdoray wrote:
> > On 2016/04/20 18:44:24, gab wrote:
> > > Francois/Rob
> > > 
> > > Please don't thoroughly review the files just yet but instead let me know
> what
> > > you think of the following items:
> > > 
> > > 0) Make SequenceSortKey and SequenceAndSortKey into classes with private
> > > non-const members:
> > >  - Allows copy or move without crazy const-casting. Immutable property
still
> > > valid if all methods are const.
> > 
> > I agree to make SequenceAndSortKey move-only, but I think SequenceSortKey
> should
> > stay copyable (its move constructor/operator= would do a copy anyway).

Sorry I wasn't clear, but yes SequenceSortKey remains copyable (see CL).

> > 
> > SequenceSortKey will no longer be immutable. It will become possible to
modify
> > it using operator=. 

That's okay, it's still somewhat immutable if all accessors are const.

> However, I think that the benefits of no longer having a
> PQ
> > of std::unique_ptr are worth it.
> > 
> > Note that we don't do crazy const-casting in PriorityQueue. We do it in
> > DelayedTaskManager.

Sorry the current const-casting is not what I meant by crazy const-casting. What
I meant by it
is that implementing a move operator (or assignment operator) when members are
const requires const-casting 
members to do the assignment and const-casting incoming members to allow
std::move...!

i.e. 
  foo_ = std::move(other.foo_))
has to be
  const_cast<Foo&>(foo_) = std::move(const_cast<Foo&>(other.foo_));
...!

> > 
> > > 
> > > 1) (0) imposes either getters or operator== on these classes for tests,
> which
> > > one do we prefer? I originally shy'ed away from operator== but am now
> thinking
> > > an inlined version right above the members could be nice? Especially since
> > this
> > > is in base::internal and its usage should thus not spread (like we were
> > worried
> > > for TaskTraits).
> > > 
> > 
> > Why would we need operator== for tests? We don't need it currently.

We compare members directly in tests (e.g. EXPECT_SEQUENCE_AND_SORT_KEY_EQ), but
now
that they're private we either need getters for each member just for the sake of
tests
(which is confusing on SequenceAndSortKey say if we have both sequence() and
take_sequence())
or we could just have operator== (which also makes the test cleaner as we can
just use EXPECT_EQ instead of custom macro + scope magic).

> > 
> > > 1) Make SequenceAndSortKey move-only:
> > >   - Allows to get rid of std::unique_ptr usage in std::priority_queue and
> > custom
> > > comparator
> > 
> > Getting rid of std::unique_ptr in std::priority_queue will potentially
improve
> > performance (less pointer chasing).
> > 
> > >   - Allows for Pop() with return value without copy -- more below.
> > > 
> > > 2) Add SequenceAndSortKey::take_sequence() to allow for move of
|sequence_|
> > > without refbump (i.e. copy) as was recently done in
> > > https://codereview.chromium.org/1901223003.
> > > 
> > 
> > If we do (3), take_sequence() would only be used by
> > PriorityQueue::Transaction::PopSequence(). Do we really need it or could we
> just
> > std::move the Sequence out of its SequenceAndSortKey?

We need take_sequence() if the members are private (which they need to be IMO if
they're not going to be const).

> > 
> > > 3) I was originally going to get rid of Peek() and only have Pop() with
> return
> > > value but Francois pointed out we need Peek() when comparing multiple PQs,
> but
> > > Peek() is ugly as its const& and we thus can't move the |sequence_| out
but
> > its
> > > also confusing to have both Peek() and Pop() with similar return values,
so
> > how
> > > about:
> > > 
> > > 3.1) Change Peek()/Pop() to PeekPriority() and PopSequence()?
> > 
> > I like this API. It allows SequenceAndSortKey to become private to
> PriorityQueue
> > and callers don't have to worry about extra ref bumps.
> > 
> > Unfortunately, I think that we will have to use a const_cast in PopSequence.
> > There is no way to std::move something out of a std::priority_queue without
a
> > const_cast.
> 
> 0) I've no qualms turning this into a class. I actually prefer this design
> pattern :-) and it gives us copy-constructable copy-assignable with const
> guarantees if we need it.
> 1) Agreed. Do we need operator== for these classes? AFAIK SeqeunceSortKey has
> all the comparators it needs.
> 2) In an exposed member world, this would normally be a move-assign, so sgtm.

0,1,2) Discussed above.

> 3) I think it's fine to have peek and pop both return the same type. This is
> well established in other implementations and would help remove some Peek/Pop
> combinations.

Right, but the usage would be confusing as Peek() is const& and used for initial
GetWork checks but then can't
be used for take_sequence() which requires Pop() (for moveable rvalue). Since we
only ever need to Peek for priority
and Pop for sequence, might as well make it do just that and split the types
(plus SequenceAndSoryKey becomes a private impl detail :-)).

From the above I gather that we agree on most of this? I'm going to go ahead and
prepare a real patch set.

Only thing left to settle is whether to introduce an operator==() instead of
getters to deal with now private members for SequenceSortKey, I think it's fine
to (probably don't need it for SequenceAndSortKey if it's going to become an
impl detail and tests will need to work without it anyways).

Thanks!

[fdoray, 2016-04-25 17:03:55]

> Right, but the usage would be confusing as Peek() is const& and used for
initial
> GetWork checks but then can't
> be used for take_sequence() which requires Pop() (for moveable rvalue). Since
we
> only ever need to Peek for priority
> and Pop for sequence, might as well make it do just that and split the types
> (plus SequenceAndSoryKey becomes a private impl detail :-)).
> 
> From the above I gather that we agree on most of this? I'm going to go ahead
and
> prepare a real patch set.
> 
> Only thing left to settle is whether to introduce an operator==() instead of
> getters to deal with now private members for SequenceSortKey, I think it's
fine
> to (probably don't need it for SequenceAndSortKey if it's going to become an
> impl detail and tests will need to work without it anyways).
> 
> Thanks!

I like PeekPriority()/PopSequence().

I think it's nice to have operator==. We already have operator< and operator> so
we don't expose anything new.

[gab, 2016-04-26 23:08:53]

Patchset #2 (id:20001) has been deleted

[gab, 2016-04-26 23:19:10]

Description was changed from

==========
Make SequenceAndSortKey move-only.

BUG=553459
==========

to

==========
TaskScheduler: Avoid Sequence refcount bump in GetWork()

vmpstr@ discovered an implicit copy constructor resulting
in a refcount bump in https://codereview.chromium.org/1895903003/
and made it explicit in https://codereview.chromium.org/1901223003/
this CL removes the need for it via a couple changes:

1) Make SequenceAndSortKey move-only
 - Allows it to be in std::priority_queue with std::unique_ptr's
 - Removing need for custom SequenceAndSortKeyComparator.
2) Add a take_sequence() method to it:
 - Allows to take its Sequence without refbump on Pop()
3) Make it a private implementation detail and make Transaction's API:
 - Push(sequence,sort_key), PeekSortKey(), PopSequence()
4) Do fdoray's TODO on adding IsEmpty() to Transaction's API:
 - Removes need for null SequenceAndSortKey (required for PeekSortKey).

Might have been able to split these up but they all line fairly nicely
and don't line up to a crazy amount of code so here it is :-).

BUG=553459
==========

[gab, 2016-04-26 23:19:43]

Description was changed from

==========
TaskScheduler: Avoid Sequence refcount bump in GetWork()

vmpstr@ discovered an implicit copy constructor resulting
in a refcount bump in https://codereview.chromium.org/1895903003/
and made it explicit in https://codereview.chromium.org/1901223003/
this CL removes the need for it via a couple changes:

1) Make SequenceAndSortKey move-only
 - Allows it to be in std::priority_queue with std::unique_ptr's
 - Removing need for custom SequenceAndSortKeyComparator.
2) Add a take_sequence() method to it:
 - Allows to take its Sequence without refbump on Pop()
3) Make it a private implementation detail and make Transaction's API:
 - Push(sequence,sort_key), PeekSortKey(), PopSequence()
4) Do fdoray's TODO on adding IsEmpty() to Transaction's API:
 - Removes need for null SequenceAndSortKey (required for PeekSortKey).

Might have been able to split these up but they all line fairly nicely
and don't line up to a crazy amount of code so here it is :-).

BUG=553459
==========

to

==========
TaskScheduler: Avoid Sequence refcount bump in GetWork()

vmpstr@ discovered an implicit copy constructor resulting
in a refcount bump in https://codereview.chromium.org/1895903003/
and made it explicit in https://codereview.chromium.org/1901223003/
this CL removes the need for it via a couple changes:

1) Make SequenceAndSortKey move-only
 - Allows it to be in std::priority_queue without std::unique_ptr's
  - Less pointer chasing == yay?!
 - Removing need for custom SequenceAndSortKeyComparator.
2) Add a take_sequence() method to it:
 - Allows to take its Sequence without refbump on Pop()
3) Make it a private implementation detail and make Transaction's API:
 - Push(sequence,sort_key), PeekSortKey(), PopSequence()
4) Do fdoray's TODO on adding IsEmpty() to Transaction's API:
 - Removes need for null SequenceAndSortKey (required for PeekSortKey).

Might have been able to split these up but they all line fairly nicely
and don't line up to a crazy amount of code so here it is :-).

BUG=553459
==========

[gab, 2016-04-26 23:21:18]

Description was changed from

==========
TaskScheduler: Avoid Sequence refcount bump in GetWork()

vmpstr@ discovered an implicit copy constructor resulting
in a refcount bump in https://codereview.chromium.org/1895903003/
and made it explicit in https://codereview.chromium.org/1901223003/
this CL removes the need for it via a couple changes:

1) Make SequenceAndSortKey move-only
 - Allows it to be in std::priority_queue without std::unique_ptr's
  - Less pointer chasing == yay?!
 - Removing need for custom SequenceAndSortKeyComparator.
2) Add a take_sequence() method to it:
 - Allows to take its Sequence without refbump on Pop()
3) Make it a private implementation detail and make Transaction's API:
 - Push(sequence,sort_key), PeekSortKey(), PopSequence()
4) Do fdoray's TODO on adding IsEmpty() to Transaction's API:
 - Removes need for null SequenceAndSortKey (required for PeekSortKey).

Might have been able to split these up but they all line fairly nicely
and don't line up to a crazy amount of code so here it is :-).

BUG=553459
==========

to

==========
TaskScheduler: Avoid Sequence refcount bump in GetWork()

vmpstr@ discovered an implicit copy constructor resulting
in a refcount bump in https://codereview.chromium.org/1895903003/
and made it explicit in https://codereview.chromium.org/1901223003/
this CL removes the need for it via a couple changes:

1) Make SequenceAndSortKey move-only
 - Allows it to be in std::priority_queue without std::unique_ptr's
  - Less pointer chasing == yay?!
 - Removing need for custom SequenceAndSortKeyComparator.
2) Add a take_sequence() method to it:
 - Allows to take its Sequence without refbump on Pop()
3) Make it a private implementation detail and make Transaction's API:
 - Push(sequence,sort_key), PeekSortKey(), PopSequence()
4) Do fdoray's TODO on adding IsEmpty() to Transaction's API:
 - Removes need for null SequenceAndSortKey (required for PeekSortKey).
5) Make SequenceSortKey copyable.
 - Required for SequenceAndSortKey's move constructor
 - Means it turns into a class with operator== for tests instead of custom
   test member verifiers.

Might have been able to split these up but they all line fairly nicely
and don't line up to a crazy amount of code so here it is :-).

BUG=553459
==========

[gab, 2016-04-26 23:21:44]

Description was changed from

==========
TaskScheduler: Avoid Sequence refcount bump in GetWork()

vmpstr@ discovered an implicit copy constructor resulting
in a refcount bump in https://codereview.chromium.org/1895903003/
and made it explicit in https://codereview.chromium.org/1901223003/
this CL removes the need for it via a couple changes:

1) Make SequenceAndSortKey move-only
 - Allows it to be in std::priority_queue without std::unique_ptr's
  - Less pointer chasing == yay?!
 - Removing need for custom SequenceAndSortKeyComparator.
2) Add a take_sequence() method to it:
 - Allows to take its Sequence without refbump on Pop()
3) Make it a private implementation detail and make Transaction's API:
 - Push(sequence,sort_key), PeekSortKey(), PopSequence()
4) Do fdoray's TODO on adding IsEmpty() to Transaction's API:
 - Removes need for null SequenceAndSortKey (required for PeekSortKey).
5) Make SequenceSortKey copyable.
 - Required for SequenceAndSortKey's move constructor
 - Means it turns into a class with operator== for tests instead of custom
   test member verifiers.

Might have been able to split these up but they all line fairly nicely
and don't line up to a crazy amount of code so here it is :-).

BUG=553459
==========

to

==========
TaskScheduler: Avoid Sequence refcount bump in GetWork()

vmpstr@ discovered an implicit copy constructor resulting
in a refcount bump in https://codereview.chromium.org/1895903003/
and made it explicit in https://codereview.chromium.org/1901223003/
this CL removes the need for it via a couple changes:

1) Make SequenceAndSortKey move-only
 - Allows it to be in std::priority_queue without std::unique_ptr's
  - Less pointer chasing == yay?!
 - Removing need for custom SequenceAndSortKeyComparator.
2) Add a take_sequence() method to it:
 - Allows to take its Sequence without refbump on Pop()
3) Make it a private implementation detail and make Transaction's API:
 - Push(sequence,sort_key), PeekSortKey(), PopSequence()
4) Do fdoray's TODO on adding IsEmpty() to Transaction's API:
 - Removes need for null SequenceAndSortKey (required for PeekSortKey).
5) Make SequenceSortKey copyable.
 - Required for SequenceAndSortKey's move constructor
 - Means it turns into a class with operator== for tests instead of custom
   test member verifiers.

Might have been able to split these up but they all line up fairly nicely
and don't amount up to a crazy amount of code so here it is :-).

BUG=553459
==========

[gab, 2016-04-26 23:22:32]

gab@chromium.org changed reviewers:
+ fdoray@chromium.org, robliao@chromium.org

[gab, 2016-04-26 23:22:32]

Here it is, Francois/Rob PTAL, thanks!

[gab, 2016-04-26 23:32:45]

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_thread_pool_impl.cc (right):

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/sch...
base/task_scheduler/scheduler_thread_pool_impl.cc:420: sequence =
std::move(single_threaded_transaction->PopSequence());
Are these moves required? I don't think so per the return value being an rvalue
but I'm always worried about scoped_refptr's copy constructor so just want to
double confirm with you guys..

[robliao, 2016-04-27 00:59:01]

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/pri...
File base/task_scheduler/priority_queue.h (right):

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/pri...
base/task_scheduler/priority_queue.h:42: // lock inter- dependency with
|sequence|.
Nit: interdependency

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_thread_pool_impl.cc (right):

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/sch...
base/task_scheduler/scheduler_thread_pool_impl.cc:420: sequence =
std::move(single_threaded_transaction->PopSequence());
On 2016/04/26 23:32:45, gab wrote:
> Are these moves required? I don't think so per the return value being an
rvalue
> but I'm always worried about scoped_refptr's copy constructor so just want to
> double confirm with you guys..

http://en.cppreference.com/w/cpp/utility/move
The functions that accept rvalue reference parameters (including move
constructors, move assignment operators, and regular member functions such as
std::vector::push_back) are selected, by overload resolution, when called with
rvalue arguments (either prvalues such as a temporary objects or xvalues such as
the one produced by std::move).

PopSequence() is an rvalue, so it looks like that statement applies. 
Copy elision is also on your side. So it looks like you can remove the std::move
here and above.

[fdoray, 2016-04-27 01:05:10]

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/pri...
File base/task_scheduler/priority_queue.cc (right):

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/pri...
base/task_scheduler/priority_queue.cc:38: scoped_refptr<Sequence>&&
take_sequence() {
&& isn't required according to my test program:

struct A {
  A() = default;
  A(const A& other) { LOG(ERROR) << "copy"; }
  A(A&& other) { LOG(ERROR) << "move"; }
  void operator=(const A& other) { LOG(ERROR) << "copy assign"; }
  void operator=(A&& other) { LOG(ERROR) << "move assign"; }
};

struct B {
  A take_A() { return std::move(a); }
  A&& take_A_rvalue() { return std::move(a); }
  A copy_A() { return a; }
  A a;
};

B b;

A a1 = b.take_A_rvalue();
// output: move

A a2 = b.take_A();
// output: move

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/pri...
base/task_scheduler/priority_queue.cc:79:
DCHECK(!outer_queue_->container_.empty());
!IsEmpty()?

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/pri...
base/task_scheduler/priority_queue.cc:85:
DCHECK(!outer_queue_->container_.empty());
!IsEmpty()?

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/pri...
File base/task_scheduler/priority_queue.h (right):

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/pri...
base/task_scheduler/priority_queue.h:48: // invalid the next time that a
Sequence is popped from this PriorityQueue.
s/that a Sequence is popped from this PriorityQueue/that the PriorityQueue is
modified/

Push() invalidates the reference if the std::vector behind the
std::priority_queue is reallocated.

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_thread_pool_impl.cc (right):

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/sch...
base/task_scheduler/scheduler_thread_pool_impl.cc:420: sequence =
std::move(single_threaded_transaction->PopSequence());
On 2016/04/27 00:59:01, robliao wrote:
> On 2016/04/26 23:32:45, gab wrote:
> > Are these moves required? I don't think so per the return value being an
> rvalue
> > but I'm always worried about scoped_refptr's copy constructor so just want
to
> > double confirm with you guys..
> 
> http://en.cppreference.com/w/cpp/utility/move
> The functions that accept rvalue reference parameters (including move
> constructors, move assignment operators, and regular member functions such as
> std::vector::push_back) are selected, by overload resolution, when called with
> rvalue arguments (either prvalues such as a temporary objects or xvalues such
as
> the one produced by std::move).
> 
> PopSequence() is an rvalue, so it looks like that statement applies. 
> Copy elision is also on your side. So it looks like you can remove the
std::move
> here and above.

According to my test program [1], std::move() doesn't change anything here (i.e.
no extra copy or move constructor is invoked without it).

[1]
https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/pri...

[gab, 2016-04-27 20:40:29]

gab@chromium.org changed reviewers:
+ danakj@chromium.org

[gab, 2016-04-27 20:40:30]

@fdoray/robliao: done, PTAL.

@danakj PTAL after https://codereview.chromium.org/1876363004/

Thanks!
Gab

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/pri...
File base/task_scheduler/priority_queue.cc (right):

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/pri...
base/task_scheduler/priority_queue.cc:38: scoped_refptr<Sequence>&&
take_sequence() {
On 2016/04/27 01:05:10, fdoray wrote:
> && isn't required according to my test program:
> 
> struct A {
>   A() = default;
>   A(const A& other) { LOG(ERROR) << "copy"; }
>   A(A&& other) { LOG(ERROR) << "move"; }
>   void operator=(const A& other) { LOG(ERROR) << "copy assign"; }
>   void operator=(A&& other) { LOG(ERROR) << "move assign"; }
> };
> 
> struct B {
>   A take_A() { return std::move(a); }
>   A&& take_A_rvalue() { return std::move(a); }
>   A copy_A() { return a; }
>   A a;
> };
> 
> B b;
> 
> A a1 = b.take_A_rvalue();
> // output: move
> 
> A a2 = b.take_A();
> // output: move

The one thing it did is force the std::move in the impl, i.e.:

A&& take_A_rvalue() { return a; }  // doesn't compile
A take_A() { return a; }  // compiles if |a| has a copy constructor

In my code the compile error for this was:
d:\src\chrome\src\base\task_scheduler\priority_queue.cc(40): error C2440:
'return': cannot convert from 'scoped_refptr<base::internal::Sequence>' to
'scoped_refptr<base::internal::Sequence> &&'
d:\src\chrome\src\base\task_scheduler\priority_queue.cc(40): note: You cannot
bind an lvalue to an rvalue reference

Which means the user of the API can know for sure it's actually given the ref
(which is the contract) instead of a copy of it.

But anyways I just learnt yesterday on chromium-dev that rvalues aren't allowed
by style guide outside of move-constructor/move-assign. So this point is moot
and it's not much worse off without the explicit rvalue :-).

But not being allowed to use explicit rvalues makes this change fairly hard as
the whole point is avoiding a refbump yet this change would totally compile with
many std::move removed, causing more implicit refbumps than the one it's trying
to solve..!

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/pri...
base/task_scheduler/priority_queue.cc:79:
DCHECK(!outer_queue_->container_.empty());
On 2016/04/27 01:05:10, fdoray wrote:
> !IsEmpty()?

Done.

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/pri...
base/task_scheduler/priority_queue.cc:85:
DCHECK(!outer_queue_->container_.empty());
On 2016/04/27 01:05:10, fdoray wrote:
> !IsEmpty()?

Done.

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/pri...
File base/task_scheduler/priority_queue.h (right):

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/pri...
base/task_scheduler/priority_queue.h:42: // lock inter- dependency with
|sequence|.
On 2016/04/27 00:59:01, robliao wrote:
> Nit: interdependency

Done.

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/pri...
base/task_scheduler/priority_queue.h:48: // invalid the next time that a
Sequence is popped from this PriorityQueue.
On 2016/04/27 01:05:10, fdoray wrote:
> s/that a Sequence is popped from this PriorityQueue/that the PriorityQueue is
> modified/
> 
> Push() invalidates the reference if the std::vector behind the
> std::priority_queue is reallocated.

Done.

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_thread_pool_impl.cc (right):

https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/sch...
base/task_scheduler/scheduler_thread_pool_impl.cc:420: sequence =
std::move(single_threaded_transaction->PopSequence());
On 2016/04/27 01:05:10, fdoray wrote:
> On 2016/04/27 00:59:01, robliao wrote:
> > On 2016/04/26 23:32:45, gab wrote:
> > > Are these moves required? I don't think so per the return value being an
> > rvalue
> > > but I'm always worried about scoped_refptr's copy constructor so just want
> to
> > > double confirm with you guys..
> > 
> > http://en.cppreference.com/w/cpp/utility/move
> > The functions that accept rvalue reference parameters (including move
> > constructors, move assignment operators, and regular member functions such
as
> > std::vector::push_back) are selected, by overload resolution, when called
with
> > rvalue arguments (either prvalues such as a temporary objects or xvalues
such
> as
> > the one produced by std::move).
> > 
> > PopSequence() is an rvalue, so it looks like that statement applies. 
> > Copy elision is also on your side. So it looks like you can remove the
> std::move
> > here and above.
> 
> According to my test program [1], std::move() doesn't change anything here
(i.e.
> no extra copy or move constructor is invoked without it).
> 
> [1]
>
https://codereview.chromium.org/1903133003/diff/40001/base/task_scheduler/pri...

Done.

[gab, 2016-04-27 20:40:45]

Description was changed from

==========
TaskScheduler: Avoid Sequence refcount bump in GetWork()

vmpstr@ discovered an implicit copy constructor resulting
in a refcount bump in https://codereview.chromium.org/1895903003/
and made it explicit in https://codereview.chromium.org/1901223003/
this CL removes the need for it via a couple changes:

1) Make SequenceAndSortKey move-only
 - Allows it to be in std::priority_queue without std::unique_ptr's
  - Less pointer chasing == yay?!
 - Removing need for custom SequenceAndSortKeyComparator.
2) Add a take_sequence() method to it:
 - Allows to take its Sequence without refbump on Pop()
3) Make it a private implementation detail and make Transaction's API:
 - Push(sequence,sort_key), PeekSortKey(), PopSequence()
4) Do fdoray's TODO on adding IsEmpty() to Transaction's API:
 - Removes need for null SequenceAndSortKey (required for PeekSortKey).
5) Make SequenceSortKey copyable.
 - Required for SequenceAndSortKey's move constructor
 - Means it turns into a class with operator== for tests instead of custom
   test member verifiers.

Might have been able to split these up but they all line up fairly nicely
and don't amount up to a crazy amount of code so here it is :-).

BUG=553459
==========

to

==========
TaskScheduler: Avoid Sequence refcount bump in GetWork()

vmpstr@ discovered an implicit copy constructor resulting
in a refcount bump in https://codereview.chromium.org/1895903003/
and made it explicit in https://codereview.chromium.org/1901223003/
this CL removes the need for it via a couple changes:

1) Make SequenceAndSortKey move-only
 - Allows it to be in std::priority_queue without std::unique_ptr's
  - Less pointer chasing == yay?!
 - Removing need for custom SequenceAndSortKeyComparator.
2) Add a take_sequence() method to it:
 - Allows to take its Sequence without refbump on Pop()
3) Make it a private implementation detail and make Transaction's API:
 - Push(sequence,sort_key), PeekSortKey(), PopSequence()
4) Do fdoray's TODO on adding IsEmpty() to Transaction's API:
 - Removes need for null SequenceAndSortKey (required for PeekSortKey).
5) Make SequenceSortKey copyable.
 - Required for SequenceAndSortKey's move constructor
 - Means it turns into a class with operator== for tests instead of custom
   test member verifiers.

Might have been able to split these up but they all line up fairly nicely
and don't amount to a crazy amount of code so here it is :-).

BUG=553459
==========

[danakj, 2016-04-27 20:50:59]

> 5) Make SequenceSortKey copyable.
>  - Required for SequenceAndSortKey's move constructor

Is it? Or could it be DISALLOW_COPY_AND_ASSIGN and make it movable?

https://codereview.chromium.org/1903133003/diff/60001/base/task_scheduler/pri...
File base/task_scheduler/priority_queue.cc (right):

https://codereview.chromium.org/1903133003/diff/60001/base/task_scheduler/pri...
base/task_scheduler/priority_queue.cc:38: scoped_refptr<Sequence>&&
take_sequence() {
return by value

https://codereview.chromium.org/1903133003/diff/60001/base/task_scheduler/seq...
File base/task_scheduler/sequence_sort_key.h (right):

https://codereview.chromium.org/1903133003/diff/60001/base/task_scheduler/seq...
base/task_scheduler/sequence_sort_key.h:15: // An immutable representation of
the priority of a Sequence.
maybe you should say "immutable but copyable" or "immutable but assignable"

[gab, 2016-04-28 13:47:16]

On 2016/04/27 20:50:59, danakj wrote:
> > 5) Make SequenceSortKey copyable.
> >  - Required for SequenceAndSortKey's move constructor
> 
> Is it? Or could it be DISALLOW_COPY_AND_ASSIGN and make it movable?

Its members are not moveable (TaskPriority/Timeticks -- POD types essentially)
so copy/move is equivalent, do you still prefer move over copy? From what I
understood the style recommendation was to not go overboard with making
everything moveable when there is no point, but I can see a benefit here if we
don't want to assume that say TimeTicks will never be a moveable type (or want
to be agnostic to the potential addition of members to SequenceSortKey).

https://codereview.chromium.org/1903133003/diff/60001/base/task_scheduler/pri...
File base/task_scheduler/priority_queue.cc (right):

https://codereview.chromium.org/1903133003/diff/60001/base/task_scheduler/pri...
base/task_scheduler/priority_queue.cc:38: scoped_refptr<Sequence>&&
take_sequence() {
On 2016/04/27 20:50:59, danakj wrote:
> return by value

oops that wasn't intended to survive this patch set, thanks!

https://codereview.chromium.org/1903133003/diff/60001/base/task_scheduler/seq...
File base/task_scheduler/sequence_sort_key.h (right):

https://codereview.chromium.org/1903133003/diff/60001/base/task_scheduler/seq...
base/task_scheduler/sequence_sort_key.h:15: // An immutable representation of
the priority of a Sequence.
On 2016/04/27 20:50:59, danakj wrote:
> maybe you should say "immutable but copyable" or "immutable but assignable"

Done.

[gab, 2016-04-28 13:48:55]

Description was changed from

==========
TaskScheduler: Avoid Sequence refcount bump in GetWork()

vmpstr@ discovered an implicit copy constructor resulting
in a refcount bump in https://codereview.chromium.org/1895903003/
and made it explicit in https://codereview.chromium.org/1901223003/
this CL removes the need for it via a couple changes:

1) Make SequenceAndSortKey move-only
 - Allows it to be in std::priority_queue without std::unique_ptr's
  - Less pointer chasing == yay?!
 - Removing need for custom SequenceAndSortKeyComparator.
2) Add a take_sequence() method to it:
 - Allows to take its Sequence without refbump on Pop()
3) Make it a private implementation detail and make Transaction's API:
 - Push(sequence,sort_key), PeekSortKey(), PopSequence()
4) Do fdoray's TODO on adding IsEmpty() to Transaction's API:
 - Removes need for null SequenceAndSortKey (required for PeekSortKey).
5) Make SequenceSortKey copyable.
 - Required for SequenceAndSortKey's move constructor
 - Means it turns into a class with operator== for tests instead of custom
   test member verifiers.

Might have been able to split these up but they all line up fairly nicely
and don't amount to a crazy amount of code so here it is :-).

BUG=553459
==========

to

==========
TaskScheduler: Avoid Sequence refcount bump in GetWork()

vmpstr@ discovered an implicit copy constructor resulting
in a refcount bump in https://codereview.chromium.org/1895903003/
and made it explicit in https://codereview.chromium.org/1901223003/
this CL removes the need for it via a couple changes:

1) Make SequenceAndSortKey move-only
 - Allows it to be in std::priority_queue without std::unique_ptr's
  - Less pointer chasing == yay?!
 - Removing need for custom SequenceAndSortKeyComparator.
2) Add a take_sequence() method to it:
 - Allows to take its Sequence without refbump on Pop()
3) Make it a private implementation detail and make Transaction's API:
 - Push(sequence,sort_key), PeekSortKey(), PopSequence()
4) Do fdoray's TODO on adding IsEmpty() to Transaction's API:
 - Removes need for null SequenceAndSortKey (required change to allow for
PeekSortKey).
5) Make SequenceSortKey copyable.
 - Required for SequenceAndSortKey's move constructor
 - Means it turns into a class with operator== for tests instead of custom
   test member verifiers.

Might have been able to split these up but they all line up fairly nicely
and don't amount to a crazy amount of code so here it is :-).

BUG=553459
==========

[gab, 2016-04-28 13:54:32]

On 2016/04/28 13:47:16, gab wrote:
> On 2016/04/27 20:50:59, danakj wrote:
> > > 5) Make SequenceSortKey copyable.
> > >  - Required for SequenceAndSortKey's move constructor
> > 
> > Is it? Or could it be DISALLOW_COPY_AND_ASSIGN and make it movable?
> 
> Its members are not moveable (TaskPriority/Timeticks -- POD types essentially)
> so copy/move is equivalent, do you still prefer move over copy? From what I
> understood the style recommendation was to not go overboard with making
> everything moveable when there is no point, but I can see a benefit here if we
> don't want to assume that say TimeTicks will never be a moveable type (or want
> to be agnostic to the potential addition of members to SequenceSortKey).

Another thing I'm pondering is whether we should explicitly declare the
copy/move constructor/assign methods (=default; -- whichever one we decide to
use) to match the fact that we now explicitly say "immutable but assignable" and
explicitly intend to allow it. WDYT?

> 
>
https://codereview.chromium.org/1903133003/diff/60001/base/task_scheduler/pri...
> File base/task_scheduler/priority_queue.cc (right):
> 
>
https://codereview.chromium.org/1903133003/diff/60001/base/task_scheduler/pri...
> base/task_scheduler/priority_queue.cc:38: scoped_refptr<Sequence>&&
> take_sequence() {
> On 2016/04/27 20:50:59, danakj wrote:
> > return by value
> 
> oops that wasn't intended to survive this patch set, thanks!
> 
>
https://codereview.chromium.org/1903133003/diff/60001/base/task_scheduler/seq...
> File base/task_scheduler/sequence_sort_key.h (right):
> 
>
https://codereview.chromium.org/1903133003/diff/60001/base/task_scheduler/seq...
> base/task_scheduler/sequence_sort_key.h:15: // An immutable representation of
> the priority of a Sequence.
> On 2016/04/27 20:50:59, danakj wrote:
> > maybe you should say "immutable but copyable" or "immutable but assignable"
> 
> Done.

[fdoray, 2016-04-28 14:32:29]

lgtm with one comment

https://codereview.chromium.org/1903133003/diff/80001/base/task_scheduler/seq...
File base/task_scheduler/sequence_sort_key_unittest.cc (right):

https://codereview.chromium.org/1903133003/diff/80001/base/task_scheduler/seq...
base/task_scheduler/sequence_sort_key_unittest.cc:142: EXPECT_EQ(key_a, key_a);
This test will succeed if operator== and operator!= always return true. You
should test operator== and operator!= separately.

TEST(... OperatorEqual) {
  EXPECT_EQ(key_a, key_a);
  EXPECT_FALSE(key_b == key_a);
  ...
}

TEST(..., OperatorNotEqual) {
  EXPECT_FALSE(key_a != key_a); 
  EXPECT_NE(key_b, key_a);
  ...
}

[gab, 2016-04-28 15:36:55]

https://codereview.chromium.org/1903133003/diff/80001/base/task_scheduler/seq...
File base/task_scheduler/sequence_sort_key_unittest.cc (right):

https://codereview.chromium.org/1903133003/diff/80001/base/task_scheduler/seq...
base/task_scheduler/sequence_sort_key_unittest.cc:142: EXPECT_EQ(key_a, key_a);
On 2016/04/28 14:32:29, fdoray wrote:
> This test will succeed if operator== and operator!= always return true. You
> should test operator== and operator!= separately.
> 
> TEST(... OperatorEqual) {
>   EXPECT_EQ(key_a, key_a);
>   EXPECT_FALSE(key_b == key_a);
>   ...
> }
> 
> TEST(..., OperatorNotEqual) {
>   EXPECT_FALSE(key_a != key_a); 
>   EXPECT_NE(key_b, key_a);
>   ...
> }

Good catch, done :-)

[danakj, 2016-04-28 17:52:24]

LGTM

[danakj, 2016-04-28 17:52:42]

On Thu, Apr 28, 2016 at 6:47 AM, <gab@chromium.org> wrote:

> On 2016/04/27 20:50:59, danakj wrote:
> > > 5) Make SequenceSortKey copyable.
> > > - Required for SequenceAndSortKey's move constructor
> >
> > Is it? Or could it be DISALLOW_COPY_AND_ASSIGN and make it movable?
>
> Its members are not moveable (TaskPriority/Timeticks -- POD types
> essentially)
> so copy/move is equivalent, do you still prefer move over copy? From what I
> understood the style recommendation was to not go overboard with making
> everything moveable when there is no point, but I can see a benefit here
> if we
> don't want to assume that say TimeTicks will never be a moveable type (or
> want
> to be agnostic to the potential addition of members to SequenceSortKey).
>

Ah I see what you're saying. Since no destructor was declared I think that
SequenceSortKey is going to get an implicit move operator as well as copy,
so the default move constructor/operator in SequenceAndSortKey probably
actually moves it. :)

You're right that they are the same though for this class, so I think what
you did is right.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[robliao, 2016-04-28 17:53:42]

lgtm

[gab, 2016-04-28 17:54:21]

The CQ bit was checked by gab@chromium.org

[gab, 2016-04-28 17:54:22]

The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org
Link to the patchset: https://codereview.chromium.org/1903133003/#ps120001
(title: "merge up to r390397")

[commit-bot: I haz the power, 2016-04-28 17:54:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1903133003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1903133003/120001

[commit-bot: I haz the power, 2016-04-28 18:02:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-28 18:02:10]

Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[gab, 2016-04-28 18:47:45]

The CQ bit was checked by gab@chromium.org

[gab, 2016-04-28 18:47:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
robliao@chromium.org, danakj@chromium.org, fdoray@chromium.org
Link to the patchset: https://codereview.chromium.org/1903133003/#ps140001
(title: "fix compile (why did this compile locally?!")

[commit-bot: I haz the power, 2016-04-28 18:48:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1903133003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1903133003/140001

[commit-bot: I haz the power, 2016-04-28 19:44:58]

Description was changed from

==========
TaskScheduler: Avoid Sequence refcount bump in GetWork()

vmpstr@ discovered an implicit copy constructor resulting
in a refcount bump in https://codereview.chromium.org/1895903003/
and made it explicit in https://codereview.chromium.org/1901223003/
this CL removes the need for it via a couple changes:

1) Make SequenceAndSortKey move-only
 - Allows it to be in std::priority_queue without std::unique_ptr's
  - Less pointer chasing == yay?!
 - Removing need for custom SequenceAndSortKeyComparator.
2) Add a take_sequence() method to it:
 - Allows to take its Sequence without refbump on Pop()
3) Make it a private implementation detail and make Transaction's API:
 - Push(sequence,sort_key), PeekSortKey(), PopSequence()
4) Do fdoray's TODO on adding IsEmpty() to Transaction's API:
 - Removes need for null SequenceAndSortKey (required change to allow for
PeekSortKey).
5) Make SequenceSortKey copyable.
 - Required for SequenceAndSortKey's move constructor
 - Means it turns into a class with operator== for tests instead of custom
   test member verifiers.

Might have been able to split these up but they all line up fairly nicely
and don't amount to a crazy amount of code so here it is :-).

BUG=553459
==========

to

==========
TaskScheduler: Avoid Sequence refcount bump in GetWork()

vmpstr@ discovered an implicit copy constructor resulting
in a refcount bump in https://codereview.chromium.org/1895903003/
and made it explicit in https://codereview.chromium.org/1901223003/
this CL removes the need for it via a couple changes:

1) Make SequenceAndSortKey move-only
 - Allows it to be in std::priority_queue without std::unique_ptr's
  - Less pointer chasing == yay?!
 - Removing need for custom SequenceAndSortKeyComparator.
2) Add a take_sequence() method to it:
 - Allows to take its Sequence without refbump on Pop()
3) Make it a private implementation detail and make Transaction's API:
 - Push(sequence,sort_key), PeekSortKey(), PopSequence()
4) Do fdoray's TODO on adding IsEmpty() to Transaction's API:
 - Removes need for null SequenceAndSortKey (required change to allow for
PeekSortKey).
5) Make SequenceSortKey copyable.
 - Required for SequenceAndSortKey's move constructor
 - Means it turns into a class with operator== for tests instead of custom
   test member verifiers.

Might have been able to split these up but they all line up fairly nicely
and don't amount to a crazy amount of code so here it is :-).

BUG=553459
==========

[commit-bot: I haz the power, 2016-04-28 19:44:59]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-04-30 17:20:23]

Patchset 7 (id:??) landed as
https://crrev.com/eaec122b5f9b9f1689e9da9d32e53939921b6f58
Cr-Commit-Position: refs/heads/master@{#390451}

[commit-bot: I haz the power, 2016-04-30 17:57:01]

Description was changed from

==========
TaskScheduler: Avoid Sequence refcount bump in GetWork()

vmpstr@ discovered an implicit copy constructor resulting
in a refcount bump in https://codereview.chromium.org/1895903003/
and made it explicit in https://codereview.chromium.org/1901223003/
this CL removes the need for it via a couple changes:

1) Make SequenceAndSortKey move-only
 - Allows it to be in std::priority_queue without std::unique_ptr's
  - Less pointer chasing == yay?!
 - Removing need for custom SequenceAndSortKeyComparator.
2) Add a take_sequence() method to it:
 - Allows to take its Sequence without refbump on Pop()
3) Make it a private implementation detail and make Transaction's API:
 - Push(sequence,sort_key), PeekSortKey(), PopSequence()
4) Do fdoray's TODO on adding IsEmpty() to Transaction's API:
 - Removes need for null SequenceAndSortKey (required change to allow for
PeekSortKey).
5) Make SequenceSortKey copyable.
 - Required for SequenceAndSortKey's move constructor
 - Means it turns into a class with operator== for tests instead of custom
   test member verifiers.

Might have been able to split these up but they all line up fairly nicely
and don't amount to a crazy amount of code so here it is :-).

BUG=553459
==========

to

==========
TaskScheduler: Avoid Sequence refcount bump in GetWork()

vmpstr@ discovered an implicit copy constructor resulting
in a refcount bump in https://codereview.chromium.org/1895903003/
and made it explicit in https://codereview.chromium.org/1901223003/
this CL removes the need for it via a couple changes:

1) Make SequenceAndSortKey move-only
 - Allows it to be in std::priority_queue without std::unique_ptr's
  - Less pointer chasing == yay?!
 - Removing need for custom SequenceAndSortKeyComparator.
2) Add a take_sequence() method to it:
 - Allows to take its Sequence without refbump on Pop()
3) Make it a private implementation detail and make Transaction's API:
 - Push(sequence,sort_key), PeekSortKey(), PopSequence()
4) Do fdoray's TODO on adding IsEmpty() to Transaction's API:
 - Removes need for null SequenceAndSortKey (required change to allow for
PeekSortKey).
5) Make SequenceSortKey copyable.
 - Required for SequenceAndSortKey's move constructor
 - Means it turns into a class with operator== for tests instead of custom
   test member verifiers.

Might have been able to split these up but they all line up fairly nicely
and don't amount to a crazy amount of code so here it is :-).

BUG=553459

Committed: https://crrev.com/eaec122b5f9b9f1689e9da9d32e53939921b6f58
Cr-Commit-Position: refs/heads/master@{#390451}
==========