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

[bugzilla-daemon at webkit.org]

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


Kenneth Russell <kbr at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #119495|review?                     |review+
               Flag|                            |




--- Comment #23 from Kenneth Russell <kbr at google.com>  2011-12-15 20:05:13 PST ---
(From update of attachment 119495)
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.

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