49320 2010-11-10 07:35:33 -0800 Minor ResourceLoadScheduler cleanups 2010-11-16 20:53:53 -0800 1 1 1 Unclassified WebKit Page Loading 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 koivisto webkit-unassigned ap japhet oldest_to_newest 307347 0 73498 koivisto 2010-11-10 07:35:33 -0800 Created attachment 73498 patch Minor ResourceLoadScheduler cleanups: - remove assertLoaderBeingCounted() as it is wrong (doesn't take deferred resources into account) and not very useful - add HighestPriority, LowestPriority synonyms so new priorities can be added without modifying the iteration code 307367 1 73498 ap 2010-11-10 08:51:28 -0800 Comment on attachment 73498 patch View in context: https://bugs.webkit.org/attachment.cgi?id=73498&action=review > WebCore/ChangeLog:6 > + - remove assertLoaderBeingCounted() as it is wrong (doesn't take deferred resources into account) and not very useful If it can wrongfully fire, we probably need a regression test. But I don't understand what's wrong about it in practice. It's only called when starting the load, can it actually be affected by defers? 307409 2 koivisto 2010-11-10 10:24:54 -0800 (In reply to comment #1) > (From update of attachment 73498 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=73498&action=review > > > WebCore/ChangeLog:6 > > + - remove assertLoaderBeingCounted() as it is wrong (doesn't take deferred resources into account) and not very useful > > If it can wrongfully fire, we probably need a regression test. But I don't understand what's wrong about it in practice. > > It's only called when starting the load, can it actually be affected by defers? There can be deferred higher priority resources pending (perhaps from another document) as the logic in servePendingRequests skips those. The claim that the first resource in the highest priority queue is always loaded first doesn't hold. I hit it loading wsj.com and checked the state in debugger. I think the correct logic for this gets overly complicated for an assert and will be fragile against any future changes. This is only ever invoked from servePendingRequests (through ResourceLoader::start()) and basically checks that some state matches what servePendingRequests makes it at that point. I think it is unlikely to catch anything interesting. Rather than correcting it I think it is better to simply remove it. 307411 3 ap 2010-11-10 10:29:05 -0800 Thanks for explanation! So it sounds like this could have a regression test. 308013 4 73498 ap 2010-11-11 11:20:00 -0800 Comment on attachment 73498 patch Marking r- per bug 49351 comment 9 (changes in bug 49351 make this assertion correct, and it helped find an actual problem). 310109 5 koivisto 2010-11-16 20:53:43 -0800 Landed the priority enum cleanup part of the patch in http://trac.webkit.org/changeset/72171 The assert was already removed elsewhere. 73498 2010-11-10 07:35:33 -0800 2010-11-16 20:53:53 -0800 patch resource-load-scheduler-cleanups.patch text/plain 6984 koivisto SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA3MTczNikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMjIgQEAKKzIwMTAtMTEtMTAgIEFudHRpIEtvaXZpc3RvICA8YW50dGlAYXBwbGUu Y29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIE1p bm9yIFJlc291cmNlTG9hZFNjaGVkdWxlciBjbGVhbnVwczoKKyAgICAgICAgLSByZW1vdmUgYXNz ZXJ0TG9hZGVyQmVpbmdDb3VudGVkKCkgYXMgaXQgaXMgd3JvbmcgKGRvZXNuJ3QgdGFrZSBkZWZl cnJlZCByZXNvdXJjZXMgaW50byBhY2NvdW50KSBhbmQgbm90IHZlcnkgdXNlZnVsCisgICAgICAg IC0gYWRkIEhpZ2hlc3RQcmlvcml0eSwgTG93ZXN0UHJpb3JpdHkgc3lub255bXMgc28gbmV3IHBy aW9yaXRpZXMgY2FuIGJlIGFkZGVkIHdpdGhvdXQgbW9kaWZ5aW5nIHRoZSBpdGVyYXRpb24gY29k ZSAKKworICAgICAgICAqIGxvYWRlci9SZXNvdXJjZUxvYWRTY2hlZHVsZXIuY3BwOgorICAgICAg ICAoV2ViQ29yZTo6UmVzb3VyY2VMb2FkU2NoZWR1bGVyOjpzY2hlZHVsZUxvYWQpOgorICAgICAg ICAoV2ViQ29yZTo6UmVzb3VyY2VMb2FkU2NoZWR1bGVyOjpzZXJ2ZVBlbmRpbmdSZXF1ZXN0cyk6 CisgICAgICAgIChXZWJDb3JlOjpSZXNvdXJjZUxvYWRTY2hlZHVsZXI6Okhvc3RJbmZvcm1hdGlv bjo6fkhvc3RJbmZvcm1hdGlvbik6CisgICAgICAgIChXZWJDb3JlOjpSZXNvdXJjZUxvYWRTY2hl ZHVsZXI6Okhvc3RJbmZvcm1hdGlvbjo6cmVtb3ZlKToKKyAgICAgICAgKFdlYkNvcmU6OlJlc291 cmNlTG9hZFNjaGVkdWxlcjo6SG9zdEluZm9ybWF0aW9uOjpoYXNSZXF1ZXN0cyk6CisgICAgICAg ICogbG9hZGVyL1Jlc291cmNlTG9hZFNjaGVkdWxlci5oOgorICAgICAgICAoV2ViQ29yZTo6UmVz b3VyY2VMb2FkU2NoZWR1bGVyOjpIb3N0SW5mb3JtYXRpb246OmxpbWl0UmVxdWVzdHMpOgorICAg ICAgICAqIGxvYWRlci9SZXNvdXJjZUxvYWRlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpSZXNv dXJjZUxvYWRlcjo6c3RhcnQpOgorCiAyMDEwLTExLTEwICBTaGVyaWZmIEJvdCAgPHdlYmtpdC5y ZXZpZXcuYm90QGdtYWlsLmNvbT4KIAogICAgICAgICBVbnJldmlld2VkLCByb2xsaW5nIG91dCBy NzE3MzMuCkluZGV4OiBXZWJDb3JlL2xvYWRlci9SZXNvdXJjZUxvYWRTY2hlZHVsZXIuY3BwCj09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT0KLS0tIFdlYkNvcmUvbG9hZGVyL1Jlc291cmNlTG9hZFNjaGVkdWxlci5jcHAJKHJl dmlzaW9uIDcxNzMxKQorKysgV2ViQ29yZS9sb2FkZXIvUmVzb3VyY2VMb2FkU2NoZWR1bGVyLmNw cAkod29ya2luZyBjb3B5KQpAQCAtMTA2LDcgKzEwNiw3IEBAIHZvaWQgUmVzb3VyY2VMb2FkU2No ZWR1bGVyOjpzY2hlZHVsZUxvYWQKIHsKICAgICBBU1NFUlQocmVzb3VyY2VMb2FkZXIpOwogI2lm ICFSRVFVRVNUX01BTkFHRU1FTlRfRU5BQkxFRAotICAgIHByaW9yaXR5ID0gSGlnaDsKKyAgICBw cmlvcml0eSA9IEhpZ2hlc3RQcmlvcml0eTsKICNlbmRpZgogCiAgICAgTE9HKFJlc291cmNlTG9h ZGluZywgIlJlc291cmNlTG9hZFNjaGVkdWxlcjo6bG9hZCByZXNvdXJjZSAlcCAnJXMnIiwgcmVz b3VyY2VMb2FkZXIsIHJlc291cmNlTG9hZGVyLT51cmwoKS5zdHJpbmcoKS5sYXRpbjEoKS5kYXRh KCkpOwpAQCAtMTc3LDcgKzE3Nyw3IEBAIHZvaWQgUmVzb3VyY2VMb2FkU2NoZWR1bGVyOjpzZXJ2 ZVBlbmRpbmcKIHsKICAgICBMT0coUmVzb3VyY2VMb2FkaW5nLCAiUmVzb3VyY2VMb2FkU2NoZWR1 bGVyOjpzZXJ2ZVBlbmRpbmdSZXF1ZXN0cyBIb3N0SW5mb3JtYXRpb24ubV9uYW1lPSclcyciLCBo b3N0LT5uYW1lKCkubGF0aW4xKCkuZGF0YSgpKTsKIAotICAgIGZvciAoaW50IHByaW9yaXR5ID0g SGlnaDsgcHJpb3JpdHkgPj0gbWluaW11bVByaW9yaXR5OyAtLXByaW9yaXR5KSB7CisgICAgZm9y IChpbnQgcHJpb3JpdHkgPSBIaWdoZXN0UHJpb3JpdHk7IHByaW9yaXR5ID49IG1pbmltdW1Qcmlv cml0eTsgLS1wcmlvcml0eSkgewogICAgICAgICBIb3N0SW5mb3JtYXRpb246OlJlcXVlc3RRdWV1 ZSYgcmVxdWVzdHNQZW5kaW5nID0gaG9zdC0+cmVxdWVzdHNQZW5kaW5nKChQcmlvcml0eSkgcHJp b3JpdHkpOwogICAgICAgICBIb3N0SW5mb3JtYXRpb246OlJlcXVlc3RRdWV1ZSBkZWZlcnJlZFJl cXVlc3RzOwogCkBAIC0yMzMsMjkgKzIzMyw2IEBAIHZvaWQgUmVzb3VyY2VMb2FkU2NoZWR1bGVy OjpyZXF1ZXN0VGltZXIKICAgICBzZXJ2ZVBlbmRpbmdSZXF1ZXN0cygpOwogfQogCi0jaWZuZGVm IE5ERUJVRwotdm9pZCBSZXNvdXJjZUxvYWRTY2hlZHVsZXI6OmFzc2VydExvYWRlckJlaW5nQ291 bnRlZChSZXNvdXJjZUxvYWRlciogcmVzb3VyY2VMb2FkZXIpCi17Ci0gICAgSG9zdEluZm9ybWF0 aW9uKiBob3N0ID0gaG9zdEZvclVSTChyZXNvdXJjZUxvYWRlci0+dXJsKCkpOwotICAgIEFTU0VS VChob3N0KTsKLSAgICBob3N0LT5hc3NlcnRMb2FkZXJCZWluZ0NvdW50ZWQocmVzb3VyY2VMb2Fk ZXIpOwotfQotCi12b2lkIFJlc291cmNlTG9hZFNjaGVkdWxlcjo6SG9zdEluZm9ybWF0aW9uOjph c3NlcnRMb2FkZXJCZWluZ0NvdW50ZWQoUmVzb3VyY2VMb2FkZXIqIHJlc291cmNlTG9hZGVyKQot ewotICAgIC8vIElmIGEgbG9hZCBpcyBiZWluZyBzdGFydGVkLCBpdCBzaG91bGQgYmUgYXQgdGhl IGZyb250IG9mIHRoZSBoaWdoZXN0IHByaW9yaXR5IHF1ZXVlCi0gICAgLy8gdGhhdCBhY3R1YWxs eSBjb250YWlucyBhIHJlcXVlc3QuCi0gICAgZm9yIChpbnQgcHJpb3JpdHkgPSBIaWdoOyBwcmlv cml0eSA+PSBWZXJ5TG93OyAtLXByaW9yaXR5KSB7ICAKLSAgICAgICAgaWYgKCFtX3JlcXVlc3Rz UGVuZGluZ1twcmlvcml0eV0uaXNFbXB0eSgpKSB7Ci0gICAgICAgICAgICBBU1NFUlQobV9yZXF1 ZXN0c1BlbmRpbmdbcHJpb3JpdHldLmZpcnN0KCkuZ2V0KCkgPT0gcmVzb3VyY2VMb2FkZXIpOwot ICAgICAgICAgICAgcmV0dXJuOwotICAgICAgICB9Ci0gICAgfQotCi0gICAgQVNTRVJUX05PVF9S RUFDSEVEKCk7Ci19Ci0jZW5kaWYKLQogUmVzb3VyY2VMb2FkU2NoZWR1bGVyOjpIb3N0SW5mb3Jt YXRpb246Okhvc3RJbmZvcm1hdGlvbihjb25zdCBTdHJpbmcmIG5hbWUsIHVuc2lnbmVkIG1heFJl cXVlc3RzSW5GbGlnaHQpCiAgICAgOiBtX25hbWUobmFtZSkKICAgICAsIG1fbWF4UmVxdWVzdHNJ bkZsaWdodChtYXhSZXF1ZXN0c0luRmxpZ2h0KQpAQCAtMjY1LDcgKzI0Miw3IEBAIFJlc291cmNl TG9hZFNjaGVkdWxlcjo6SG9zdEluZm9ybWF0aW9uOjoKIFJlc291cmNlTG9hZFNjaGVkdWxlcjo6 SG9zdEluZm9ybWF0aW9uOjp+SG9zdEluZm9ybWF0aW9uKCkKIHsKICAgICBBU1NFUlQobV9yZXF1 ZXN0c0xvYWRpbmcuaXNFbXB0eSgpKTsKLSAgICBmb3IgKHVuc2lnbmVkIHAgPSAwOyBwIDw9IEhp Z2g7IHArKykKKyAgICBmb3IgKHVuc2lnbmVkIHAgPSAwOyBwIDw9IEhpZ2hlc3RQcmlvcml0eTsg cCsrKQogICAgICAgICBBU1NFUlQobV9yZXF1ZXN0c1BlbmRpbmdbcF0uaXNFbXB0eSgpKTsKIH0K ICAgICAKQEAgLTI4Nyw3ICsyNjQsNyBAQCB2b2lkIFJlc291cmNlTG9hZFNjaGVkdWxlcjo6SG9z dEluZm9ybWF0CiAgICAgICAgIHJldHVybjsKICAgICB9CiAgICAgCi0gICAgZm9yIChpbnQgcHJp b3JpdHkgPSBIaWdoOyBwcmlvcml0eSA+PSBWZXJ5TG93OyAtLXByaW9yaXR5KSB7ICAKKyAgICBm b3IgKGludCBwcmlvcml0eSA9IEhpZ2hlc3RQcmlvcml0eTsgcHJpb3JpdHkgPj0gTG93ZXN0UHJp b3JpdHk7IC0tcHJpb3JpdHkpIHsgIAogICAgICAgICBSZXF1ZXN0UXVldWU6Oml0ZXJhdG9yIGVu ZCA9IG1fcmVxdWVzdHNQZW5kaW5nW3ByaW9yaXR5XS5lbmQoKTsKICAgICAgICAgZm9yIChSZXF1 ZXN0UXVldWU6Oml0ZXJhdG9yIGl0ID0gbV9yZXF1ZXN0c1BlbmRpbmdbcHJpb3JpdHldLmJlZ2lu KCk7IGl0ICE9IGVuZDsgKytpdCkgewogICAgICAgICAgICAgaWYgKCppdCA9PSByZXNvdXJjZUxv YWRlcikgewpAQCAtMzAyLDcgKzI3OSw3IEBAIGJvb2wgUmVzb3VyY2VMb2FkU2NoZWR1bGVyOjpI b3N0SW5mb3JtYXQKIHsKICAgICBpZiAoIW1fcmVxdWVzdHNMb2FkaW5nLmlzRW1wdHkoKSkKICAg ICAgICAgcmV0dXJuIHRydWU7Ci0gICAgZm9yICh1bnNpZ25lZCBwID0gMDsgcCA8PSBIaWdoOyBw KyspIHsKKyAgICBmb3IgKHVuc2lnbmVkIHAgPSAwOyBwIDw9IEhpZ2hlc3RQcmlvcml0eTsgcCsr KSB7CiAgICAgICAgIGlmICghbV9yZXF1ZXN0c1BlbmRpbmdbcF0uaXNFbXB0eSgpKQogICAgICAg ICAgICAgcmV0dXJuIHRydWU7CiAgICAgfQpJbmRleDogV2ViQ29yZS9sb2FkZXIvUmVzb3VyY2VM b2FkU2NoZWR1bGVyLmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9sb2FkZXIvUmVzb3VyY2VMb2Fk U2NoZWR1bGVyLmgJKHJldmlzaW9uIDcxNzMxKQorKysgV2ViQ29yZS9sb2FkZXIvUmVzb3VyY2VM b2FkU2NoZWR1bGVyLmgJKHdvcmtpbmcgY29weSkKQEAgLTQ3LDcgKzQ3LDcgQEAgY2xhc3MgUmVz b3VyY2VMb2FkU2NoZWR1bGVyIDogcHVibGljIE5vbgogcHVibGljOgogICAgIGZyaWVuZCBSZXNv dXJjZUxvYWRTY2hlZHVsZXIqIHJlc291cmNlTG9hZFNjaGVkdWxlcigpOwogICAgIAotICAgIGVu dW0gUHJpb3JpdHkgeyBWZXJ5TG93LCBMb3csIE1lZGl1bSwgSGlnaCB9OworICAgIGVudW0gUHJp b3JpdHkgeyBWZXJ5TG93LCBMb3csIE1lZGl1bSwgSGlnaCwgSGlnaGVzdFByaW9yaXR5ID0gSGln aCwgTG93ZXN0UHJpb3JpdHkgPSBWZXJ5TG93IH07CiAgICAgUGFzc1JlZlB0cjxTdWJyZXNvdXJj ZUxvYWRlcj4gc2NoZWR1bGVTdWJyZXNvdXJjZUxvYWQoRnJhbWUqLCBTdWJyZXNvdXJjZUxvYWRl ckNsaWVudCosIGNvbnN0IFJlc291cmNlUmVxdWVzdCYsIFByaW9yaXR5ID0gTG93LCBTZWN1cml0 eUNoZWNrUG9saWN5ID0gRG9TZWN1cml0eUNoZWNrLCBib29sIHNlbmRSZXNvdXJjZUxvYWRDYWxs YmFja3MgPSB0cnVlLCBib29sIHNob3VsZENvbnRlbnRTbmlmZiA9IHRydWUpOwogICAgIFBhc3NS ZWZQdHI8TmV0c2NhcGVQbHVnSW5TdHJlYW1Mb2FkZXI+IHNjaGVkdWxlUGx1Z2luU3RyZWFtTG9h ZChGcmFtZSosIE5ldHNjYXBlUGx1Z0luU3RyZWFtTG9hZGVyQ2xpZW50KiwgY29uc3QgUmVzb3Vy Y2VSZXF1ZXN0Jik7CiAgICAgdm9pZCBhZGRNYWluUmVzb3VyY2VMb2FkKFJlc291cmNlTG9hZGVy Kik7CkBAIC01NywxMCArNTcsNiBAQCBwdWJsaWM6CiAgICAgdm9pZCBzZXJ2ZVBlbmRpbmdSZXF1 ZXN0cyhQcmlvcml0eSBtaW5pbXVtUHJpb3JpdHkgPSBWZXJ5TG93KTsKICAgICB2b2lkIHN1c3Bl bmRQZW5kaW5nUmVxdWVzdHMoKTsKICAgICB2b2lkIHJlc3VtZVBlbmRpbmdSZXF1ZXN0cygpOwot ICAgIAotI2lmbmRlZiBOREVCVUcKLSAgICB2b2lkIGFzc2VydExvYWRlckJlaW5nQ291bnRlZChS ZXNvdXJjZUxvYWRlciopOwotI2VuZGlmCiAKIHByaXZhdGU6CiAgICAgUmVzb3VyY2VMb2FkU2No ZWR1bGVyKCk7CkBAIC04MSwxNiArNzcsMTIgQEAgcHJpdmF0ZToKICAgICAgICAgdm9pZCByZW1v dmUoUmVzb3VyY2VMb2FkZXIqKTsKICAgICAgICAgYm9vbCBoYXNSZXF1ZXN0cygpIGNvbnN0Owog ICAgICAgICBib29sIGxpbWl0UmVxdWVzdHMoKSBjb25zdCB7IHJldHVybiBtX3JlcXVlc3RzTG9h ZGluZy5zaXplKCkgPj0gbV9tYXhSZXF1ZXN0c0luRmxpZ2h0OyB9Ci0gICAgICAgIAotI2lmbmRl ZiBOREVCVUcKLSAgICAgICAgdm9pZCBhc3NlcnRMb2FkZXJCZWluZ0NvdW50ZWQoUmVzb3VyY2VM b2FkZXIqKTsKLSNlbmRpZgogCiAgICAgICAgIHR5cGVkZWYgRGVxdWU8UmVmUHRyPFJlc291cmNl TG9hZGVyPiA+IFJlcXVlc3RRdWV1ZTsKICAgICAgICAgUmVxdWVzdFF1ZXVlJiByZXF1ZXN0c1Bl bmRpbmcoUHJpb3JpdHkgcHJpb3JpdHkpIHsgcmV0dXJuIG1fcmVxdWVzdHNQZW5kaW5nW3ByaW9y aXR5XTsgfQogCiAgICAgcHJpdmF0ZTogICAgICAgICAgICAgICAgICAgIAotICAgICAgICBSZXF1 ZXN0UXVldWUgbV9yZXF1ZXN0c1BlbmRpbmdbSGlnaCArIDFdOworICAgICAgICBSZXF1ZXN0UXVl dWUgbV9yZXF1ZXN0c1BlbmRpbmdbSGlnaGVzdFByaW9yaXR5ICsgMV07CiAgICAgICAgIHR5cGVk ZWYgSGFzaFNldDxSZWZQdHI8UmVzb3VyY2VMb2FkZXI+ID4gUmVxdWVzdE1hcDsKICAgICAgICAg UmVxdWVzdE1hcCBtX3JlcXVlc3RzTG9hZGluZzsKICAgICAgICAgY29uc3QgU3RyaW5nIG1fbmFt ZTsKSW5kZXg6IFdlYkNvcmUvbG9hZGVyL1Jlc291cmNlTG9hZGVyLmNwcAo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBXZWJDb3JlL2xvYWRlci9SZXNvdXJjZUxvYWRlci5jcHAJKHJldmlzaW9uIDcxNzMxKQorKysg V2ViQ29yZS9sb2FkZXIvUmVzb3VyY2VMb2FkZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xNDMs MTAgKzE0Myw3IEBAIGJvb2wgUmVzb3VyY2VMb2FkZXI6OmxvYWQoY29uc3QgUmVzb3VyY2UKIGJv b2wgUmVzb3VyY2VMb2FkZXI6OnN0YXJ0KCkKIHsKICAgICBBU1NFUlQoIW1faGFuZGxlKTsKLSNp Zm5kZWYgTkRFQlVHCi0gICAgcmVzb3VyY2VMb2FkU2NoZWR1bGVyKCktPmFzc2VydExvYWRlckJl aW5nQ291bnRlZCh0aGlzKTsKLSNlbmRpZgotICAgIAorCiAgICAgaWYgKG1fZG9jdW1lbnRMb2Fk ZXItPnNjaGVkdWxlQXJjaGl2ZUxvYWQodGhpcywgbV9yZXF1ZXN0LCBtX3JlcXVlc3QudXJsKCkp KQogICAgICAgICByZXR1cm4gdHJ1ZTsKICAgICAK