Bug 68105 – Reduce EventTarget memory usage by deferring hash map allocation until there are listeners for more than 1 event type.

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

[Andreas Kling]

As discussed on bug 67133, we can significantly decrease the memory usage of EventTarget by having a special path for when there is only a single event type with listeners on the target.
With the test page from the same bug, memory allocated below EventTarget::addEventListener() goes from 700kB to 47kB.

[Andreas Kling]

Created attachment 107378 [details]
Proposed patch

First stab at this. While the memory savings are very good, some things are no longer inline which may be an issue.
Looking forward to comments.

[Andreas Kling]

Created attachment 107380 [details]
Proposed patch

With ChangeLog this time..

[Darin Adler]

Comment on attachment 107380 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107380&action=review

Seems OK as is, but I would prefer use of an OwnPtr to manual deletion here. I have some other thoughts too.

> Source/WebCore/dom/EventListenerMap.cpp:77
> +        m_fastEventListener = pair<AtomicString, EventListenerVector*>(AtomicString(), 0);

I think you can write this just as:

    pair<AtomicString, EventListenerVector*>()

Without the AtomicString(), 0 part.

> Source/WebCore/dom/EventListenerMap.h:58
> +    EventListenerVector* find(const AtomicString& eventType);

I suppose we couldn’t support this operation if we used just a RegisteredEventListener for a single event handler, but I still think it’s worth exploring if we can do that.

> Source/WebCore/dom/EventListenerMap.h:75
> +    pair<AtomicString, EventListenerVector*> m_fastEventListener;

I think I would have named this with the word “single” rather than “fast”. Or something like that.

I also probably would have used two separate data members rather than a pair.

This should be OwnPtr<EventListenerVector> rather than EventListenerVector*.

If the common case is just a single event listener, then we instead could just have a RegisteredEventListener here rather than an OwnPtr<EventListenerVector>. That would save another memory block for each. I don’t know what data you have about what is the common case.

> Source/WebCore/dom/EventListenerMap.h:78
> +class EventListenerIterator {

Should this class be noncopyable?

> Source/WebCore/dom/EventListenerMap.h:82
> +    // EventTarget must not be modified while an iterator is active.

Could we implement debugging machinery to catch this mistake? One way to do that is to keep a set of EventListenerIterator in the EventListenerMap in debug builds and then invalidate them when the map is changed. HashMap does this for hash map iterators so we could use code similar to what it does.

> Source/WebCore/svg/SVGUseElement.cpp:951
> +    if (SVGElement* shadowTreeElement = target->shadowTreeElement())
> +        if (EventTargetData* d = originalElement->eventTargetData())
> +            d->eventListenerMap.copyEventListenersNotCreatedFromMarkupToTarget(shadowTreeElement);

Missing brace here around the multi line inner if statement. I also suggest the name "data" rather than "d".

[Andreas Kling]

(In reply to comment #3)
> (From update of attachment 107380 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107380&action=review
> 
> Seems OK as is, but I would prefer use of an OwnPtr to manual deletion here.

Great, will land with the pair split into separate AtomicString and OwnPtr<EventListenerVector>.

> > Source/WebCore/dom/EventListenerMap.h:75
> > +    pair<AtomicString, EventListenerVector*> m_fastEventListener;
> 
> I think I would have named this with the word “single” rather than “fast”. Or something like that.

Sure thing.

> If the common case is just a single event listener, then we instead could just have a RegisteredEventListener here rather than an OwnPtr<EventListenerVector>. That would save another memory block for each. I don’t know what data you have about what is the common case.

Sure, I'll collect some more data and look into this for a follow-up patch.

> > Source/WebCore/dom/EventListenerMap.h:78
> > +class EventListenerIterator {
> 
> Should this class be noncopyable?

Seems reasonable. Will tweak.

> > Source/WebCore/dom/EventListenerMap.h:82
> > +    // EventTarget must not be modified while an iterator is active.
> 
> Could we implement debugging machinery to catch this mistake? One way to do that is to keep a set of EventListenerIterator in the EventListenerMap in debug builds and then invalidate them when the map is changed. HashMap does this for hash map iterators so we could use code similar to what it does.

Good idea, I'll do that in a separate patch.

[Andreas Kling]

Created attachment 107730 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 107730 [details]
Patch for landing

Clearing flags on attachment: 107730

Committed r95372: <http://trac.webkit.org/changeset/95372>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jeff Miller]

This broke the Windows build (and likely others as well). Fix landed in <http://trac.webkit.org/changeset/95382>