Add a class to allocate small objects w/o extra calls to new.

Add a class to allocate small objects w/o extra calls to new.

Add SkSmallAllocator, a template for allocating small (as defined by the
instantiation) objects without extra calls to new. Add a helper macro to
make using it simple.

Remove SkTemplatesPriv.h, whose behavior is replaced by SkSmallAllocator.
The old SK_PLACEMENT_NEW had the following drawbacks:
- Easily confused with SkNEW_PLACEMENT.
- Requires passing around lots of void*s along with the storageSize.
- Requires using a separate class for deleting it.
- We had multiple ways Auto objects for deleting in different places.
- It always did a straight heap allocation on Windows, meaning Windows
  did not get any advantages from the confusing code.
The new SkSmallAllocator simplifies things:
- It is clear about what it does.
- It takes care of the deletion in one place that is automatically
  handled.

Further, the new class can be used to create more than one object. This
is in preparation for BUG=skia:1976
, for which we would like to create
a new object without extra heap allocations. The plan is to create both
the blitter and the new object on the stack using the SkSmallAllocator.

Add a new test for SkSmallAllocator.

SkShader.h:
Move the private version of CreateBitmapShader to SkBitmapProcShader
(which already has the implementation) and remove the friend class
(which was only used to call this private function). This allows
SkSmallAllocator to reside in the private src/ directory.

SkBitmapProcShader:
Move CreateBitmapShader and the macro for the storage size here. With
the macro in a (private) header, the (private) headers with function
declarations (which now depend on the storage size used) can see the
macro.
Use SkSmallAllocator in CreateBitmapShader.
Change the macro to kBlitterStorageByteCount, since SkSmallAllocator
takes a byte count as its template parameter.

SkBlitter:
Use the SkSmallAllocator.
Remove Sk3DShader::fKillProc and SkAutoCallProc. Both of their
behaviors have been moved into SkSmallAllocator (SkAutoCallProc was
unnecessary anyway, because the only time we ever used it we also
called detach(), so its auto behavior never happened).
Create the Sk3DShader on the stack, if there's room.
Remove the helper version of Choose, which was unused.

SmallAllocatorTest:
Test for the new class.

The rest:
Use SkSmallAllocator.

BUG=skia:1976
Committed: http://code.google.com/p/skia/source/detail?r=13696

[tfarina, 2014-02-27 17:33:15]

https://codereview.chromium.org/179343005/diff/40001/tests/StackAllocatorTest...
File tests/StackAllocatorTest.cpp (right):

https://codereview.chromium.org/179343005/diff/40001/tests/StackAllocatorTest...
tests/StackAllocatorTest.cpp:8: #include "Test.h"
could you keep this sorted?

[bungeman-skia, 2014-02-27 18:26:01]

https://codereview.chromium.org/179343005/diff/40001/src/core/SkStackAllocator.h
File src/core/SkStackAllocator.h (right):

https://codereview.chromium.org/179343005/diff/40001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:81: *  this function so we can create the object as
well?)
With perfect forwarding in c++11 you can. With c++03, sadly no (at least not in
general).

[reed1, 2014-02-27 18:48:12]

https://codereview.chromium.org/179343005/diff/40001/src/core/SkBlitter.cpp
File src/core/SkBlitter.cpp (right):

https://codereview.chromium.org/179343005/diff/40001/src/core/SkBlitter.cpp#n...
src/core/SkBlitter.cpp:815: SkStackAllocator<kBlitterStorageByteCount>*
allocator,
typedef SkStackAllocator<kBlitterStorageByteCount> SkTBlitterAllocator;  ?

[mtklein, 2014-02-27 18:51:02]

https://codereview.chromium.org/179343005/diff/40001/src/core/SkStackAllocator.h
File src/core/SkStackAllocator.h (right):

