Simplify partitionReallocGeneric() implementation

Simplify partitionReallocGeneric() implementation

Using the new partitionAllocActualSize() and partitionAllocGetSize()
the implementation of partitionReallocGeneric() can be made quite a
bit simpler and more high-level.

R=cevans@chromium.org
BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=167997

[Jens Widell, 2014-02-25 19:50:13]

Just a minor cleanup.

[Chris Evans, 2014-02-26 00:22:51]

I do agree that the overall flow of the function is easier with the new helper
APIs used.

A few things to look into.

https://codereview.chromium.org/180293003/diff/1/Source/wtf/PartitionAlloc.cpp
File Source/wtf/PartitionAlloc.cpp (left):

https://codereview.chromium.org/180293003/diff/1/Source/wtf/PartitionAlloc.cp...
Source/wtf/PartitionAlloc.cpp:778: // TODO: note that tcmalloc will "ignore" a
downsizing realloc() unless the
This TODO is not TO-DONE :) so I'm not sure why it is removed?

https://codereview.chromium.org/180293003/diff/1/Source/wtf/PartitionAlloc.cp...
Source/wtf/PartitionAlloc.cpp:789: // TODO: for a downsize on a direct mapped
allocation, we really should
This TODO is also not TO-DONE. So we might want to keep it. But can you fix it?
It should say "for an upsize or downsize on a direct mapped allocation, we
should really try and resize it in-place".

https://codereview.chromium.org/180293003/diff/1/Source/wtf/PartitionAlloc.cpp
File Source/wtf/PartitionAlloc.cpp (right):

https://codereview.chromium.org/180293003/diff/1/Source/wtf/PartitionAlloc.cp...
Source/wtf/PartitionAlloc.cpp:780: size_t actualNewSize =
partitionAllocActualSize(root, newSize);
Don't we need partitionCookieSizeAdjustAdd() around newSize?

https://codereview.chromium.org/180293003/diff/1/Source/wtf/PartitionAlloc.cp...
Source/wtf/PartitionAlloc.cpp:792: size_t copySize = actualOldSize;
I think the loss of the partitionCookieSizeAdjustSubtract() call here might
cause failure in debug mode?

[Jens Widell, 2014-02-26 05:45:44]

https://codereview.chromium.org/180293003/diff/1/Source/wtf/PartitionAlloc.cpp
File Source/wtf/PartitionAlloc.cpp (left):

https://codereview.chromium.org/180293003/diff/1/Source/wtf/PartitionAlloc.cp...
Source/wtf/PartitionAlloc.cpp:778: // TODO: note that tcmalloc will "ignore" a
downsizing realloc() unless the
On 2014/02/26 00:22:52, Chris Evans wrote:
> This TODO is not TO-DONE :) so I'm not sure why it is removed?

Sorry about that; quite sloppy of me. Will restore.

https://codereview.chromium.org/180293003/diff/1/Source/wtf/PartitionAlloc.cpp
File Source/wtf/PartitionAlloc.cpp (right):

https://codereview.chromium.org/180293003/diff/1/Source/wtf/PartitionAlloc.cp...
Source/wtf/PartitionAlloc.cpp:780: size_t actualNewSize =
partitionAllocActualSize(root, newSize);
On 2014/02/26 00:22:52, Chris Evans wrote:
> Don't we need partitionCookieSizeAdjustAdd() around newSize?

We don't need it for the comparison to actualOldSize; both values are actual
usable space and thus "inside" the cookies.

There are partitionCookieSizeAdjust{Add,Subtract} calls first and last in
partitionAllocActualSize().

https://codereview.chromium.org/180293003/diff/1/Source/wtf/PartitionAlloc.cp...
Source/wtf/PartitionAlloc.cpp:792: size_t copySize = actualOldSize;
On 2014/02/26 00:22:52, Chris Evans wrote:
> I think the loss of the partitionCookieSizeAdjustSubtract() call here might
> cause failure in debug mode?

I don't see it. This function is now only using "public" functions, so ought to
be unaffected by the cookie details the same way other client code using the
public API is.

Looking at the tests I can't really see any checks that we actually copied all
the bytes we were supposed to, only checks that we copied the first byte, and
one check that we didn't copy too much. I should increase coverage a bit along
with this patch.

[Jens Widell, 2014-02-26 06:35:25]

Patch updated with TODO comments and some addition test coverage.

[Chris Evans, 2014-02-26 20:56:54]

LGTM with note.

https://codereview.chromium.org/180293003/diff/20001/Source/wtf/PartitionAllo...
File Source/wtf/PartitionAllocTest.cpp (right):

https://codereview.chromium.org/180293003/diff/20001/Source/wtf/PartitionAllo...
Source/wtf/PartitionAllocTest.cpp:690: EXPECT_EQ(WTF::kUninitializedByte,
static_cast<unsigned char>(charPtr[size - 1]));
I think "size - 1" is technically out of bounds here.
It's going to work out because of bucket sizing and the fact we only write debug
cookies at well-aligned addresses.

Feel free to leave it in or remove it.

[Jens Widell, 2014-02-27 07:18:12]

https://codereview.chromium.org/180293003/diff/20001/Source/wtf/PartitionAllo...
File Source/wtf/PartitionAllocTest.cpp (right):

https://codereview.chromium.org/180293003/diff/20001/Source/wtf/PartitionAllo...
Source/wtf/PartitionAllocTest.cpp:690: EXPECT_EQ(WTF::kUninitializedByte,
static_cast<unsigned char>(charPtr[size - 1]));
On 2014/02/26 20:56:55, Chris Evans wrote:
> I think "size - 1" is technically out of bounds here.
> It's going to work out because of bucket sizing and the fact we only write
debug
> cookies at well-aligned addresses.

The test sets 'size' to be a "perfect fit" (and checks that it is using
partitionAllocActualSize().) I think it makes sense for the test to assume that
requesting 'size - 1' bytes then will actually give 'size' bytes, given the
basic design of the allocator.

The cookies are not just written at well-aligned addresses, they are written
inside the outer edges of the slot (as opposed to outside the outer edges of the
allocation.) This is in fact a requirement for partitionAllocGetSize()'s
returned size to be safe to use without trampling the cookies.

> Feel free to leave it in or remove it.

I'll leave it in, then. :-)

[Jens Widell, 2014-02-27 07:18:24]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-02-27 07:18:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jl@opera.com/180293003/20001

[commit-bot: I haz the power, 2014-02-27 09:27:21]

Change committed as 167997