[webkit-reviews] review denied: [Bug 73712] PODIntervalTree takes 1.7MB memory on www.nytimes.com : [Attachment 117731] Fix style issue.

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied  review:
Bug 73712: PODIntervalTree takes 1.7MB memory on www.nytimes.com
https://bugs.webkit.org/show_bug.cgi?id=73712

Attachment 117731: Fix style issue.
https://bugs.webkit.org/attachment.cgi?id=117731&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
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.