https://codereview.chromium.org/179343005/diff/40001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:81: *  this function so we can create the object as
well?)
On 2014/02/27 18:26:01, bungeman1 wrote:
> With perfect forwarding in c++11 you can. With c++03, sadly no (at least not
in
> general).

A good compromise is to make a few versions:

template <typename T> T* createT();
template <typename T, typename A1> T* createT(A1);
template <typename T, typename A1, typename A2> T* createT(A1, A2);
// Add more as needed;

Template methods only need to be syntactically valid until they're called.  This
means whichever one of these has the right arity will work happily and the
others will fail to compile if someone tries to call it.

[mtklein, 2014-02-27 23:40:54]

https://codereview.chromium.org/179343005/diff/40001/src/core/SkStackAllocator.h
File src/core/SkStackAllocator.h (right):

https://codereview.chromium.org/179343005/diff/40001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:49: *  Template class for allocating small (as
defined by kSize) objects on the
This sort of reads like we'll be able to allocate kSize objects, or objects of
size kSize.  It's any number of objects totalling less that kSize bytes, right?

https://codereview.chromium.org/179343005/diff/40001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:54: template<size_t kSize> class SkStackAllocator :
public SkNoncopyable {
It looked like we're generally using this to allocate a single object of one of
several types.  Is that always true?  I'm curious if enforcing that would make
this smaller.

I've seen code that works a bit like this:

{
  OneOf<int, double, SomeClass> x;
  SomeClass* ptr = x.set<SomeClass>(...);  // SomeClass() called.
  SkASSERT(x.is<SomeClass>());
  // ~SomeClass() called here.
}

I think sizeof(OneOf<a, b, c, ...>) could be as small as the maximum of its
types plus a byte to remember which type is the one set (we wouldn't need to
store pointers to the destructor, and we'd never overflow).

https://codereview.chromium.org/179343005/diff/40001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:105: void* fObj;
If you really want to keep things small, we might consider using a tagged
pointer here instead of two.  There are always at least two unused bits at the
bottom of a pointer returned from malloc (due to alignment).

struct Pair {
  uintptr_t fData;
  void (*fKillproc)(void*);

  void setPtr(void* ptr, bool onHeap) {
    fData = uintptr_t(ptr) | uintptr_t(onHeap);
  }

  void* getPtr() {
    return (void*)(fData & (UINTPTR_MAX - 1));
  }

  bool onHeap() {
    return fData & 1 == 1;
  }
}

https://codereview.chromium.org/179343005/diff/40001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:115: uint32_t            fStorage[SkAlign4(kSize) >>
2];
Consider storing everything in fStorage?  Does it not defeat the purpose
somewhat to use SkTDArray to store the metadata?  Can we make something like
this work?

[void* deleter] [4-byte N] [N-byte data ...] [void* deleter] [4-byte N] [N-byte
data ...]

https://codereview.chromium.org/179343005/diff/40001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:117: // This class only makes sense on the stack.
This doesn't strictly have to be on the stack right?  This could be useful as a
class member, something created on the heap, or even as (shudder) a global.

Nothing in here is actually stack-specific, so maybe we should just rename it
SkSmallAllocator?

[scroggo, 2014-03-05 20:25:51]

https://codereview.chromium.org/179343005/diff/40001/src/core/SkBlitter.cpp
File src/core/SkBlitter.cpp (right):

https://codereview.chromium.org/179343005/diff/40001/src/core/SkBlitter.cpp#n...
src/core/SkBlitter.cpp:815: SkStackAllocator<kBlitterStorageByteCount>*
allocator,
On 2014/02/27 18:48:12, reed1 wrote:
> typedef SkStackAllocator<kBlitterStorageByteCount> SkTBlitterAllocator;  ?

Done.

https://codereview.chromium.org/179343005/diff/40001/src/core/SkStackAllocator.h
File src/core/SkStackAllocator.h (right):

https://codereview.chromium.org/179343005/diff/40001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:49: *  Template class for allocating small (as
defined by kSize) objects on the
On 2014/02/27 23:40:54, mtklein wrote:
> This sort of reads like we'll be able to allocate kSize objects, or objects of
> size kSize.  It's any number of objects totalling less that kSize bytes,
right?

