One allocation for an SkData which makes a copy.

One allocation for an SkData which makes a copy.

[bungeman-skia, 2013-06-11 20:33:09]

I think this is what is needed for an SkData with a copy to make one alloc.

[reed1, 2013-06-11 21:28:15]

https://codereview.chromium.org/15675025/diff/1/src/core/SkData.cpp
File src/core/SkData.cpp (right):

https://codereview.chromium.org/15675025/diff/1/src/core/SkData.cpp#newcode75
src/core/SkData.cpp:75: // This function intentionally left blank.
SkASSERT(!"don't call me");

https://codereview.chromium.org/15675025/diff/1/src/core/SkData.cpp#newcode81
src/core/SkData.cpp:81: void* context = this->fReleaseProcContext;
Isn't context always == this? can we call free on that instead?

https://codereview.chromium.org/15675025/diff/1/src/core/SkData.cpp#newcode94
src/core/SkData.cpp:94: char* everything =
reinterpret_cast<char*>(sk_malloc_throw(sizeof(SkData) + length));
This is an 80-col file...

[bungeman-skia, 2013-06-11 22:25:32]

Slightly more clever way of doing this.

https://codereview.chromium.org/15675025/diff/1/src/core/SkData.cpp
File src/core/SkData.cpp (right):

https://codereview.chromium.org/15675025/diff/1/src/core/SkData.cpp#newcode75
src/core/SkData.cpp:75: // This function intentionally left blank.
On 2013/06/11 21:28:15, reed1 wrote:
> SkASSERT(!"don't call me");

Removed this function.

https://codereview.chromium.org/15675025/diff/1/src/core/SkData.cpp#newcode81
src/core/SkData.cpp:81: void* context = this->fReleaseProcContext;
On 2013/06/11 21:28:15, reed1 wrote:
> Isn't context always == this? can we call free on that instead?

Hmmmm... yes, we do magically know that. In fact, no one else should be able to
set our fReleaseProcContext to 'this' (without being crazy) because we don't
allow changing fReleaseProcContext and 'this' isn't known until after the user
creates the object. Using this allows removal of the proc as well.

https://codereview.chromium.org/15675025/diff/1/src/core/SkData.cpp#newcode94
src/core/SkData.cpp:94: char* everything =
reinterpret_cast<char*>(sk_malloc_throw(sizeof(SkData) + length));
On 2013/06/11 21:28:15, reed1 wrote:
> This is an 80-col file...

Done.

[reed1, 2013-06-12 13:39:31]

lgtm w/ comment suggestion

https://codereview.chromium.org/15675025/diff/5001/src/core/SkData.cpp
File src/core/SkData.cpp (right):

https://codereview.chromium.org/15675025/diff/5001/src/core/SkData.cpp#newcode84
src/core/SkData.cpp:84: 
The code is good, but lets add a comment block describing what we're doing at a
high-level. e.g. "for speed, combine the SkData and the caller's data into a
single malloc. Our internal_dispose detects this (by seeing that
fReleaseProcContext == this) and will cleanup properly."

[bungeman-skia, 2013-06-12 23:02:18]

While patch set 2 is rather clever, patch set 3 does more of what we want to be
able to do. SkData on x64 is already 48 bytes and this change the placement
version in 64. This is great for flexibility, but most of the time a great deal
of those bytes are just NULL and ignored. I'll see if there is any way to get
around this, so that we can create SkData of the size we want (everything other
than fData and fSize are just book-keeping that can be fudged about with.

[bungeman-skia, 2013-06-13 19:51:15]

The size of the SkData varies depending on what it needs.

[bungeman-skia, 2013-06-21 16:21:20]

With Patch Set 6 the SkData::NewWithCopy does one allocation. The advantage in
Patch Set 6 is that this can be done generically, so that outside users (like
SkStream and others) can also safely place SkData. While doing this, I realized
that often we're wasting a lot of pointers which are always NULL, so now SkData
are often SkManagedData which are only the size needed.

[reed1, 2013-06-21 19:00:36]

seems like we're going to create a bunch of new template-driving code. Not sure
why this is clearer/better than just function-ptrs.

https://codereview.chromium.org/15675025/diff/24002/include/core/SkData.h
File include/core/SkData.h (right):

https://codereview.chromium.org/15675025/diff/24002/include/core/SkData.h#new...
include/core/SkData.h:126: *  Such an SkData is just a pointer and length.
Not sure what this 2nd sentence means.

"The caller must ensure that the data pointer remains valid for the lifetime of
the returned SkData object. Since SkData is reference countable, this can be
hard to know in the general case. One safe example would be if the pointer was
to global read-only data."

https://codereview.chromium.org/15675025/diff/24002/include/core/SkData.h#new...
include/core/SkData.h:128: static SkData* NewUnowned(const void* data, size_t
length);
NewUnmanaged() ?
maybe unowned is better.

I presume the reason this is public at all is for SkStream?

https://codereview.chromium.org/15675025/diff/24002/include/core/SkWeakRefCnt.h
File include/core/SkWeakRefCnt.h (right):

https://codereview.chromium.org/15675025/diff/24002/include/core/SkWeakRefCnt...
include/core/SkWeakRefCnt.h:77: this->INHERITED::validate();
please please please consider landing this and related changes independent of
this SkData effort. We keep iterating on SkData, but these sorts of changes are
fine, and unrelated (afaik).

[bungeman-skia, 2014-09-18 19:10:27]

Closing this now that https://codereview.chromium.org/560653004 has landed.