Bug 73712 – PODIntervalTree takes 1.7MB memory on www.nytimes.com

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Yongjun Zhang]

PODIntervalTree instances take large amount of memory, e.g., in nytime.com, its total heap consumption is 1.7MB; in cnn.com, 2.2MB.

Looks like each RenderBlock containing floating objects owns a PODIntervalTree, which owns an PODArena with at least on 16KB chunk.  Nytime.com has over 100 RenderBlocks with floating objects, hence 1.7MB altogether.  One idea is to have one PODArena per document and having all PODIntervalTrees share it.

[Yongjun Zhang]

In addition, 16KB chunk seems to be over generous.  I tested on Alexa top 100 sites, the largest PODIntervalTree has 25 nodes and they take less than 1KB in the chunk.

[Yongjun Zhang]

<rdar://problem/10507353>

[Yongjun Zhang]

Created attachment 117721 [details]
Use a shared PODArena for all PODIntervalTrees inside a document.

[WebKit Review Bot]

Attachment 117721 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/dom/Document.h:43:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yongjun Zhang]

Created attachment 117731 [details]
Fix style issue.

[Yongjun Zhang]

Darin, Kenneth, would any of you mind review this patch?

[Darin Adler]

Comment on attachment 117731 [details]
Fix style issue.

View in context: https://bugs.webkit.org/attachment.cgi?id=117731&action=review

> Source/WebCore/platform/PODArena.h:110
> +        ChunkVector::const_iterator it = m_chunks.begin();
> +        while (it != end) {
> +            if ((*it)->contains(ptr)) {
> +                (*it)->free(ptr);
> +                break;
> +            }
> +            ++it;
> +        }

We normally use a for loop for something like this rather than a while loop.

> Source/WebCore/platform/PODArena.h:170
> +                ChunkVector::const_iterator it = m_chunks.begin();
> +                while (it < end) {
> +                    if ((*it)->hasFreeList()) {
> +                        ptr = (*it)->allocate(roundedSize);
> +                        break;
> +                    }
> +                    ++it;
> +                }

Same comment about for loop rather than while loop. Also better to use != rather than < for the end check.

If the allocation fails, don’t we want to keep iterating to use the free space in a different chunk? I think it should be if (ptr) break; rather than just break;

> Source/WebCore/platform/PODArena.h:225
> +                void* p = m_freeList;
> +                m_freeList = m_freeList->m_next;
> +                return p;

We normally prefer to use a word rather than a letter for a local variable like “p”.

> Source/WebCore/platform/PODArena.h:244
> +            FreeCell* cell = (FreeCell*)ptr;

This should be static_cast rather than a C-style cast.

> Source/WebCore/platform/PODArena.h:251
> +            return (ptr >= m_base && ptr < m_base + m_size);

No need for the parentheses here.

> Source/WebCore/platform/PODRedBlackTree.h:171
> +    void initIfNeeded(PassRefPtr<PODArena> arena)
> +    {
> +        if (!m_arena)
> +            m_arena = arena;
> +    }

It would be slightly more efficient if this took a raw pointer instead of PassRefPtr because this does not always take ownership of the passed in arena, and when it does not, the PassRefPtr produces unnecessary reference count churn.

[Kenneth Russell]

Comment on attachment 117731 [details]
Fix style issue.

View in context: https://bugs.webkit.org/attachment.cgi?id=117731&action=review

First, nice work tracking down this issue and proposing a fix.

I'm not qualified to review the renderer code; hyatt or darin should.

However, I am concerned about the overall strategy of adding a free list to the PODArena. As indicated above, the currently proposed changes violate invariants, so they will either have to be fixed or a different approach will need to be taken. Now, it's possible to fix the free list code, but a solution that would be more in line with the design of the arena, red/black and interval trees would be to just clear out the arena wholesale at certain points in time. I don't know whether there's a good point in the layout algorithm to do this, but if there is, I would lean toward a solution of that form. That could introduce the possibility of dangling pointers from existing red/black and interval tree instances, and that might be a worse side-effect than the additional complexity of free list maintenance, so the tradeoffs would have to be carefully considered.

