WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
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
Summary
Reduce EventTarget memory usage by deferring hash map allocation until there ...
Andreas Kling
Reported
2011-09-14 12:37:43 PDT
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.
Attachments
Proposed patch
(34.27 KB, patch)
2011-09-14 12:49 PDT
,
Andreas Kling
no flags
Details
Formatted Diff
Diff
Proposed patch
(37.63 KB, patch)
2011-09-14 12:50 PDT
,
Andreas Kling
no flags
Details
Formatted Diff
Diff
Patch for landing
(38.54 KB, patch)
2011-09-16 15:31 PDT
,
Andreas Kling
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Andreas Kling
Comment 1
2011-09-14 12:49:27 PDT
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
Comment 2
2011-09-14 12:50:26 PDT
Created
attachment 107380
[details]
Proposed patch With ChangeLog this time..
Darin Adler
Comment 3
2011-09-16 11:49:12 PDT
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
Comment 4
2011-09-16 14:46:47 PDT
(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
Comment 5
2011-09-16 15:31:10 PDT
Created
attachment 107730
[details]
Patch for landing
WebKit Review Bot
Comment 6
2011-09-16 23:33:11 PDT
Comment on
attachment 107730
[details]
Patch for landing Clearing flags on attachment: 107730 Committed
r95372
: <
http://trac.webkit.org/changeset/95372
>
WebKit Review Bot
Comment 7
2011-09-16 23:33:16 PDT
All reviewed patches have been landed. Closing bug.
Jeff Miller
Comment 8
2011-09-17 08:58:48 PDT
This broke the Windows build (and likely others as well). Fix landed in <
http://trac.webkit.org/changeset/95382
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug