Bug 50405

Summary: Update ResourceLoaderSet Enumeration
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: WebCore Misc.Assignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, joepeck
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
[PATCH] Switch to copyToVector
none
[PATCH] "size_t" instead of "unsigned" none

Description Joseph Pecoraro 2010-12-02 12:23:19 PST
From: https://bugs.webkit.org/show_bug.cgi?id=49838#c4
Darin Adler said:

> > WebCore/loader/DocumentLoader.cpp:79
> > +    const ResourceLoaderSet copy = loaders;
> 
> We normally don’t use const for local variables like this one.
> 
> Rather than copying the set it would be more efficient to instead
> use a local vector and the copyToVector function.

Here I'm updating the static functions that take the older approach.

I believe it is more efficient because copying a HashSet requires copying
the inner HashTable, which deals with internal HashTable machinery. In
both cases they use an enumerator, but with copyToVector there isn't
that overhead of the HashTable internals.
Comment 1 Joseph Pecoraro 2010-12-02 12:25:14 PST
Created attachment 75403 [details]
[PATCH] Switch to copyToVector
Comment 2 Joseph Pecoraro 2010-12-02 12:33:11 PST
These are other places that might benefit. It looks like they are places where a HashSet would be copied:

    bindings/js/JSClipboardCustom.cpp:55:    HashSet<String> types = clipboard->types();
    bindings/v8/custom/V8ClipboardCustom.cpp:52:    HashSet<String> types = clipboard->types();
    dom/Node.cpp:2325:    HashSet<SVGElementInstance*> instances = instancesForSVGElement(this);
    dom/Node.cpp:2370:    HashSet<SVGElementInstance*> instances = instancesForSVGElement(this);
    inspector/InspectorDOMAgent.cpp:238:    ListHashSet<RefPtr<Document> > copy = m_documents;
    page/EventHandler.cpp:1780:            HashSet<SVGElementInstance*> instances = lastCorrespondingElement->instancesForElement();
    platform/graphics/mac/MediaPlayerPrivateQTKit.mm:1392:    HashSet<String> commonTypes = mimeCommonTypesCache();
    plugins/PluginView.cpp:327:    HashSet<RefPtr<PluginStream> > streams = m_streams;
Comment 3 Darin Adler 2010-12-02 12:47:59 PST
Comment on attachment 75403 [details]
[PATCH] Switch to copyToVector

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

> WebCore/loader/DocumentLoader.cpp:64
> +    unsigned count = loadersCopy.size();
> +    for (unsigned i = 0; i < count; ++i)

This should be size_t, not unsigned. The local variable name should be size, not count.

> WebCore/loader/DocumentLoader.cpp:73
> +    unsigned count = loadersCopy.size();
> +    for (unsigned i = 0; i < count; ++i)

Same here.
Comment 4 Joseph Pecoraro 2010-12-02 13:24:29 PST
Landed in r73191:
http://trac.webkit.org/changeset/73191

If anyone wants to investigate / address the other cases they can.
Comment 5 Joseph Pecoraro 2010-12-02 14:52:05 PST
Created attachment 75416 [details]
[PATCH] "size_t" instead of "unsigned"

• unsigned is always 32 bit.
• size_t may be 64 bits when built in 64 bit.

So size_t should always be used with other size_t's.
Comment 6 Joseph Pecoraro 2010-12-02 14:58:50 PST
Comment on attachment 75403 [details]
[PATCH] Switch to copyToVector

Clearing flag so queues won't get confused.
Comment 7 WebKit Commit Bot 2010-12-02 15:50:12 PST
Comment on attachment 75416 [details]
[PATCH] "size_t" instead of "unsigned"

Clearing flags on attachment: 75416

Committed r73210: <http://trac.webkit.org/changeset/73210>
Comment 8 WebKit Commit Bot 2010-12-02 15:50:17 PST
All reviewed patches have been landed.  Closing bug.