I'm marking this patch r- for a couple of style issues as well as the correctness problem with the free list. Will be glad to review future iterations, though again one of the other reviewers will have to give the final r+.

> Source/WebCore/ChangeLog:3
> +        PODInervalTree takes 1.7MB memory on www.nytimes.com.

Typo: PODInervalTree

> Source/WebCore/dom/Document.h:578
> +    PODArena* PODIntervalArena() const { return m_PODIntervalArena.get(); }

Capitalization doesn't match WebKit's style. When the interval tree and arena classes were originally added, there were objections to the use of the "POD" prefix, since it isn't quite accurate, and I still intend at some point to generalize these classes to fix this -- so I would suggest naming this just "intervalArena()" and the member "m_intervalArena".

>> Source/WebCore/platform/PODArena.h:110
>> +        }
> 
> We normally use a for loop for something like this rather than a while loop.

Are you sure it is a good idea to introduce a search into this critical point of the algorithm? Would it be possible to map from a pointer to its containing Chunk in constant time? One way to do this would be to always allocate space for the Chunk backpointer before the start of the object, though this would waste space.

> Source/WebCore/platform/PODArena.h:167
> +                        break;

If you're iterating through the chunks already, why not generalize the code and write "if (ptr) break;" instead?

> Source/WebCore/platform/PODArena.h:197
> +        };

For correctness as this class is currently written, each entry on the free list would have to keep track of its size.