Correct. I've updated the comment. Sound better?

https://codereview.chromium.org/179343005/diff/40001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:54: template<size_t kSize> class SkStackAllocator :
public SkNoncopyable {
On 2014/02/27 23:40:54, mtklein wrote:
> It looked like we're generally using this to allocate a single object of one
of
> several types.  Is that always true? 

Actually, no. The Sk3DBlitter will be allocated by this object when it has
already allocated one object. Also, the motivation behind this change is so that
I can also allocate another object (the Context object which will be returned by
SkShader::setContext, in a future CL - this change is large enough that I think
it warrants a separate CL).

> I'm curious if enforcing that would make
> this smaller.
> 
> I've seen code that works a bit like this:
> 
> {
>   OneOf<int, double, SomeClass> x;
>   SomeClass* ptr = x.set<SomeClass>(...);  // SomeClass() called.
>   SkASSERT(x.is<SomeClass>());
>   // ~SomeClass() called here.
> }
> 
> I think sizeof(OneOf<a, b, c, ...>) could be as small as the maximum of its
> types plus a byte to remember which type is the one set (we wouldn't need to
> store pointers to the destructor, and we'd never overflow).

https://codereview.chromium.org/179343005/diff/40001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:81: *  this function so we can create the object as
well?)
On 2014/02/27 18:51:02, mtklein wrote:
> On 2014/02/27 18:26:01, bungeman1 wrote:
> > With perfect forwarding in c++11 you can. With c++03, sadly no (at least not
> in
> > general).
> 
> A good compromise is to make a few versions:
> 
> template <typename T> T* createT();
> template <typename T, typename A1> T* createT(A1);
> template <typename T, typename A1, typename A2> T* createT(A1, A2);
> // Add more as needed;
> 
> Template methods only need to be syntactically valid until they're called. 
This
> means whichever one of these has the right arity will work happily and the
> others will fail to compile if someone tries to call it.

Done.

https://codereview.chromium.org/179343005/diff/40001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:105: void* fObj;
On 2014/02/27 23:40:54, mtklein wrote:
> If you really want to keep things small, we might consider using a tagged
> pointer here instead of two.  There are always at least two unused bits at the
> bottom of a pointer returned from malloc (due to alignment).
> 
> struct Pair {
>   uintptr_t fData;
>   void (*fKillproc)(void*);
> 
>   void setPtr(void* ptr, bool onHeap) {
>     fData = uintptr_t(ptr) | uintptr_t(onHeap);
>   }
> 
>   void* getPtr() {
>     return (void*)(fData & (UINTPTR_MAX - 1));
>   }
> 
>   bool onHeap() {
>     return fData & 1 == 1;
>   }
> }

Interesting idea! Are we guaranteed the bottom bit is unused for fStorage as
well? We'll generally only have a few Triples/Pairs (I've renamed it to Rec,
mimicking other classes). Think it's worth it to save a few pointers?

https://codereview.chromium.org/179343005/diff/40001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:115: uint32_t            fStorage[SkAlign4(kSize) >>
2];
On 2014/02/27 23:40:54, mtklein wrote:
> Consider storing everything in fStorage?  Does it not defeat the purpose
> somewhat to use SkTDArray to store the metadata?  Can we make something like
> this work?
> 
> [void* deleter] [4-byte N] [N-byte data ...] [void* deleter] [4-byte N]
[N-byte
> data ...]

That's a very good point. I've removed the SkTDArray to remove its calls to
malloc.

An important consequence of this change is that we now have a limited number of
objects that can be created before we exhaust fStorage.

I can think of a few ways to handle the case of exhausting fStorage:
1) assert or return NULL
2) keep a linked list for more objects
3) make this class templated in both the max number of objects needed and
storage size needed

