Add ListContainer::AppendByMoving

Add ListContainer::AppendByMoving

This patch implements "void ListContainer::AppendByMoving(T item)" for
appending items to a ListContainer without copying. Because the source
item will not be destructed when moving to the destination, a new item
is constructed in-place to prevent double-destruction and dev sadness.
To support appending derived types when only the base type is known we
memcpy the largest possible element size.

BUG=484943
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/737f36b52ce9137b242b8259b8f440c61e6b60ba
Cr-Commit-Position: refs/heads/master@{#335826}

[pdr., 2015-06-23 05:16:37]

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

[danakj, 2015-06-23 20:01:13]

LGTM with a bunch of comments about style/comments

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container.h
File cc/base/list_container.h (right):

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container....
cc/base/list_container.h:288: // Appends a new item without copying. The
original item will not be
nit: add whitespace above this

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container....
cc/base/list_container.h:289: // destructed and will be replaced with a new
DerivedElementType.
Can you also mention that the DerivedElementType does not have to match the type
of the original item, since we will copy the full allocated block of bytes?

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container....
cc/base/list_container.h:294: memcpy(new_item, (void*)other_item,
max_size_for_derived_class);
why do you have to (void*)? and if so, use static_cast.

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container....
cc/base/list_container.h:295: new (other_item) DerivedElementType;
can you leave a comment explaining this is overwriting the old memory with a new
DerivedElementType?

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
File cc/base/list_container_unittest.cc (right):

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
cc/base/list_container_unittest.cc:850: 
Can you VerifyExpectations here that it wasn't destructed yet?

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
cc/base/list_container_unittest.cc:851: EXPECT_CALL(*de_1, Destruct());
Then do this as the last step in the test?

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
cc/base/list_container_unittest.cc:873: list_1.AppendByMoving(list_2.back());
Can you change the value of the item before moving it for one of these cases, so
we can see that the old element is being replaced, not just staying the same?
(And a comment explaining what you're doing pls)

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
cc/base/list_container_unittest.cc:881: EXPECT_EQ(2u, list_1.size());
Can you leave a comment explaining these numbers?

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
cc/base/list_container_unittest.cc:886: class LongSimpleDerivedElement : public
SimpleDerivedElement {
whitespace before the class

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
cc/base/list_container_unittest.cc:889: void set_all_values(unsigned long value)
{
SetAllValues

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
cc/base/list_container_unittest.cc:893: bool all_values_equal_to(size_t value)
const {
AreAllValuesEqualTo

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
cc/base/list_container_unittest.cc:906: class
LongSimpleDerivedElementConstructMagicNumber
whitespace before the class pls

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
cc/base/list_container_unittest.cc:909:
LongSimpleDerivedElementConstructMagicNumber() : LongSimpleDerivedElement() {
you don't need to explicitly call default constructor right?

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
cc/base/list_container_unittest.cc:933: list.AppendByMoving(list.front());
Can you comment explaining what you are testing here? (The fact that the type
passed is a base class which is smaller than what is actually being copied)

[danakj, 2015-06-23 20:01:44]

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
File cc/base/list_container_unittest.cc (right):

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
cc/base/list_container_unittest.cc:905: const unsigned long
kMagicNumberToUseForLongSimpleDerivedElement = -1618ul;
e:\b\build\slave\win\build\src\cc\base\list_container_unittest.cc(905) : warning
C4146: unary minus operator applied to unsigned type, result still unsigned


Use a positive value here?

[pdr., 2015-06-23 21:42:43]

Thanks for the detailed review.

PTAL

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container.h
File cc/base/list_container.h (right):

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container....
cc/base/list_container.h:288: // Appends a new item without copying. The
original item will not be
On 2015/06/23 at 20:01:12, danakj wrote:
> nit: add whitespace above this

Done. I also added spaces to the 3 others above (ReplaceExistingElement,
AllocateAndCopyFrom, AllocateAndConstruct) for consistency.

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container....
cc/base/list_container.h:289: // destructed and will be replaced with a new
DerivedElementType.
On 2015/06/23 at 20:01:12, danakj wrote:
> Can you also mention that the DerivedElementType does not have to match the
type of the original item, since we will copy the full allocated block of bytes?

Done.

Comment now reads:
// Appends a new item without copying. The original item will not be
// destructed and will be replaced with a new DerivedElementType. The
// DerivedElementType does not have to match the moved type as a full block
// of memory will be moved (up to MaxSizeForDerivedClass()).

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container....
cc/base/list_container.h:294: memcpy(new_item, (void*)other_item,
max_size_for_derived_class);
On 2015/06/23 at 20:01:12, danakj wrote:
> why do you have to (void*)? and if so, use static_cast.

The c-style cats have been replaced with static_cats.

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container....
cc/base/list_container.h:295: new (other_item) DerivedElementType;
On 2015/06/23 at 20:01:12, danakj wrote:
> can you leave a comment explaining this is overwriting the old memory with a
new DerivedElementType?

Done. The comment now reads:
// Construct a new element in-pace so it can be destructed safely.

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
File cc/base/list_container_unittest.cc (right):

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
cc/base/list_container_unittest.cc:850: 
On 2015/06/23 at 20:01:12, danakj wrote:
> Can you VerifyExpectations here that it wasn't destructed yet?

Done.

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
cc/base/list_container_unittest.cc:851: EXPECT_CALL(*de_1, Destruct());
On 2015/06/23 at 20:01:12, danakj wrote:
> Then do this as the last step in the test?

Done (Now the last step in a sequence.)

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
cc/base/list_container_unittest.cc:873: list_1.AppendByMoving(list_2.back());
On 2015/06/23 at 20:01:13, danakj wrote:
> Can you change the value of the item before moving it for one of these cases,
so we can see that the old element is being replaced, not just staying the same?
(And a comment explaining what you're doing pls)

Done.

I needed to add a new constant, kMagicNumberToUseForSimpleDerivedElementThree,
as using One or Two could hide bugs.

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
cc/base/list_container_unittest.cc:881: EXPECT_EQ(2u, list_1.size());
On 2015/06/23 at 20:01:13, danakj wrote:
> Can you leave a comment explaining these numbers?

Done. The comment now reads:
// AppendByMoving replaces the source element with a new derived element so
// we do not expect sizes to shrink after AppendByMoving is called.
EXPECT_EQ(2u, list_1.size()); // One direct allocation and one AppendByMoving.
EXPECT_EQ(1u, list_2.size()); // One AppendByMoving.

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
cc/base/list_container_unittest.cc:886: class LongSimpleDerivedElement : public
SimpleDerivedElement {
On 2015/06/23 at 20:01:12, danakj wrote:
> whitespace before the class

Done. The new whitespace looks like:

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
cc/base/list_container_unittest.cc:889: void set_all_values(unsigned long value)
{
On 2015/06/23 at 20:01:12, danakj wrote:
> SetAllValues

Done.

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
cc/base/list_container_unittest.cc:893: bool all_values_equal_to(size_t value)
const {
On 2015/06/23 at 20:01:12, danakj wrote:
> AreAllValuesEqualTo

Done.

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
cc/base/list_container_unittest.cc:905: const unsigned long
kMagicNumberToUseForLongSimpleDerivedElement = -1618ul;
On 2015/06/23 at 20:01:44, danakj wrote:
> e:\b\build\slave\win\build\src\cc\base\list_container_unittest.cc(905) :
warning C4146: unary minus operator applied to unsigned type, result still
unsigned
> 
> 
> Use a positive value here?

Done.

I was hoping to use a value that would be sensitive to begin stomped (mostly
1's) on but that's probably overkill.

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
cc/base/list_container_unittest.cc:906: class
LongSimpleDerivedElementConstructMagicNumber
On 2015/06/23 at 20:01:12, danakj wrote:
> whitespace before the class pls

Done.

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
cc/base/list_container_unittest.cc:909:
LongSimpleDerivedElementConstructMagicNumber() : LongSimpleDerivedElement() {
On 2015/06/23 at 20:01:13, danakj wrote:
> you don't need to explicitly call default constructor right?

Good catch. Fixed.

I also updated SimpleDerivedElementConstructMagicNumberOne and
SimpleDerivedElementConstructMagicNumberTwo which had this mistake as well.

https://codereview.chromium.org/1202153002/diff/40001/cc/base/list_container_...
cc/base/list_container_unittest.cc:933: list.AppendByMoving(list.front());
On 2015/06/23 at 20:01:12, danakj wrote:
> Can you comment explaining what you are testing here? (The fact that the type
passed is a base class which is smaller than what is actually being copied)

Done. The comment now reads:
// Test that moving a simple derived element actually moves enough data so
// that the LongSimpleDerivedElement at this location is entirely moved.

[danakj, 2015-06-23 22:20:08]

https://codereview.chromium.org/1202153002/diff/60001/cc/base/list_container.h
File cc/base/list_container.h (right):

https://codereview.chromium.org/1202153002/diff/60001/cc/base/list_container....
cc/base/list_container.h:300: // Construct a new element in-pace so it can be
destructed safely.
in-pace

https://codereview.chromium.org/1202153002/diff/60001/cc/base/list_container_...
File cc/base/list_container_unittest.cc (right):

https://codereview.chromium.org/1202153002/diff/60001/cc/base/list_container_...
cc/base/list_container_unittest.cc:850: testing::InSequence s;
You can do this
https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/layer_test...

[pdr., 2015-06-23 22:31:07]

The CQ bit was checked by pdr@chromium.org

[pdr., 2015-06-23 22:31:08]

TY, off to CQ

[pdr., 2015-06-23 22:31:08]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1202153002/#ps80001
(title: "Address reviewer comments")

[commit-bot: I haz the power, 2015-06-23 22:34:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1202153002/80001

[commit-bot: I haz the power, 2015-06-24 00:23:47]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-24 00:25:50]

Patchset 5 (id:??) landed as
https://crrev.com/737f36b52ce9137b242b8259b8f440c61e6b60ba
Cr-Commit-Position: refs/heads/master@{#335826}

[jbroman, 2015-06-29 14:04:37]

(retroactive, sorry)

This is a very very big precondition to add (that the object can be safely moved
with memcpy), and one that doesn't seem to be compile-time checked or even
commented.

In Blink, Vector uses a traits struct (VectorTraits<T>::canMoveWithMemcpy, which
ultimately defaults to __has_trivial_assign(T)). WDYT of me at least filing a
bug to check this, since this could result in rather weird bugs if, for
instance, a list container ever contained something that vends WeakPtrs.

[danakj, 2015-06-29 18:01:35]

Hm, weak pointers.

It looks like canMoveWithMemcpy ultimately uses __has_trivial_assign, which
would return false if the if the class has any virtual functions.
http://docs.embarcadero.com/products/rad_studio/delphiAndcpp2009/HelpUpdate2/...

Maybe the right way to resolve this is to use moveable display items and
std::move once we can do that. Should we file a bug and put a warning/TODO
about it?

On Mon, Jun 29, 2015 at 7:04 AM, <jbroman@chromium.org> wrote:

> (retroactive, sorry)
>
> This is a very very big precondition to add (that the object can be safely
> moved
> with memcpy), and one that doesn't seem to be compile-time checked or even
> commented.
>
> In Blink, Vector uses a traits struct (VectorTraits<T>::canMoveWithMemcpy,
> which
> ultimately defaults to __has_trivial_assign(T)). WDYT of me at least
> filing a
> bug to check this, since this could result in rather weird bugs if, for
> instance, a list container ever contained something that vends WeakPtrs.
>
> https://codereview.chromium.org/1202153002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.