> Source/WebCore/platform/PODArena.h:221
> +            if (m_freeList) {

You'd better ASSERT that size >= sizeof(FreeCell), or later frees of the returned pointer will blow up.

> Source/WebCore/platform/PODArena.h:223
> +                void* p = m_freeList;

This code violates invariants of the class. The PODArena as written has to handle allocations of multiple sizes. You aren't keeping track of the size of the block of memory pointed to by the freed pointer, so you don't know whether each entry on the free list can satisfy the allocation. Now, the current usage of PODArena in the handling of floating objects happens to make this code work, but you can't rely on that. If you go down the route of adding a free list then either the free list needs to be generalized, or the PODArena has to be specialized to only support allocating one data type per arena instance.

>> Source/WebCore/platform/PODArena.h:244
>> +            FreeCell* cell = (FreeCell*)ptr;
> 
> This should be static_cast rather than a C-style cast.

Use a C++-style cast here, either static_cast or reinterpret_cast.

> Source/WebCore/platform/PODArena.h:246
> +            m_freeList = cell;

As mentioned above, this doesn't work in the general case.

[Kenneth Russell]

(darin, sorry to contradict your r+, but I really think the correctness problem should be solved before committing this.)

[Darin Adler]

(In reply to comment #9)
> (darin, sorry to contradict your r+, but I really think the correctness problem should be solved before committing this.)

No need to apologize. Glad you caught it.

[Yongjun Zhang]

(In reply to comment #8)
> (From update of attachment 117731 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117731&action=review
> 
> First, nice work tracking down this issue and proposing a fix.
> 

Thanks for the comments!

> I'm not qualified to review the renderer code; hyatt or darin should.
> 
> However, I am concerned about the overall strategy of adding a free list to the PODArena. As indicated above, the currently proposed changes violate invariants, so they will either have to be fixed or a different approach will need to be taken. Now, it's possible to fix the free list code, but a solution that would be more in line with the design of the arena, red/black and interval trees would be to just clear out the arena wholesale at certain points in time. I don't know whether there's a good point in the layout algorithm to do this, but if there is, I would lean toward a solution of that form. That could introduce the possibility of dangling pointers from existing red/black and interval tree instances, and that might be a worse side-effect than the additional complexity of free list maintenance, so the tradeoffs would have to be carefully considered.

That was also my concern when first looking at the code.  However, it seems like a PODBlackRedTree only allocates fixed size objects from its PODArena, and PODArena is so far only used by PODBlackRedTree.  So this patch makes the assumption that each object allocated from the same arena has the same size, which simplifies the freelist implementation and we don't need to track the size of each cell from a chunk.   I agree it is not future proof if we are to use PODArena with different sized cells.

> 
> I'm marking this patch r- for a couple of style issues as well as the correctness problem with the free list. Will be glad to review future iterations, though again one of the other reviewers will have to give the final r+.
> 

Sure, I will keep working on it.

> >> Source/WebCore/platform/PODArena.h:110
> >> +        }
> > 
> > We normally use a for loop for something like this rather than a while loop.
> 
> Are you sure it is a good idea to introduce a search into this critical point of the algorithm? Would it be possible to map from a pointer to its containing Chunk in constant time? One way to do this would be to always allocate space for the Chunk backpointer before the start of the object, though this would waste space.

From my test, it doesn't look like we will have lots of chunks in real sites.  And we can reduce the search if we set the chunk size big enough (4K seems to be reasonable,).

> 
> > Source/WebCore/platform/PODArena.h:167
> > +                        break;
> 
> If you're iterating through the chunks already, why not generalize the code and write "if (ptr) break;" instead?

Good point!

> 
> > Source/WebCore/platform/PODArena.h:197
> > +        };
> 
> For correctness as this class is currently written, each entry on the free list would have to keep track of its size.
> 

Again, the assumption is each entry in the free list has the same size.

> > Source/WebCore/platform/PODArena.h:221
> > +            if (m_freeList) {
> 
> You'd better ASSERT that size >= sizeof(FreeCell), or later frees of the returned pointer will blow up.
> 
> > Source/WebCore/platform/PODArena.h:223
> > +                void* p = m_freeList;
> 
> This code violates invariants of the class. The PODArena as written has to handle allocations of multiple sizes. You aren't keeping track of the size of the block of memory pointed to by the freed pointer, so you don't know whether each entry on the free list can satisfy the allocation. Now, the current usage of PODArena in the handling of floating objects happens to make this code work, but you can't rely on that. If you go down the route of adding a free list then either the free list needs to be generalized, or the PODArena has to be specialized to only support allocating one data type per arena instance.

I agree.  How about we make a special cased PODArena for the PODIntervalTree that we know for sure each entry will have the same size, and keep the generalize case as it is?  That way we fix the memory issue without violating the invariants of PODArena.

[Kenneth Russell]

(In reply to comment #11)
> (In reply to comment #8)
> > (From update of attachment 117731 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=117731&action=review
> > 
> > First, nice work tracking down this issue and proposing a fix.
> > 
> 
> Thanks for the comments!
> 
> > I'm not qualified to review the renderer code; hyatt or darin should.
> > 
> > However, I am concerned about the overall strategy of adding a free list to the PODArena. As indicated above, the currently proposed changes violate invariants, so they will either have to be fixed or a different approach will need to be taken. Now, it's possible to fix the free list code, but a solution that would be more in line with the design of the arena, red/black and interval trees would be to just clear out the arena wholesale at certain points in time. I don't know whether there's a good point in the layout algorithm to do this, but if there is, I would lean toward a solution of that form. That could introduce the possibility of dangling pointers from existing red/black and interval tree instances, and that might be a worse side-effect than the additional complexity of free list maintenance, so the tradeoffs would have to be carefully considered.
> 
> That was also my concern when first looking at the code.  However, it seems like a PODBlackRedTree only allocates fixed size objects from its PODArena, and PODArena is so far only used by PODBlackRedTree.  So this patch makes the assumption that each object allocated from the same arena has the same size, which simplifies the freelist implementation and we don't need to track the size of each cell from a chunk.   I agree it is not future proof if we are to use PODArena with different sized cells.

Again, that's true for its current usage but not true in the general case. See Source/WebCore/platform/graphics/gpu/LoopBlinnPathProcessor.cpp (which is not in use and which is going away) for an example of more general usage.

> I agree.  How about we make a special cased PODArena for the PODIntervalTree that we know for sure each entry will have the same size, and keep the generalize case as it is?  That way we fix the memory issue without violating the invariants of PODArena.

Yes, you could subclass PODArena into PODFreeListArena<T> which only supports allocations and frees of a particular type. But would you give my suggestion about clearing out the arena some thought?

[Yongjun Zhang]

(In reply to comment #12)
> > I agree.  How about we make a special cased PODArena for the PODIntervalTree that we know for sure each entry will have the same size, and keep the generalize case as it is?  That way we fix the memory issue without violating the invariants of PODArena.
> 
> Yes, you could subclass PODArena into PODFreeListArena<T> which only supports allocations and frees of a particular type. But would you give my suggestion about clearing out the arena some thought?

I was thinking to have Document hold up to a shared PODFreeListArena<T> and the arena will get cleared when the document is destroyed.

[Kenneth Russell]

(In reply to comment #13)
> (In reply to comment #12)
> > > I agree.  How about we make a special cased PODArena for the PODIntervalTree that we know for sure each entry will have the same size, and keep the generalize case as it is?  That way we fix the memory issue without violating the invariants of PODArena.
> > 
> > Yes, you could subclass PODArena into PODFreeListArena<T> which only supports allocations and frees of a particular type. But would you give my suggestion about clearing out the arena some thought?
> 
> I was thinking to have Document hold up to a shared PODFreeListArena<T> and the arena will get cleared when the document is destroyed.

That wasn't my point -- I'm suggesting to not add the free list functionality at all, keep the arena class simple, and at opportune times, clear out the shared arena. The question is whether there is a good point during layout to clear the arena because we know that we are going to clear out and recompute all of the floating object sets.

[Yongjun Zhang]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > > I agree.  How about we make a special cased PODArena for the PODIntervalTree that we know for sure each entry will have the same size, and keep the generalize case as it is?  That way we fix the memory issue without violating the invariants of PODArena.
> > > 
> > > Yes, you could subclass PODArena into PODFreeListArena<T> which only supports allocations and frees of a particular type. But would you give my suggestion about clearing out the arena some thought?
> > 
> > I was thinking to have Document hold up to a shared PODFreeListArena<T> and the arena will get cleared when the document is destroyed.
> 
> That wasn't my point -- I'm suggesting to not add the free list functionality at all, keep the arena class simple, and at opportune times, clear out the shared arena. The question is whether there is a good point during layout to clear the arena because we know that we are going to clear out and recompute all of the floating object sets.

If the shared arena is per document, looks like it is hard to find such a point that we could clear the arena before tearing down the entire render tree.  m_placedFloatsTree is needed for layout involved with its owning renderblock and we can only reliably clear this tree when its owning renderblock is destroyed.  That means we have to wait all PODIntervalTree's get destroyed before we can clear the shared arena, and probably also means we are going to throw away the whole render tree anyway.

[Kenneth Russell]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > (In reply to comment #12)
> > > > > I agree.  How about we make a special cased PODArena for the PODIntervalTree that we know for sure each entry will have the same size, and keep the generalize case as it is?  That way we fix the memory issue without violating the invariants of PODArena.
> > > > 
> > > > Yes, you could subclass PODArena into PODFreeListArena<T> which only supports allocations and frees of a particular type. But would you give my suggestion about clearing out the arena some thought?
> > > 
> > > I was thinking to have Document hold up to a shared PODFreeListArena<T> and the arena will get cleared when the document is destroyed.
> > 
> > That wasn't my point -- I'm suggesting to not add the free list functionality at all, keep the arena class simple, and at opportune times, clear out the shared arena. The question is whether there is a good point during layout to clear the arena because we know that we are going to clear out and recompute all of the floating object sets.
> 
> If the shared arena is per document, looks like it is hard to find such a point that we could clear the arena before tearing down the entire render tree.  m_placedFloatsTree is needed for layout involved with its owning renderblock and we can only reliably clear this tree when its owning renderblock is destroyed.  That means we have to wait all PODIntervalTree's get destroyed before we can clear the shared arena, and probably also means we are going to throw away the whole render tree anyway.

OK, thanks for giving this some thought. In that case let's go with your suggested approach of adding a new PODFreeListArena.

[Yongjun Zhang]

Created attachment 119134 [details]
Create a separate class PODFreeListArena which allocates objects of the same size.

[Kenneth Russell]

Comment on attachment 119134 [details]
Create a separate class PODFreeListArena which allocates objects of the same size.

View in context: https://bugs.webkit.org/attachment.cgi?id=119134&action=review

> Source/WebCore/platform/PODFreeListArena.h:33
> +class PODFreeListArena : public PODArena {

This is a step in the right direction but doesn't address the basic problem in the last review. This class still allows allocation and freeing of different sizes.

My thought was to parameterize PODFreeListArena on the one type of object it knows how to allocate. I would use private inheritance from PODArena. The PODFreeListArena's allocateObject() methods shouldn't take the first "class T" template argument. Change PODRedBlackTree to accept a PODFreeListArena<T> rather than a PODArena.

The challenge will be to figure out how to describe the type parameter. It will be a Node<T> where T is the type parameter of the RedBlackTree. Currently Node is an internal concept to RedBlackTree. Perhaps you can expose it with a public typedef.

The solution I'm outlining will also allow getting rid of the virtual allocate and free functions, which are undesirable.

Another solution could be to configure PODFreeListArena so that it can only allocate objects of a certain size, and sprinkle assertions throughout the code. This is gross though and I would rather find a solution that uses the type system to enforce correct usage.

[Yongjun Zhang]

(In reply to comment #18)
> (From update of attachment 119134 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119134&action=review
> 
> > Source/WebCore/platform/PODFreeListArena.h:33
> > +class PODFreeListArena : public PODArena {
> 
> This is a step in the right direction but doesn't address the basic problem in the last review. This class still allows allocation and freeing of different sizes.
> 
> My thought was to parameterize PODFreeListArena on the one type of object it knows how to allocate. I would use private inheritance from PODArena. The PODFreeListArena's allocateObject() methods shouldn't take the first "class T" template argument. Change PODRedBlackTree to accept a PODFreeListArena<T> rather than a PODArena.
> 

thanks,  this should work!  I was trying to keep the change in PODRedBlackTree small, but I didn't realize a PODRedBlackTree will always ask its arena to allocate objects of the same size.

> The challenge will be to figure out how to describe the type parameter. It will be a Node<T> where T is the type parameter of the RedBlackTree. Currently Node is an internal concept to RedBlackTree. Perhaps you can expose it with a public typedef.
> 

Yeah, it would be a bit awkward to declare the interval arena in Document as something like:

PODFreeListArena<PODRedBlackTree<int, FloatingObject*>::Node>

> The solution I'm outlining will also allow getting rid of the virtual allocate and free functions, which are undesirable.
> 

true, and we can also avoid the overhead of virtual function calls.

> Another solution could be to configure PODFreeListArena so that it can only allocate objects of a certain size, and sprinkle assertions throughout the code. This is gross though and I would rather find a solution that uses the type system to enforce correct usage.

This is essential what the current patch is doing (without assertions though).  Anyway, I will try the approach you suggested and see how it goes.  thanks!

[Yongjun Zhang]

Created attachment 119477 [details]
Take 3: use PODFreeListArena<T> to ensure the objects allocated from the arena having the same size.

In addition to changing PODFreeListArena to a templated class, the patch also moves the shared interval arena from Document to RenderView, since Document doesn't have visibility to PODInteval<int, FloatingObject*> which is defined in RenderBlock.

[WebKit Review Bot]

Comment on attachment 119477 [details]
Take 3: use PODFreeListArena<T> to ensure the objects allocated from the arena having the same size.

Attachment 119477 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10914341

[Yongjun Zhang]

Created attachment 119495 [details]
fix build break in chromium's test code.

[Kenneth Russell]

Comment on attachment 119495 [details]
fix build break in chromium's test code.

View in context: https://bugs.webkit.org/attachment.cgi?id=119495&action=review

This looks great. Thanks for persisting and reaching this solution. A couple of comments which I'd appreciate it if you'd address, but don't want to hold you back from committing. r=me

> Source/WebCore/platform/PODArena.h:115
> +    virtual ~PODArena() { }

virtual is unnecessary here. The user deals explicitly with instances of PODFreeListArena and never downcasts to PODArena.

> Source/WebCore/platform/PODArena.h:175
> +        virtual ~Chunk()

Good call on this though.

> Source/WebCore/platform/PODFreeListArena.h:34
> +class PODFreeListArena : public PODArena {

Can you use private inheritance here (" : private PODArena")? That way you can use the implementation details of PODArena without exposing its interface. Conceptually PODFreeListArena is completely distinct from PODArena. I don't know how that would play with the RefCounted machinery though. If it's at all possible to make this change I would strongly recommend doing it.

> Source/WebCore/platform/PODRedBlackTree.h:178
> +        Node* node = m_arena->template allocateObject<T>(data);

I've never seen this construct before. However, I think that using private inheritance as mentioned above will let you get rid of the "template" here, since then only PODFreeListArena's allocateObject method would be visible.

> Source/WebCore/platform/PODRedBlackTree.h:-238
> -protected:

Do you think it's possible to keep the Node class hidden? For example,
protected:
  class Node { ... };
public:
  typedef Node NodeType;
protected:
  // ...

It's OK as is, but it would be nicer to not have to expose the internal class.

[Yongjun Zhang]

(In reply to comment #23)
> (From update of attachment 119495 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119495&action=review
> 
> This looks great. Thanks for persisting and reaching this solution. A couple of comments which I'd appreciate it if you'd address, but don't want to hold you back from committing. r=me

thank you for the help!

> 
> > Source/WebCore/platform/PODArena.h:115
> > +    virtual ~PODArena() { }
> 
> virtual is unnecessary here. The user deals explicitly with instances of PODFreeListArena and never downcasts to PODArena.
> 
> > Source/WebCore/platform/PODArena.h:175
> > +        virtual ~Chunk()
> 
> Good call on this though.
> 
> > Source/WebCore/platform/PODFreeListArena.h:34
> > +class PODFreeListArena : public PODArena {
> 
> Can you use private inheritance here (" : private PODArena")? That way you can use the implementation details of PODArena without exposing its interface. Conceptually PODFreeListArena is completely distinct from PODArena. I don't know how that would play with the RefCounted machinery though. If it's at all possible to make this change I would strongly recommend doing it.
> 

I tried with private inheritance but it failed compiling because RefCounted needs to be public inheritance.

> > Source/WebCore/platform/PODRedBlackTree.h:178
> > +        Node* node = m_arena->template allocateObject<T>(data);
> 
> I've never seen this construct before. However, I think that using private inheritance as mentioned above will let you get rid of the "template" here, since then only PODFreeListArena's allocateObject method would be visible.

yeah, private inheritance would be ideal but we have RefCounted issue.

> 
> > Source/WebCore/platform/PODRedBlackTree.h:-238
> > -protected:
> 
> Do you think it's possible to keep the Node class hidden? For example,
> protected:
>   class Node { ... };
> public:
>   typedef Node NodeType;
> protected:
>   // ...
> 

that doesn't work... typedef doesn't seem to change the visibility of Node.

> It's OK as is, but it would be nicer to not have to expose the internal class.

[WebKit Review Bot]

Comment on attachment 119495 [details]
fix build break in chromium's test code.

Clearing flags on attachment: 119495

Committed r103030: <http://trac.webkit.org/changeset/103030>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Rowe (bdash)]

Comment on attachment 119495 [details]
fix build break in chromium's test code.

View in context: https://bugs.webkit.org/attachment.cgi?id=119495&action=review

> Source/WebCore/platform/PODFreeListArena.h:105
> +    void* allocate(size_t size)
> +    {
> +        void* ptr = 0;
> +        if (m_current) {
> +            // First allocate from the current chunk.
> +            ptr = m_current->allocate(size);
> +            if (!ptr) {
> +                // Check if we can allocate from other chunks' free list.
> +                ChunkVector::const_iterator end = m_chunks.end();
> +                for (ChunkVector::const_iterator it = m_chunks.begin(); it != end; ++it) {
> +                    FreeListChunk* chunk = static_cast<FreeListChunk*>(it->get());
> +                    if (chunk->hasFreeList()) {
> +                        ptr = chunk->allocate(size);
> +                        if (ptr)
> +                            break;
> +                    }
> +                }
> +            }
> +        }
> +
> +        if (!ptr) {
> +            if (size > m_currentChunkSize)
> +                m_currentChunkSize = size;
> +            m_chunks.append(adoptPtr(new FreeListChunk(m_allocator.get(), m_currentChunkSize)));
> +            m_current = m_chunks.last().get();
> +            ptr = m_current->allocate(size);
> +        }
> +        return ptr;
> +    }

This function would be much, much easier to follow if it used early returns rather than multiple levels of nested if statements.