[webkit-reviews] review granted: [Bug 68105] Reduce EventTarget memory usage by deferring hash map allocation until there are listeners for more than 1 event type. : [Attachment 107380] 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 68105: Reduce EventTarget memory usage by deferring hash map allocation
until there are listeners for more than 1 event type.
https://bugs.webkit.org/show_bug.cgi?id=68105

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

------- Additional Comments from Darin Adler <darin at apple.com>
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(shadowTreeEl
ement);

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