6219 2005-12-23 07:14:46 -0800 Perf regression: -[NSImage initWithData:] called repeatedly while moving the cursor over an image 2006-02-25 00:12:49 -0800 1 1 1 Unclassified WebKit WebKit Misc. 420+ Mac OS X 10.4 VERIFIED FIXED P2 Normal --- 1 mitz webkit-unassigned darin oldest_to_newest 26432 0 mitz 2005-12-23 07:14:46 -0800 Each call to -[WebView elementAtPoint:] for a point inside an image entails a call to -[NSImage initWithData:] to fill the WebElementImageKey. This results in pretty high CPU usage when simply moving the mouse cursor across a big image. See the testcase for example. I think this started with the changes to QPixmap when bug 5689 was fixed. 26433 1 5250 mitz 2005-12-23 07:15:43 -0800 Created attachment 5250 Testcase 26434 2 eric 2005-12-23 10:03:52 -0800 Great catch. 27300 3 5395 mitz 2005-12-31 04:30:48 -0800 Created attachment 5395 Lame patch With this patch, initWithData: will still be called once per image, which is almost always a waste of time. The non-lame approach, I think, is to use an NSImage subclass or a proxy that does lazy instantiation as the value of WebElementImageKey in the element dictionary. 27401 4 5395 darin 2005-12-31 18:14:17 -0800 Comment on attachment 5395 Lame patch Better to just change the pixmap() function to return a const QPixmap& -- that would do the trick. 27412 5 5406 mitz 2006-01-01 01:43:52 -0800 Created attachment 5406 Change pixmap() to return a const QPixmap& I really ought to learn that "programming" thing. 27413 6 5406 mitz 2006-01-01 01:49:57 -0800 Comment on attachment 5406 Change pixmap() to return a const QPixmap& Maybe there should be a FIXME in WebHTMLView where it creates the NSImage saying that it would be better to delay the actual NSImage instantiation? It seems that in an overwhelming majority of cases, the WebElementImageKey isn't used. Another option, instead of using a proxy or an NSImage subclass, is to use an NSMutableDictionary subclass for the result of -[WebHTMLView elementAtPoint:] that knows how to generate the value of the WebElementImageKey from the WebCoreElementImageRendererKey. 27416 7 mitz 2006-01-01 07:21:41 -0800 Okay, the last suggestion was really stupid. 27505 8 5406 darin 2006-01-02 14:35:51 -0800 Comment on attachment 5406 Change pixmap() to return a const QPixmap& Looks fine, r=me. Good suggestions about implementing our own Objective-C dictionary so we can compute this lazily. 27509 9 darin 2006-01-02 14:43:14 -0800 Mitz's last suggestion was *not* stupid. We can't create an NSMutableDictionary subclass, but we can make our own dictionary class that's part of the NSDictionary "class cluster". And that is indeed the best way to fix this "for real". And yes, I think the the FIXME is a good idea. 27561 10 mjs 2006-01-03 00:35:56 -0800 Agreed, in fact some APIs in AppKit return custom NSDictionary subclasses. 27935 11 darin 2006-01-07 10:20:10 -0800 To be even more specific, we should use an NSDictionary subclass. It only has to implement the three methods count, objectForKey:, and keyEnumerator. And the keyEnumerator will be an NSEnumerator that only has to implement nextObject. We should make sure there's a method on DOMNode or DOMElement for each of the properties and then we can just map the dictionary key to a selector and call it on the DOMElement. 33944 12 mitz 2006-02-25 00:12:49 -0800 See bug 7450 5250 2005-12-23 07:15:43 -0800 2005-12-23 07:15:43 -0800 Testcase fatImage.html text/html 151 mitz PGJvZHk+CjxpbWcgc3JjPSJodHRwOi8vZWFydGhvYnNlcnZhdG9yeS5uYXNhLmdvdi9OYXR1cmFs SGF6YXJkcy9BcmNoaXZlL1NlcDIwMDIvaW5kb2NoaW5hXzE0c2VwMDJfMjUwbV8yMjEudGlmZi5q cGciIHdpZHRoPSIyMDAiIGhlaWdodD0iMjAwIj4KPC9ib2R5Pg== 5395 2005-12-31 04:30:48 -0800 2006-01-01 01:43:52 -0800 Lame patch 6219_easy_r1.patch text/plain 1798 mitz SW5kZXg6IFdlYkNvcmUva2h0bWwvcmVuZGVyaW5nL3JlbmRlcl9pbWFnZS5oCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K UkNTIGZpbGU6IC9jdnMvcm9vdC9XZWJDb3JlL2todG1sL3JlbmRlcmluZy9yZW5kZXJfaW1hZ2Uu aCx2CnJldHJpZXZpbmcgcmV2aXNpb24gMS4yOQpkaWZmIC1wIC11IC1yMS4yOSBXZWJDb3JlL2to dG1sL3JlbmRlcmluZy9yZW5kZXJfaW1hZ2UuaAotLS0gV2ViQ29yZS9raHRtbC9yZW5kZXJpbmcv cmVuZGVyX2ltYWdlLmgJMjEgTm92IDIwMDUgMDE6MjA6MjUgLTAwMDAJMS4yOQorKysgV2ViQ29y ZS9raHRtbC9yZW5kZXJpbmcvcmVuZGVyX2ltYWdlLmgJMzEgRGVjIDIwMDUgMTI6MzE6MDQgLTAw MDAKQEAgLTU2LDYgKzU2LDcgQEAgcHVibGljOgogICAgIHZpcnR1YWwgdm9pZCBzZXRQaXhtYXAo IGNvbnN0IFFQaXhtYXAgJiwgY29uc3QgUVJlY3QmLCBDYWNoZWRJbWFnZSAqKTsKIAogICAgIFFQ aXhtYXAgcGl4bWFwKCkgY29uc3QgeyByZXR1cm4gcGl4OyB9CisgICAgY29uc3QgUVBpeG1hcCAq cGl4bWFwUHRyKCkgY29uc3QgeyByZXR1cm4gJnBpeDsgfQogICAgIC8vIGRvbid0IGV2ZW4gdGhp bmsgYWJvdXQgbWFraW5nIHRoaXMgbWV0aG9kIHZpcnR1YWwhCiAgICAgRE9NOjpIVE1MRWxlbWVu dEltcGwqIGVsZW1lbnQoKSBjb25zdAogICAgIHsgcmV0dXJuIHN0YXRpY19jYXN0PERPTTo6SFRN TEVsZW1lbnRJbXBsKj4oUmVuZGVyT2JqZWN0OjplbGVtZW50KCkpOyB9CkluZGV4OiBXZWJDb3Jl L2t3cS9XZWJDb3JlQnJpZGdlLm1tCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KUkNTIGZpbGU6IC9jdnMvcm9vdC9XZWJD b3JlL2t3cS9XZWJDb3JlQnJpZGdlLm1tLHYKcmV0cmlldmluZyByZXZpc2lvbiAxLjQ0NgpkaWZm IC1wIC11IC1yMS40NDYgV2ViQ29yZS9rd3EvV2ViQ29yZUJyaWRnZS5tbQotLS0gV2ViQ29yZS9r d3EvV2ViQ29yZUJyaWRnZS5tbQkyNiBEZWMgMjAwNSAyMTo0NjoxNyAtMDAwMAkxLjQ0NgorKysg V2ViQ29yZS9rd3EvV2ViQ29yZUJyaWRnZS5tbQkzMSBEZWMgMjAwNSAxMjozMToxMSAtMDAwMApA QCAtMTEyNCw5ICsxMTI0LDkgQEAgc3RhdGljIEhUTUxGb3JtRWxlbWVudEltcGwgKmZvcm1FbGVt ZW50RgogICAgICAgICBpZiAobm9kZS0+cmVuZGVyZXIoKSAmJiBub2RlLT5yZW5kZXJlcigpLT5p c0ltYWdlKCkpIHsKICAgICAgICAgICAgIFJlbmRlckltYWdlICpyID0gc3RhdGljX2Nhc3Q8UmVu ZGVySW1hZ2UgKj4obm9kZS0+cmVuZGVyZXIoKSk7CiAgICAgICAgICAgICBpZiAoIXItPmlzRGlz cGxheWluZ0Vycm9yKCkpIHsKLSAgICAgICAgICAgICAgICBRUGl4bWFwIHAgPSByLT5waXhtYXAo KTsKLSAgICAgICAgICAgICAgICBpZiAocC5pbWFnZVJlbmRlcmVyKCkpCi0gICAgICAgICAgICAg ICAgICAgIFtlbGVtZW50IHNldE9iamVjdDpwLmltYWdlUmVuZGVyZXIoKSBmb3JLZXk6V2ViQ29y ZUVsZW1lbnRJbWFnZVJlbmRlcmVyS2V5XTsKKyAgICAgICAgICAgICAgICBjb25zdCBRUGl4bWFw ICpwID0gci0+cGl4bWFwUHRyKCk7CisgICAgICAgICAgICAgICAgaWYgKHAtPmltYWdlUmVuZGVy ZXIoKSkKKyAgICAgICAgICAgICAgICAgICAgW2VsZW1lbnQgc2V0T2JqZWN0OnAtPmltYWdlUmVu ZGVyZXIoKSBmb3JLZXk6V2ViQ29yZUVsZW1lbnRJbWFnZVJlbmRlcmVyS2V5XTsKICAgICAgICAg ICAgIH0KIAogICAgICAgICAgICAgaW50IHgsIHk7Cg== 5406 2006-01-01 01:43:52 -0800 2006-01-02 14:35:51 -0800 Change pixmap() to return a const QPixmap& 6219_easy_r2.patch text/plain 2542 mitz SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KUkNTIGZpbGU6IC9jdnMvcm9vdC9X ZWJDb3JlL0NoYW5nZUxvZyx2CnJldHJpZXZpbmcgcmV2aXNpb24gMS43MQpkaWZmIC1wIC11IC1y MS43MSBXZWJDb3JlL0NoYW5nZUxvZwotLS0gV2ViQ29yZS9DaGFuZ2VMb2cJMzEgRGVjIDIwMDUg MDA6MTQ6MTkgLTAwMDAJMS43MQorKysgV2ViQ29yZS9DaGFuZ2VMb2cJMSBKYW4gMjAwNiAwOTo0 ODozOCAtMDAwMApAQCAtMSwzICsxLDE3IEBACisyMDA2LTAxLSMjICBNaXR6IFBldHRlbCAgPG9w ZW5kYXJ3aW4ub3JnQG1pdHpwZXR0ZWwuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIC0gZml4IGh0dHA6Ly9idWd6aWxsYS5vcGVuZGFyd2luLm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9NjIxOQorICAgICAgICAgIFBlcmYgcmVncmVzc2lvbjogLVtOU0lt YWdlIGluaXRXaXRoRGF0YTpdIGNhbGxlZCByZXBlYXRlZGx5IHdoaWxlCisgICAgICAgICAgbW92 aW5nIHRoZSBjdXJzb3Igb3ZlciBhbiBpbWFnZQorCisgICAgICAgICoga2h0bWwvcmVuZGVyaW5n L3JlbmRlcl9pbWFnZS5oOgorICAgICAgICAoa2h0bWw6OlJlbmRlckltYWdlOjpwaXhtYXApOiBS ZXR1cm4gYSBjb25zdCBRUGl4bWFwJi4KKyAgICAgICAgKiBrd3EvV2ViQ29yZUJyaWRnZS5tbToK KyAgICAgICAgKC1bV2ViQ29yZUJyaWRnZSBlbGVtZW50QXRQb2ludDpdKTogQXZvaWQgY29weWlu ZyB0aGUgUVBpeG1hcCBhbmQgaXRzCisgICAgICAgIHJlbmRlcmVyLgorCiAyMDA1LTEyLTMwICBE YXJpbiBBZGxlciAgPGRhcmluQGFwcGxlLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBNYWNp ZWouCkluZGV4OiBXZWJDb3JlL2todG1sL3JlbmRlcmluZy9yZW5kZXJfaW1hZ2UuaAo9PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09ClJDUyBmaWxlOiAvY3ZzL3Jvb3QvV2ViQ29yZS9raHRtbC9yZW5kZXJpbmcvcmVuZGVyX2lt YWdlLmgsdgpyZXRyaWV2aW5nIHJldmlzaW9uIDEuMjkKZGlmZiAtcCAtdSAtcjEuMjkgV2ViQ29y ZS9raHRtbC9yZW5kZXJpbmcvcmVuZGVyX2ltYWdlLmgKLS0tIFdlYkNvcmUva2h0bWwvcmVuZGVy aW5nL3JlbmRlcl9pbWFnZS5oCTIxIE5vdiAyMDA1IDAxOjIwOjI1IC0wMDAwCTEuMjkKKysrIFdl YkNvcmUva2h0bWwvcmVuZGVyaW5nL3JlbmRlcl9pbWFnZS5oCTEgSmFuIDIwMDYgMDc6MjU6NTcg LTAwMDAKQEAgLTU1LDcgKzU1LDcgQEAgcHVibGljOgogCiAgICAgdmlydHVhbCB2b2lkIHNldFBp eG1hcCggY29uc3QgUVBpeG1hcCAmLCBjb25zdCBRUmVjdCYsIENhY2hlZEltYWdlICopOwogCi0g ICAgUVBpeG1hcCBwaXhtYXAoKSBjb25zdCB7IHJldHVybiBwaXg7IH0KKyAgICBjb25zdCBRUGl4 bWFwJiBwaXhtYXAoKSBjb25zdCB7IHJldHVybiBwaXg7IH0KICAgICAvLyBkb24ndCBldmVuIHRo aW5rIGFib3V0IG1ha2luZyB0aGlzIG1ldGhvZCB2aXJ0dWFsIQogICAgIERPTTo6SFRNTEVsZW1l bnRJbXBsKiBlbGVtZW50KCkgY29uc3QKICAgICB7IHJldHVybiBzdGF0aWNfY2FzdDxET006OkhU TUxFbGVtZW50SW1wbCo+KFJlbmRlck9iamVjdDo6ZWxlbWVudCgpKTsgfQpJbmRleDogV2ViQ29y ZS9rd3EvV2ViQ29yZUJyaWRnZS5tbQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09ClJDUyBmaWxlOiAvY3ZzL3Jvb3QvV2Vi Q29yZS9rd3EvV2ViQ29yZUJyaWRnZS5tbSx2CnJldHJpZXZpbmcgcmV2aXNpb24gMS40NDYKZGlm ZiAtcCAtdSAtcjEuNDQ2IFdlYkNvcmUva3dxL1dlYkNvcmVCcmlkZ2UubW0KLS0tIFdlYkNvcmUv a3dxL1dlYkNvcmVCcmlkZ2UubW0JMjYgRGVjIDIwMDUgMjE6NDY6MTcgLTAwMDAJMS40NDYKKysr IFdlYkNvcmUva3dxL1dlYkNvcmVCcmlkZ2UubW0JMSBKYW4gMjAwNiAwNzoyNjowNSAtMDAwMApA QCAtMTEyNCw3ICsxMTI0LDcgQEAgc3RhdGljIEhUTUxGb3JtRWxlbWVudEltcGwgKmZvcm1FbGVt ZW50RgogICAgICAgICBpZiAobm9kZS0+cmVuZGVyZXIoKSAmJiBub2RlLT5yZW5kZXJlcigpLT5p c0ltYWdlKCkpIHsKICAgICAgICAgICAgIFJlbmRlckltYWdlICpyID0gc3RhdGljX2Nhc3Q8UmVu ZGVySW1hZ2UgKj4obm9kZS0+cmVuZGVyZXIoKSk7CiAgICAgICAgICAgICBpZiAoIXItPmlzRGlz cGxheWluZ0Vycm9yKCkpIHsKLSAgICAgICAgICAgICAgICBRUGl4bWFwIHAgPSByLT5waXhtYXAo KTsKKyAgICAgICAgICAgICAgICBjb25zdCBRUGl4bWFwJiBwID0gci0+cGl4bWFwKCk7CiAgICAg ICAgICAgICAgICAgaWYgKHAuaW1hZ2VSZW5kZXJlcigpKQogICAgICAgICAgICAgICAgICAgICBb ZWxlbWVudCBzZXRPYmplY3Q6cC5pbWFnZVJlbmRlcmVyKCkgZm9yS2V5OldlYkNvcmVFbGVtZW50 SW1hZ2VSZW5kZXJlcktleV07CiAgICAgICAgICAgICB9Cg==