The downside to (1) is that SkSmallAllocator will suddenly stop working. On the
other hand, if the client is us, and we know what we're doing, maybe that's okay
since we'll always use enough.

With (2), SkSmallAllocator will continue working, but will do more allocations
and have tricky code for a case we probably don't intend. It also means that we
may end up following this path when we really intended this to only create a
couple of objects.

With (3), the client is in charge and should know what to expect.

For my use case, I'll only be using the SkSmallAllocator to create a few
objects. I've gone with option (3), but I could be convinced to take a different
approach. Thoughts?

https://codereview.chromium.org/179343005/diff/40001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:117: // This class only makes sense on the stack.
On 2014/02/27 23:40:54, mtklein wrote:
> This doesn't strictly have to be on the stack right?  This could be useful as
a
> class member, something created on the heap, or even as (shudder) a global.
> 
> Nothing in here is actually stack-specific, so maybe we should just rename it
> SkSmallAllocator?

Agreed. Renamed SkSmallAllocator.

And I've removed the macro (which wouldn't work since it's a template anyway).

https://codereview.chromium.org/179343005/diff/40001/tests/StackAllocatorTest...
File tests/StackAllocatorTest.cpp (right):

https://codereview.chromium.org/179343005/diff/40001/tests/StackAllocatorTest...
tests/StackAllocatorTest.cpp:8: #include "Test.h"
On 2014/02/27 17:33:15, tfarina wrote:
> could you keep this sorted?

Done.

[mtklein, 2014-03-05 20:59:56]

lgtm with a couple new questions

https://codereview.chromium.org/179343005/diff/80001/src/core/SkBitmapProcSha...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/179343005/diff/80001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:308: SkShader::TileMode tmy,
SkTBlitterAllocator* allocator) {
Just curious: does anyone pass a NULL allocator?

https://codereview.chromium.org/179343005/diff/80001/src/core/SkBlitter.cpp
File src/core/SkBlitter.cpp (right):

https://codereview.chromium.org/179343005/diff/80001/src/core/SkBlitter.cpp#n...
src/core/SkBlitter.cpp:947: blitter =
allocator->createT<Sk3DBlitter>(innerBlitter, shader3D);
Ah, this is why we need 2?

https://codereview.chromium.org/179343005/diff/80001/src/core/SkStackAllocator.h
File src/core/SkStackAllocator.h (right):

https://codereview.chromium.org/179343005/diff/80001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:8: // FIXME: Move to SkSmallAllocator.h
:)  I have found writing a git-fr (find-replace) script helps for this sort of
thing:

/m/s/skia (clean) $ cat ~/bin/git-fr
#!/bin/sh
git grep -l "$1" | xargs perl -p -i -e "s/$1/$2/g"

https://codereview.chromium.org/179343005/diff/80001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:26: *  kBytes is the number of bytes provided for
storage. If an object is larger
It's kBytes total?  Or kBytes per object?  E.g. if I use <2, 12>, then can I
create up to 2 objects each up to 12 bytes, or up to two objects up to 12 bytes
in total?  Trying to think about what
  typedef SkSmallAllocator<2, sizeof(SkBitmapProcShader)> SkTBlitterAllocator;
means.  I read it and its comment together as "you can allocate 0,1 or 2 objects
each <= sizeof(SkBitmapProcShader)".

https://codereview.chromium.org/179343005/diff/80001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:27: *  than the storage (minus storage already
used), it will be allocated on the
Can we get away with a hard failure like for count instead of falling back on
heap?  I'd go as far as return NULL and assert in debug for both types of
overrun.  It'd be nice to know that if you're using this it just never hits
heap.

https://codereview.chromium.org/179343005/diff/80001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:60: template<typename T> T* createT() {
Might want to break the line after > for all of them for consistency?

