50405 2010-12-02 12:23:19 -0800 Update ResourceLoaderSet Enumeration 2010-12-02 15:50:17 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 joepeck joepeck commit-queue darin joepeck oldest_to_newest 316333 0 joepeck 2010-12-02 12:23:19 -0800 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. 316334 1 75403 joepeck 2010-12-02 12:25:14 -0800 Created attachment 75403 [PATCH] Switch to copyToVector 316345 2 joepeck 2010-12-02 12:33:11 -0800 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; 316360 3 75403 darin 2010-12-02 12:47:59 -0800 Comment on attachment 75403 [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. 316393 4 joepeck 2010-12-02 13:24:29 -0800 Landed in r73191: http://trac.webkit.org/changeset/73191 If anyone wants to investigate / address the other cases they can. 316430 5 75416 joepeck 2010-12-02 14:52:05 -0800 Created attachment 75416 [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. 316432 6 75403 joepeck 2010-12-02 14:58:50 -0800 Comment on attachment 75403 [PATCH] Switch to copyToVector Clearing flag so queues won't get confused. 316471 7 75416 commit-queue 2010-12-02 15:50:12 -0800 Comment on attachment 75416 [PATCH] "size_t" instead of "unsigned" Clearing flags on attachment: 75416 Committed r73210: <http://trac.webkit.org/changeset/73210> 316472 8 commit-queue 2010-12-02 15:50:17 -0800 All reviewed patches have been landed. Closing bug. 75403 2010-12-02 12:25:14 -0800 2010-12-02 14:58:50 -0800 [PATCH] Switch to copyToVector copyToVector.patch text/plain 1995 joepeck ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg NDk5ZGUyYi4uYTE5Mzc2ZCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNCBAQAorMjAxMC0xMi0wMiAgSm9zZXBoIFBlY29y YXJvICA8am9lcGVja0B3ZWJraXQub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgIFVwZGF0ZSBSZXNvdXJjZUxvYWRlclNldCBFbnVtZXJhdGlvbgor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTA0MDUKKwor ICAgICAgICAqIGxvYWRlci9Eb2N1bWVudExvYWRlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpj YW5jZWxBbGwpOiBpbnN0ZWFkIG9mIGNvcHlpbmcgdGhlIEhhc2hTZXQsIHVzZSBjb3B5VG9WZWN0 b3IuCisgICAgICAgIChXZWJDb3JlOjpzZXRBbGxEZWZlcnNMb2FkaW5nKTogaW5zdGVhZCBvZiBj b3B5aW5nIHRoZSBIYXNoU2V0LCB1c2UgY29weVRvVmVjdG9yLgorCiAyMDEwLTEyLTAyICBDaHJp cyBSb2dlcnMgIDxjcm9nZXJzQGdvb2dsZS5jb20+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgS2Vu bmV0aCBSdXNzZWxsLgpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9sb2FkZXIvRG9jdW1lbnRMb2FkZXIu Y3BwIGIvV2ViQ29yZS9sb2FkZXIvRG9jdW1lbnRMb2FkZXIuY3BwCmluZGV4IGNjNDYxMTMuLmQ0 Mzk1ZDAgMTAwNjQ0Ci0tLSBhL1dlYkNvcmUvbG9hZGVyL0RvY3VtZW50TG9hZGVyLmNwcAorKysg Yi9XZWJDb3JlL2xvYWRlci9Eb2N1bWVudExvYWRlci5jcHAKQEAgLTU4LDE4ICs1OCwyMCBAQCBu YW1lc3BhY2UgV2ViQ29yZSB7CiAKIHN0YXRpYyB2b2lkIGNhbmNlbEFsbChjb25zdCBSZXNvdXJj ZUxvYWRlclNldCYgbG9hZGVycykKIHsKLSAgICBjb25zdCBSZXNvdXJjZUxvYWRlclNldCBjb3B5 ID0gbG9hZGVyczsKLSAgICBSZXNvdXJjZUxvYWRlclNldDo6Y29uc3RfaXRlcmF0b3IgZW5kID0g Y29weS5lbmQoKTsKLSAgICBmb3IgKFJlc291cmNlTG9hZGVyU2V0Ojpjb25zdF9pdGVyYXRvciBp dCA9IGNvcHkuYmVnaW4oKTsgaXQgIT0gZW5kOyArK2l0KQotICAgICAgICAoKml0KS0+Y2FuY2Vs KCk7CisgICAgVmVjdG9yPFJlZlB0cjxSZXNvdXJjZUxvYWRlcj4gPiBsb2FkZXJzQ29weTsKKyAg ICBjb3B5VG9WZWN0b3IobG9hZGVycywgbG9hZGVyc0NvcHkpOworICAgIHVuc2lnbmVkIGNvdW50 ID0gbG9hZGVyc0NvcHkuc2l6ZSgpOworICAgIGZvciAodW5zaWduZWQgaSA9IDA7IGkgPCBjb3Vu dDsgKytpKQorICAgICAgICBsb2FkZXJzQ29weVtpXS0+Y2FuY2VsKCk7CiB9CiAKIHN0YXRpYyB2 b2lkIHNldEFsbERlZmVyc0xvYWRpbmcoY29uc3QgUmVzb3VyY2VMb2FkZXJTZXQmIGxvYWRlcnMs IGJvb2wgZGVmZXJzKQogewotICAgIGNvbnN0IFJlc291cmNlTG9hZGVyU2V0IGNvcHkgPSBsb2Fk ZXJzOwotICAgIFJlc291cmNlTG9hZGVyU2V0Ojpjb25zdF9pdGVyYXRvciBlbmQgPSBjb3B5LmVu ZCgpOwotICAgIGZvciAoUmVzb3VyY2VMb2FkZXJTZXQ6OmNvbnN0X2l0ZXJhdG9yIGl0ID0gY29w eS5iZWdpbigpOyBpdCAhPSBlbmQ7ICsraXQpCi0gICAgICAgICgqaXQpLT5zZXREZWZlcnNMb2Fk aW5nKGRlZmVycyk7CisgICAgVmVjdG9yPFJlZlB0cjxSZXNvdXJjZUxvYWRlcj4gPiBsb2FkZXJz Q29weTsKKyAgICBjb3B5VG9WZWN0b3IobG9hZGVycywgbG9hZGVyc0NvcHkpOworICAgIHVuc2ln bmVkIGNvdW50ID0gbG9hZGVyc0NvcHkuc2l6ZSgpOworICAgIGZvciAodW5zaWduZWQgaSA9IDA7 IGkgPCBjb3VudDsgKytpKQorICAgICAgICBsb2FkZXJzQ29weVtpXS0+c2V0RGVmZXJzTG9hZGlu ZyhkZWZlcnMpOwogfQogCiBEb2N1bWVudExvYWRlcjo6RG9jdW1lbnRMb2FkZXIoY29uc3QgUmVz b3VyY2VSZXF1ZXN0JiByZXEsIGNvbnN0IFN1YnN0aXR1dGVEYXRhJiBzdWJzdGl0dXRlRGF0YSkK 75416 2010-12-02 14:52:05 -0800 2010-12-02 15:50:12 -0800 [PATCH] "size_t" instead of "unsigned" patch.patch text/plain 1557 joepeck ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg YWI2OGExZi4uOTEzYzE2ZiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsNSArMSwxOCBAQAogMjAxMC0xMi0wMiAgSm9zZXBoIFBlY29y YXJvICA8am9lcGVja0B3ZWJraXQub3JnPgogCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgIFVwZGF0ZSBSZXNvdXJjZUxvYWRlclNldCBFbnVtZXJhdGlvbgor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTA0MDUKKwor ICAgICAgICBVc2Ugc2l6ZV90IHdoZW4gd29ya2luZyB3aXRoIFZlY3RvcnMgYW5kIHdoZW4gY29t cGFyaW5nIGJldHdlZW4gc2l6ZV90IHR5cGVzLgorCisgICAgICAgICogbG9hZGVyL0RvY3VtZW50 TG9hZGVyLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OmNhbmNlbEFsbCk6CisgICAgICAgIChXZWJD b3JlOjpzZXRBbGxEZWZlcnNMb2FkaW5nKToKKworMjAxMC0xMi0wMiAgSm9zZXBoIFBlY29yYXJv ICA8am9lcGVja0B3ZWJraXQub3JnPgorCiAgICAgICAgIFJldmlld2VkIGJ5IERhcmluIEFkbGVy LgogCiAgICAgICAgIFVwZGF0ZSBSZXNvdXJjZUxvYWRlclNldCBFbnVtZXJhdGlvbgpkaWZmIC0t Z2l0IGEvV2ViQ29yZS9sb2FkZXIvRG9jdW1lbnRMb2FkZXIuY3BwIGIvV2ViQ29yZS9sb2FkZXIv RG9jdW1lbnRMb2FkZXIuY3BwCmluZGV4IGRjODc1NDIuLjJmMjEwNjYgMTAwNjQ0Ci0tLSBhL1dl YkNvcmUvbG9hZGVyL0RvY3VtZW50TG9hZGVyLmNwcAorKysgYi9XZWJDb3JlL2xvYWRlci9Eb2N1 bWVudExvYWRlci5jcHAKQEAgLTYxLDcgKzYxLDcgQEAgc3RhdGljIHZvaWQgY2FuY2VsQWxsKGNv bnN0IFJlc291cmNlTG9hZGVyU2V0JiBsb2FkZXJzKQogICAgIFZlY3RvcjxSZWZQdHI8UmVzb3Vy Y2VMb2FkZXI+ID4gbG9hZGVyc0NvcHk7CiAgICAgY29weVRvVmVjdG9yKGxvYWRlcnMsIGxvYWRl cnNDb3B5KTsKICAgICBzaXplX3Qgc2l6ZSA9IGxvYWRlcnNDb3B5LnNpemUoKTsKLSAgICBmb3Ig KHVuc2lnbmVkIGkgPSAwOyBpIDwgc2l6ZTsgKytpKQorICAgIGZvciAoc2l6ZV90IGkgPSAwOyBp IDwgc2l6ZTsgKytpKQogICAgICAgICBsb2FkZXJzQ29weVtpXS0+Y2FuY2VsKCk7CiB9CiAKQEAg LTcwLDcgKzcwLDcgQEAgc3RhdGljIHZvaWQgc2V0QWxsRGVmZXJzTG9hZGluZyhjb25zdCBSZXNv dXJjZUxvYWRlclNldCYgbG9hZGVycywgYm9vbCBkZWZlcnMpCiAgICAgVmVjdG9yPFJlZlB0cjxS ZXNvdXJjZUxvYWRlcj4gPiBsb2FkZXJzQ29weTsKICAgICBjb3B5VG9WZWN0b3IobG9hZGVycywg bG9hZGVyc0NvcHkpOwogICAgIHNpemVfdCBzaXplID0gbG9hZGVyc0NvcHkuc2l6ZSgpOwotICAg IGZvciAodW5zaWduZWQgaSA9IDA7IGkgPCBzaXplOyArK2kpCisgICAgZm9yIChzaXplX3QgaSA9 IDA7IGkgPCBzaXplOyArK2kpCiAgICAgICAgIGxvYWRlcnNDb3B5W2ldLT5zZXREZWZlcnNMb2Fk aW5nKGRlZmVycyk7CiB9CiAK