Bug 67133 – Viewing a post on reddit.com wastes a lot of memory on event listeners.

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Andreas Kling]

On a typical reddit.com front page post, WebKit allocates between 1-2 MB of (zeroed) memory in EventTarget::addEventListener(). This is for click event listeners on a *lot* of nodes. Example URL: http://www.reddit.com/r/reddit.com/comments/jxdp5/voltaire_when_asked_on_his_deathbed_to_renounce/

EventTarget is currently storing the event listeners in a HashMap<AtomicString, EventListenerVector*> (the AtomicString is the event name.)

The minimum table size for a HashMap in WebKit is currently 64, which means that as soon as you add an event listener to an EventTarget, we allocate 64 * sizeof(EventListenerVector*).

I did a quick crawl of ~20 popular websites and found that very few pages ever register listeners for more than 1-2 specific events. Twitter and Facebook had the most listeners, around 9-10, peaking at 14 for some nodes.

I believe we could easily reduce the minimum table size for this particular HashMap to 32, and cut memory usage in half. Perhaps we could even go lower.

Patch idea coming..

[Andreas Kling]

Created attachment 105496 [details]
Proposed patch

Lay it on me, gentlemen.

[Darin Adler]

Comment on attachment 105496 [details]
Proposed patch

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.

[Andreas Kling]

(In reply to comment #2)
> (From update of attachment 105496 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105496&action=review
>     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.

I was toying with the same idea (for a follow-up patch.) Will hack something up! :)

>     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.

I certainly see what you mean, it looks a bit awkward as-is. Would you prefer that this "table trait" (along with the min/max "load" values) be promoted to a separate template argument?

> > Source/JavaScriptCore/wtf/HashTable.h:411
> > -        static const int m_minTableSize = 64;
> >          static const int m_maxLoad = 2;
> >          static const int m_minLoad = 6;
> 
> I think that all these numbers could benefit from “why” comments.

I agree that comments would be nice, though I don't know how you decided on these numbers in <http://trac.webkit.org/changeset/9527>.

> > 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.

Will tweak before landing.

[Darin Adler]

(In reply to comment #3)
> Would you prefer that this "table trait" (along with the min/max "load" values) be promoted to a separate template argument?

I think this might make things worse, rather than better. This is something we should discuss with Maciej, since he’s really the architect of these classes and is thoughtful about such things. He’s out for a week, and will be back on Monday.

> > > Source/JavaScriptCore/wtf/HashTable.h:411
> > > -        static const int m_minTableSize = 64;
> > >          static const int m_maxLoad = 2;
> > >          static const int m_minLoad = 6;
> > 
> > I think that all these numbers could benefit from “why” comments.
> 
> I agree that comments would be nice, though I don't know how you decided on these numbers in <http://trac.webkit.org/changeset/9527>.

I agree that it was Maciej who should have added those comments back 6 years ago. (I was the reviewer, not the author.)

The maxLoad of 2 means “never be more than 1/2 full” and minLoad of 6 means “never be less than 1/6 full”. As to where those come from and what best practices are, Maciej can probably remember.

[Andreas Kling]

Committed r93990: <http://trac.webkit.org/changeset/93990>