Bug 50405 - Update ResourceLoaderSet Enumeration
Summary: Update ResourceLoaderSet Enumeration
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Joseph Pecoraro
Depends on:
Reported: 2010-12-02 12:23 PST by Joseph Pecoraro
Modified: 2010-12-02 15:50 PST (History)
3 users (show)

See Also:

[PATCH] Switch to copyToVector (1.95 KB, patch)
2010-12-02 12:25 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] "size_t" instead of "unsigned" (1.52 KB, patch)
2010-12-02 14:52 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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:

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.