Bug 50405 – Update ResourceLoaderSet Enumeration

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 75403 [details]
[PATCH] Switch to copyToVector

[Joseph Pecoraro]

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;

[Darin Adler]

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.

[Joseph Pecoraro]

Landed in r73191:
http://trac.webkit.org/changeset/73191

If anyone wants to investigate / address the other cases they can.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Comment on attachment 75403 [details]
[PATCH] Switch to copyToVector

Clearing flag so queues won't get confused.

[WebKit Commit Bot]

Comment on attachment 75416 [details]
[PATCH] "size_t" instead of "unsigned"

Clearing flags on attachment: 75416

Committed r73210: <http://trac.webkit.org/changeset/73210>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.