Add sk_calloc and sk_calloc_throw to SkMemory_new_handler.cpp.

Add sk_calloc and sk_calloc_throw to SkMemory_new_handler.cpp.

After finding that calloc can be significantly faster than malloc+bzero on some
platforms (notably, Android), Skia wants to start experimenting with using
calloc where possible.  For this, we've added sk_calloc and sk_calloc_throw to
the SkMemory API.

This CL is my ham-handed approach to add calloc support to
SkMemory_new_handler.cpp.  It seems to me that this exists as an alternative to
SkMemory_malloc.cpp so that we can try to dodge any OOM crash that the
underlying malloc implementation might helpfully provide?

I've tried to mimic what I saw here for the non-throwing malloc onto sk_calloc,
and just did some refactoring to make this code look a bit more like the
current SkMemory_malloc.cpp.

BUG=skia:1662
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=227563

[mtklein, 2013-09-24 20:12:19]

Hi guys, I'm in unfamiliar code here so I've picked most likely reviewers via
blame.  I've got the usual questions an unfamiliar coder would ask: are you good
reviewers?  is this a sane change to ask for (we can always fake calloc, but
that'd be sad)?  any suggestions on how best to test this?

[Scott Hess - ex-Googler, 2013-09-24 21:05:53]

I think LGTM from the osx side.

rsesek FYI.

https://codereview.chromium.org/23455061/diff/1/base/process/memory_mac.mm
File base/process/memory_mac.mm (right):

https://codereview.chromium.org/23455061/diff/1/base/process/memory_mac.mm#ne...
base/process/memory_mac.mm:505: ThreadLocalBooleanAutoReset
flag(g_unchecked_malloc.Pointer(), true);
This reads confusing - maybe change g_unchecked_malloc to g_unchecked_alloc
everywhere, since this is really talking to the malloc library?

But I think it is correct.

https://codereview.chromium.org/23455061/diff/1/skia/ext/SkMemory_new_handler...
File skia/ext/SkMemory_new_handler.cpp (right):

https://codereview.chromium.org/23455061/diff/1/skia/ext/SkMemory_new_handler...
skia/ext/SkMemory_new_handler.cpp:62: // unprotected from races with other code
that calls set_new_handler.
+1 for the added comment.

https://codereview.chromium.org/23455061/diff/1/skia/ext/SkMemory_new_handler...
skia/ext/SkMemory_new_handler.cpp:83: void* sk_calloc(size_t size) {
Neither the calloc() interface, nor the sk_malloc_flags/sk_malloc_nothrow
convention.  :-).  But I see that sk_calloc is already in skia, so I guess that
ship has sailed.

https://codereview.chromium.org/23455061/diff/1/skia/ext/SkMemory_new_handler...
skia/ext/SkMemory_new_handler.cpp:97: 
Extra whitespace is extra.

[mtklein, 2013-09-24 21:12:18]

https://codereview.chromium.org/23455061/diff/1/base/process/memory_mac.mm
File base/process/memory_mac.mm (right):

https://codereview.chromium.org/23455061/diff/1/base/process/memory_mac.mm#ne...
base/process/memory_mac.mm:505: ThreadLocalBooleanAutoReset
flag(g_unchecked_malloc.Pointer(), true);
On 2013/09/24 21:05:54, shess wrote:
> This reads confusing - maybe change g_unchecked_malloc to g_unchecked_alloc
> everywhere, since this is really talking to the malloc library?
> 
> But I think it is correct.

You bet.  Was thinking that myself.  Done.

https://codereview.chromium.org/23455061/diff/1/skia/ext/SkMemory_new_handler...
File skia/ext/SkMemory_new_handler.cpp (right):

https://codereview.chromium.org/23455061/diff/1/skia/ext/SkMemory_new_handler...
skia/ext/SkMemory_new_handler.cpp:83: void* sk_calloc(size_t size) {
On 2013/09/24 21:05:54, shess wrote:
> Neither the calloc() interface, nor the sk_malloc_flags/sk_malloc_nothrow
> convention.  :-).  But I see that sk_calloc is already in skia, so I guess
that
> ship has sailed.

Yeah... :'(

https://codereview.chromium.org/23455061/diff/1/skia/ext/SkMemory_new_handler...
skia/ext/SkMemory_new_handler.cpp:97: 
On 2013/09/24 21:05:54, shess wrote:
> Extra whitespace is extra.

Done.

[Scott Hess - ex-Googler, 2013-09-24 21:21:32]

still LGTM.

[Stephen White, 2013-09-24 22:35:02]

LGTM

https://codereview.chromium.org/23455061/diff/11001/skia/ext/SkMemory_new_han...
File skia/ext/SkMemory_new_handler.cpp (right):

https://codereview.chromium.org/23455061/diff/11001/skia/ext/SkMemory_new_han...
skia/ext/SkMemory_new_handler.cpp:17: // new_handler itself will crash on
failure when using tcmalloc).
(Hysterical note: this is actually not tcmalloc-specific: malloc() calling the
new_handler actually came from MSC's malloc implementation; tcmalloc just
inherited it from MSC to make our lives easier when we switched to tcmalloc).

