[webkit-reviews] review granted: [Bug 73712] PODIntervalTree takes 1.7MB memory on www.nytimes.com : [Attachment 119495] fix build break in chromium's test code.

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.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 119495: fix build break in chromium's test code.
https://bugs.webkit.org/attachment.cgi?id=119495&action=review

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