https://codereview.chromium.org/179343005/diff/80001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:121: rec->fObj = static_cast<void*>(fStorage +
(fStorageUsed >> 2));
I'm developing a preference for / 4 unless the argument is really meant to be
interpreted as bits, like an 8888 or 565 pixel.  Here we really semantically
mean "divide by 4" not "shift those bits over by two".

https://codereview.chromium.org/179343005/diff/80001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:131: void* fObj;
Now that I'm thinking about it, it's probably not worth using tagged pointers
here for these guys.  We're not making zillions of these at a time; we're making
1 or 2.  The space used by the extra pointer isn't a big deal, especially
compared to the complexity of using tagged pointers.

[scroggo, 2014-03-05 23:23:26]

https://codereview.chromium.org/179343005/diff/80001/src/core/SkBitmapProcSha...
File src/core/SkBitmapProcShader.cpp (right):

https://codereview.chromium.org/179343005/diff/80001/src/core/SkBitmapProcSha...
src/core/SkBitmapProcShader.cpp:308: SkShader::TileMode tmy,
SkTBlitterAllocator* allocator) {
On 2014/03/05 20:59:56, mtklein wrote:
> Just curious: does anyone pass a NULL allocator?

Yes. SkShader::CreateBitmapShader (in SkShader.cpp).

https://codereview.chromium.org/179343005/diff/80001/src/core/SkBlitter.cpp
File src/core/SkBlitter.cpp (right):

https://codereview.chromium.org/179343005/diff/80001/src/core/SkBlitter.cpp#n...
src/core/SkBlitter.cpp:947: blitter =
allocator->createT<Sk3DBlitter>(innerBlitter, shader3D);
On 2014/03/05 20:59:56, mtklein wrote:
> Ah, this is why we need 2?

For the moment, yes. In a future CL, I'll be bumping this up to 3.

https://codereview.chromium.org/179343005/diff/80001/src/core/SkStackAllocator.h
File src/core/SkStackAllocator.h (right):

https://codereview.chromium.org/179343005/diff/80001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:26: *  kBytes is the number of bytes provided for
storage. If an object is larger
On 2014/03/05 20:59:56, mtklein wrote:
> It's kBytes total?  Or kBytes per object?  E.g. if I use <2, 12>, then can I
> create up to 2 objects each up to 12 bytes, or up to two objects up to 12
bytes
> in total?  Trying to think about what
>   typedef SkSmallAllocator<2, sizeof(SkBitmapProcShader)> SkTBlitterAllocator;
> means.  I read it and its comment together as "you can allocate 0,1 or 2
objects
> each <= sizeof(SkBitmapProcShader)".

I thought this was pretty clear, but I've changed kBytes to kTotalBytes and
modified the comment a bit. The comment in SkBitmapProcShader.h is definitely
confusing. I've updated that one as well.

https://codereview.chromium.org/179343005/diff/80001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:27: *  than the storage (minus storage already
used), it will be allocated on the
On 2014/03/05 20:59:56, mtklein wrote:
> Can we get away with a hard failure like for count instead of falling back on
> heap?  I'd go as far as return NULL and assert in debug for both types of
> overrun.  It'd be nice to know that if you're using this it just never hits
> heap.

That's a good idea. Added an assert for too many objects (and modified the test
accordingly). One concern about making the storage size a hard limit though: How
good do we think our testing coverage is? I'm not positive I know our largest
blitter. The old code falls back to the heap if the blitter is larger than
SkBitmapProcShader. If we ever had to fall back we'll have to find whatever's
bigger. (reed@, do you remember why we used sizeof(SkBitmapProcShader)? Any idea
whether we ever exceeded that?) Might there be a blitter that we don't test (or
combo blitter plus 3DBlitter) that would fail but we wouldn't catch until it
happens on a particular website? (Also, once I use this for setContext as well,
we'll have to consider a new object for each shader. And SkComposeShader allows
devious objects, as you could compose two SkComposeShaders which each are
composed of two SkComposeShaders. For those we may have to give up and use the
heap.)

