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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yongjun Zhang
<yongjun_zhang at apple.com>'s request for 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 Darin Adler <darin at apple.com>
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.