https://codereview.chromium.org/23455061/diff/11001/skia/ext/SkMemory_new_han...
skia/ext/SkMemory_new_handler.cpp:78: void* sk_calloc_throw(size_t size) {
Nit: shouldn't this have the same signature as calloc() itself?

[mtklein, 2013-09-26 14:44:16]

https://codereview.chromium.org/23455061/diff/11001/skia/ext/SkMemory_new_han...
File skia/ext/SkMemory_new_handler.cpp (right):

https://codereview.chromium.org/23455061/diff/11001/skia/ext/SkMemory_new_han...
skia/ext/SkMemory_new_handler.cpp:78: void* sk_calloc_throw(size_t size) {
On 2013/09/24 22:35:02, Stephen White wrote:
> Nit: shouldn't this have the same signature as calloc() itself?

Personally, I'm with you.  But that's not what the header says.

[mtklein, 2013-09-26 14:50:15]

Nico, do you mind taking a look at this OWNERS approval?

[mtklein, 2013-09-26 15:03:56]

*for* OWNERS approval... sigh.


On Thu, Sep 26, 2013 at 10:50 AM, <mtklein@google.com> wrote:

> Nico, do you mind taking a look at this OWNERS approval?
>
>
https://codereview.chromium.**org/23455061/<https://codereview.chromium.org/2...
>

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

[mtklein, 2013-10-02 14:12:21]

Ping?


On Thu, Sep 26, 2013 at 11:03 AM, Mike Klein <mtklein@google.com> wrote:

> *for* OWNERS approval... sigh.
>
>
> On Thu, Sep 26, 2013 at 10:50 AM, <mtklein@google.com> wrote:
>
>> Nico, do you mind taking a look at this OWNERS approval?
>>
>>
https://codereview.chromium.**org/23455061/<https://codereview.chromium.org/2...
>>
>
>

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

[mtklein, 2013-10-04 16:02:16]

+darin

Darin, can you take a look for OWNERS approval?  I don't seem to be able to
pique Nico's interest.

[darin (slow to review), 2013-10-05 06:11:59]

base/ changes LGTM

https://codereview.chromium.org/23455061/diff/11001/skia/ext/SkMemory_new_han...
File skia/ext/SkMemory_new_handler.cpp (right):

https://codereview.chromium.org/23455061/diff/11001/skia/ext/SkMemory_new_han...
skia/ext/SkMemory_new_handler.cpp:21: static inline void* throwOnFailure(size_t
size, void* p) {
style nit: throwOnFailure -> ThrowOnFailure ?

Actually, I'm not sure what style guide applies to this file, so my suggestion
might be off base. The indentation is 4 chars instead of 2. Lines are not
wrapped to 80. Is this following Skia conventions?

[mtklein, 2013-10-07 17:32:36]

https://codereview.chromium.org/23455061/diff/11001/skia/ext/SkMemory_new_han...
File skia/ext/SkMemory_new_handler.cpp (right):

https://codereview.chromium.org/23455061/diff/11001/skia/ext/SkMemory_new_han...
skia/ext/SkMemory_new_handler.cpp:21: static inline void* throwOnFailure(size_t
size, void* p) {
On 2013/10/05 06:12:00, darin wrote:
> style nit: throwOnFailure -> ThrowOnFailure ?
> 
> Actually, I'm not sure what style guide applies to this file, so my suggestion
> might be off base. The indentation is 4 chars instead of 2. Lines are not
> wrapped to 80. Is this following Skia conventions?

Ah, good eye.  The intent was to (continue to) follow Skia style here.  I took a
look and weirdly we were both wrong: Skia wants this as throw_on_failure. 
Uploaded a new version.

[darin (slow to review), 2013-10-08 04:02:46]

OK, LGTM

[commit-bot: I haz the power, 2013-10-08 12:21:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtklein@google.com/23455061/24001

[commit-bot: I haz the power, 2013-10-08 13:51:17]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-10-08 17:13:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtklein@google.com/23455061/24001

[commit-bot: I haz the power, 2013-10-08 19:22:43]

Change committed as 227563