[webkit-reviews] review granted: [Bug 67133] Viewing a post on reddit.com wastes a lot of memory on event listeners. : [Attachment 105496] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andreas Kling <kling at webkit.org>'s
request for review:
Bug 67133: Viewing a post on reddit.com wastes a lot of memory on event
listeners.
https://bugs.webkit.org/show_bug.cgi?id=67133

Attachment 105496: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=105496&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=105496&action=review


My thoughts:

    1) Great to optimize this!

    2) We’d likely save even more memory if we did an optimization in
EventTarget where we store one or two listeners in EventTargetData directly,
use OwnPtr for the map and don’t even allocate a map unless there are more than
two listeners.

    3) Since HashTraits are really traits of the key, not of the hash table as
a whole, it’s a little strange to have a table size trait in there, but not so
strange that I think it’s an unacceptable design. You can see that in the lines
of the code that are making decisions for the table size based on “key traits”,
which smells wrong.

> Source/JavaScriptCore/wtf/HashTable.h:411
> -	   static const int m_minTableSize = 64;
>	   static const int m_maxLoad = 2;
>	   static const int m_minLoad = 6;

Are these load numbers related to the table size? If so, we might consider
moving them to the traits as well.

I think that all these numbers could benefit from “why” comments.

> Source/JavaScriptCore/wtf/HashTraits.h:41
>	   static const bool emptyValueIsZero = false;
>	   static const bool needsDestruction = true;
> +	   static const int minimumTableSize = 64;

It occurs to me that having the static data members defined only here is not
fully conforming C++. There’s supposed to be a definition of each outside the
class definition too. Probably something we can just ignore until we have to
deal with some compiler where it’s actually a problem.

> Source/JavaScriptCore/wtf/HashTraits.h:48
> +	   static const int minimumTableSize = 64;

A little annoying to have this twice. One way of avoiding that would be to have
GenricHashTraitsBase<true, T> derive from GenericHashTraitsBase<false, T>.
There was no need to do that before because there was no member that would be
the same, but an advantage to doing it now would be to not have the “64” number
in two places.