[Webkit-unassigned] [Bug 73712] PODIntervalTree takes 1.7MB memory on www.nytimes.com

Thu Dec 8 17:25:09 PST 2011

https://bugs.webkit.org/show_bug.cgi?id=73712

Kenneth Russell <kbr at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #117731|review+                     |review-
               Flag|                            |

--- Comment #8 from Kenneth Russell <kbr at google.com>  2011-12-08 17:25:09 PST ---
(From update of attachment 117731)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.