https://codereview.chromium.org/179343005/diff/80001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:60: template<typename T> T* createT() {
On 2014/03/05 20:59:56, mtklein wrote:
> Might want to break the line after > for all of them for consistency?

Done.

https://codereview.chromium.org/179343005/diff/80001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:121: rec->fObj = static_cast<void*>(fStorage +
(fStorageUsed >> 2));
On 2014/03/05 20:59:56, mtklein wrote:
> I'm developing a preference for / 4 unless the argument is really meant to be
> interpreted as bits, like an 8888 or 565 pixel.  Here we really semantically
> mean "divide by 4" not "shift those bits over by two".

Done.

[mtklein, 2014-03-06 15:04:07]

https://codereview.chromium.org/179343005/diff/80001/src/core/SkStackAllocator.h
File src/core/SkStackAllocator.h (right):

https://codereview.chromium.org/179343005/diff/80001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:26: *  kBytes is the number of bytes provided for
storage. If an object is larger
On 2014/03/05 23:23:26, scroggo wrote:
> On 2014/03/05 20:59:56, mtklein wrote:
> > It's kBytes total?  Or kBytes per object?  E.g. if I use <2, 12>, then can I
> > create up to 2 objects each up to 12 bytes, or up to two objects up to 12
> bytes
> > in total?  Trying to think about what
> >   typedef SkSmallAllocator<2, sizeof(SkBitmapProcShader)>
SkTBlitterAllocator;
> > means.  I read it and its comment together as "you can allocate 0,1 or 2
> objects
> > each <= sizeof(SkBitmapProcShader)".
> 
> I thought this was pretty clear, but I've changed kBytes to kTotalBytes and
> modified the comment a bit. The comment in SkBitmapProcShader.h is definitely
> confusing. I've updated that one as well.

Thanks!  I am no longer confused.  I see now this comment was clear and correct,
but the one in the other file was confusing.

https://codereview.chromium.org/179343005/diff/80001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:27: *  than the storage (minus storage already
used), it will be allocated on the
On 2014/03/05 23:23:26, scroggo wrote:
> On 2014/03/05 20:59:56, mtklein wrote:
> > Can we get away with a hard failure like for count instead of falling back
on
> > heap?  I'd go as far as return NULL and assert in debug for both types of
> > overrun.  It'd be nice to know that if you're using this it just never hits
> > heap.
> 
> That's a good idea. Added an assert for too many objects (and modified the
test
> accordingly). One concern about making the storage size a hard limit though:
How
> good do we think our testing coverage is? I'm not positive I know our largest
> blitter. The old code falls back to the heap if the blitter is larger than
> SkBitmapProcShader. If we ever had to fall back we'll have to find whatever's
> bigger. (reed@, do you remember why we used sizeof(SkBitmapProcShader)? Any
idea
> whether we ever exceeded that?) Might there be a blitter that we don't test
(or
> combo blitter plus 3DBlitter) that would fail but we wouldn't catch until it
> happens on a particular website? (Also, once I use this for setContext as
well,
> we'll have to consider a new object for each shader. And SkComposeShader
allows
> devious objects, as you could compose two SkComposeShaders which each are
> composed of two SkComposeShaders. For those we may have to give up and use the
> heap.)

