Bug 120611 - Cut down on double hashing and code needlessly using hash table iterators
Summary: Cut down on double hashing and code needlessly using hash table iterators
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on: 120620
Blocks:
  Show dependency treegraph
 
Reported: 2013-09-02 16:33 PDT by Darin Adler
Modified: 2013-09-03 15:36 PDT (History)
3 users (show)

See Also:


Attachments
Patch (59.04 KB, patch)
2013-09-02 16:58 PDT, Darin Adler
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2013-09-02 16:33:33 PDT
Cut down on double hashing and code needlessly using hash table iterators
Comment 1 Darin Adler 2013-09-02 16:58:32 PDT
Created attachment 210314 [details]
Patch
Comment 2 Andreas Kling 2013-09-02 17:16:59 PDT
Comment on attachment 210314 [details]
Patch

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

These all look good.

> Source/WebCore/bridge/NP_jsobject.cpp:74
> +        ASSERT(m_map.contains(rootObject));
> +        m_map.remove(rootObject);

It would be cool if we could think of a better pattern that would let us avoid the contains() in debug builds.

> Source/WebCore/inspector/InspectorProfilerAgent.cpp:318
>      if (type == CPUProfileType) {
> -        if (m_profiles.contains(uid))
> -            m_profiles.remove(uid);
> +        m_profiles.remove(uid);
>      } else if (type == HeapProfileType) {
> -        if (m_snapshots.contains(uid))
> -            m_snapshots.remove(uid);
> +        m_snapshots.remove(uid);
>      }

Looks like we don't need curly braces here anymore.
Comment 3 Andreas Kling 2013-09-02 19:44:25 PDT
There's a bug in the DocumentEventQueue change:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x0000000109fe854c WTF::RefPtr<WebCore::EventTarget>::get() const + 12 (RefPtr.h:64)
1   com.apple.WebCore             	0x0000000109fe6d1c WebCore::Event::target() const + 28 (Event.h:99)
2   com.apple.WebCore             	0x000000010a468660 WebCore::DocumentEventQueue::dispatchEvent(WTF::PassRefPtr<WebCore::Event>) + 32 (DocumentEventQueue.cpp:144)
3   com.apple.WebCore             	0x000000010a46860d WebCore::DocumentEventQueue::pendingEventTimerFired() + 525 (DocumentEventQueue.cpp:139)
4   com.apple.WebCore             	0x000000010a46c212 WebCore::DocumentEventQueueTimer::fired() + 98 (DocumentEventQueue.cpp:57)
5   com.apple.WebCore             	0x000000010b86d4b3 WebCore::ThreadTimers::sharedTimerFiredInternal() + 307 (ThreadTimers.cpp:132)
6   com.apple.WebCore             	0x000000010b86d1c9 WebCore::ThreadTimers::sharedTimerFired() + 25 (ThreadTimers.cpp:106)
7   com.apple.WebCore             	0x000000010b606313 WebCore::timerFired(__CFRunLoopTimer*, void*) + 67 (SharedTimerMac.mm:134)
Comment 4 Andreas Kling 2013-09-02 19:47:43 PDT
Comment on attachment 210314 [details]
Patch

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

> Source/WebCore/dom/DocumentEventQueue.cpp:138
> -    while (!m_queuedEvents.isEmpty()) {
> -        ListHashSet<RefPtr<Event>, 16>::iterator iter = m_queuedEvents.begin();
> -        RefPtr<Event> event = *iter;
> -        m_queuedEvents.remove(iter);
> -        if (!event)
> -            break;
> -        dispatchEvent(event.get());
> -    }
> +    while (!m_queuedEvents.isEmpty())
> +        dispatchEvent(m_queuedEvents.takeFirst().get());

Before this code runs, a null Event pointer is inserted in the queue, marking the place to stop.
Comment 5 Darin Adler 2013-09-02 22:51:38 PDT
I’ll land this without any DocumentEventQueue changes.
Comment 6 Darin Adler 2013-09-02 22:53:41 PDT
Comment on attachment 210314 [details]
Patch

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

>> Source/WebCore/dom/DocumentEventQueue.cpp:138
>> +        dispatchEvent(m_queuedEvents.takeFirst().get());
> 
> Before this code runs, a null Event pointer is inserted in the queue, marking the place to stop.

So funny that I missed that. So the while loop is nuts! It says while (!empty) but it’s never going to be empty.
Comment 7 Darin Adler 2013-09-02 22:59:41 PDT
Committed r154967: <http://trac.webkit.org/changeset/154967>
Comment 8 Simon Fraser (smfr) 2013-09-03 15:36:52 PDT
This cause terrible window resize performance: bug 120653.