Hmm, that's a good point.  It'd be easy to see a heap fallback happen if we've
got a test that triggers it, but it's not easy to prove that no possible input
could trigger that path.  Perhaps we could SkASSERT that heap fallback does not
happen but handle it correctly anyway, and add a comment explaining we'd like it
not to happen but aren't sure if it's happening in the Chromium wild.  (Then
guard our unittest for it with #if !SK_DEBUG.)

Won't the devious SkComposeShaders overrun the hard count limit?

[scroggo, 2014-03-06 18:07:04]

ptal

https://codereview.chromium.org/179343005/diff/80001/src/core/SkStackAllocator.h
File src/core/SkStackAllocator.h (right):

https://codereview.chromium.org/179343005/diff/80001/src/core/SkStackAllocato...
src/core/SkStackAllocator.h:27: *  than the storage (minus storage already
used), it will be allocated on the
On 2014/03/06 15:04:07, mtklein wrote:
> On 2014/03/05 23:23:26, scroggo wrote:
> > On 2014/03/05 20:59:56, mtklein wrote:
> > > Can we get away with a hard failure like for count instead of falling back
> on
> > > heap?  I'd go as far as return NULL and assert in debug for both types of
> > > overrun.  It'd be nice to know that if you're using this it just never
hits
> > > heap.
> > 
> > That's a good idea. Added an assert for too many objects (and modified the
> test
> > accordingly). One concern about making the storage size a hard limit though:
> How
> > good do we think our testing coverage is? I'm not positive I know our
largest
> > blitter. The old code falls back to the heap if the blitter is larger than
> > SkBitmapProcShader. If we ever had to fall back we'll have to find
whatever's
> > bigger. (reed@, do you remember why we used sizeof(SkBitmapProcShader)? Any
> idea
> > whether we ever exceeded that?) Might there be a blitter that we don't test
> (or
> > combo blitter plus 3DBlitter) that would fail but we wouldn't catch until it
> > happens on a particular website? (Also, once I use this for setContext as
> well,
> > we'll have to consider a new object for each shader. And SkComposeShader
> allows
> > devious objects, as you could compose two SkComposeShaders which each are
> > composed of two SkComposeShaders. For those we may have to give up and use
the
> > heap.)
> 
> Hmm, that's a good point.  It'd be easy to see a heap fallback happen if we've
> got a test that triggers it, but it's not easy to prove that no possible input
> could trigger that path.  Perhaps we could SkASSERT that heap fallback does
not
> happen but handle it correctly anyway, and add a comment explaining we'd like
it
> not to happen but aren't sure if it's happening in the Chromium wild.  (Then
> guard our unittest for it with #if !SK_DEBUG.)

That sounds like a good compromise. Done. I didn't hit the assert using our SKPs
in render pictures or in gm, tests (after modifying my test) or dm.

> 
> Won't the devious SkComposeShaders overrun the hard count limit?

That may depend on how it's implemented. The plan is to ask the shader up front
how much space will be needed for the context, and count that as one object
(from the SkSmallAllocator's perspective). See
https://codereview.chromium.org/160103002/ if you're curious.

[mtklein, 2014-03-06 18:34:25]

> > Won't the devious SkComposeShaders overrun the hard count limit?
> 
> That may depend on how it's implemented. The plan is to ask the shader up
front
> how much space will be needed for the context, and count that as one object
> (from the SkSmallAllocator's perspective). See
> https://codereview.chromium.org/160103002/ if you're curious.
Exciting!  Dominik and I are working on a new way to record paints that leaves
the mutability of shaders (particularly SkBitmapProcShaders) as the last big
bottleneck.

This CL lgtm.

[scroggo, 2014-03-06 18:48:18]

https://codereview.chromium.org/179343005/diff/170001/src/core/SkSmallAllocat...
File src/core/SkSmallAllocator.h (right):

https://codereview.chromium.org/179343005/diff/170001/src/core/SkSmallAllocat...
src/core/SkSmallAllocator.h:8: #ifndef SkSmallAllocator_DEFINED
Reitveld isn't smart enough to notice this but this file is unchanged from last
patch's SkStackAllocator.h, except without the FIXME.

[reed1, 2014-03-06 19:17:56]

public API lgtm

[scroggo, 2014-03-06 19:59:09]

The CQ bit was checked by scroggo@google.com

[commit-bot: I haz the power, 2014-03-06 19:59:15]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/scroggo@google.com/179343005/170001

[commit-bot: I haz the power, 2014-03-07 03:24:47]

Change committed as 13696