195228 2019-03-01 14:04:30 -0800 gPictureOwnerMap is unnecessary 2019-03-04 19:26:21 -0800 1 1 1 Unclassified WebKit DOM WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 rniwa rniwa cdumez dbates dino esprehn+autocc ews-watchlist gyuyoung.kim simon.fraser webkit-bug-importer zalan oldest_to_newest 1511605 0 rniwa 2019-03-01 14:04:30 -0800 We can just store it in HTMLImageElement. 1511606 1 363373 rniwa 2019-03-01 14:09:23 -0800 Created attachment 363373 Fixes the bug 1511865 2 363373 dbates 2019-03-02 15:30:07 -0800 Comment on attachment 363373 Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=363373&action=review > Source/WebCore/ChangeLog:8 > + Just store in HTMLImageElement. An extra pointer isn't going to affect the memory use here. I’m leaning towards you’re right, but I just can’t shake my intuition that images are the most used subresource on the internet today (now there is more than one way to load an image and I don’t know the breakdown in usage for that). This thought prevents me from reviewing this patch without seeing some numbers or reassurance that you actually thought this through because you’re increasing the size of each <img> by 8 bytes. Yes, I know there is overhead for the HashMap, but we pay it once no matter how many images a page has and, for now, how many images in loaded in subframes. > Source/WebCore/ChangeLog:9 > + If anything, we should worry about m_editableImage and m_pendingClonedAttachmentID instead. Why? What is so different about these things? At least the latter is conditionally compiled in so only platforms that enable it pay the cost. 1512348 3 363373 rniwa 2019-03-04 13:46:59 -0800 Comment on attachment 363373 Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=363373&action=review >> Source/WebCore/ChangeLog:8 >> + Just store in HTMLImageElement. An extra pointer isn't going to affect the memory use here. > > I’m leaning towards you’re right, but I just can’t shake my intuition that images are the most used subresource on the internet today (now there is more than one way to load an image and I don’t know the breakdown in usage for that). This thought prevents me from reviewing this patch without seeing some numbers or reassurance that you actually thought this through because you’re increasing the size of each <img> by 8 bytes. Yes, I know there is overhead for the HashMap, but we pay it once no matter how many images a page has and, for now, how many images in loaded in subframes. This is not something we need to get data to figure out. We know for a matter of fact, the most resource consuming thing about an image is its backing store, not the image element itself. In general, this kind of byte-by-byte counting we historically did turned out to be pretty silly exercise. In today's web, the most of memory is going to text nodes, JS heap, CA layers, and other more memory hungry objects. >> Source/WebCore/ChangeLog:9 >> + If anything, we should worry about m_editableImage and m_pendingClonedAttachmentID instead. > > Why? What is so different about these things? At least the latter is conditionally compiled in so only platforms that enable it pay the cost. m_editableImage is definitely compiled on all platforms now, and nobody really uses it right now. It's definitely not used in Safari and most of other web browsers. 1512355 4 rniwa 2019-03-04 14:00:30 -0800 Committed r242391: <https://trac.webkit.org/changeset/242391> 1512358 5 webkit-bug-importer 2019-03-04 14:14:04 -0800 <rdar://problem/48576577> 1512438 6 dbates 2019-03-04 18:00:25 -0800 (In reply to Ryosuke Niwa from comment #3) > Comment on attachment 363373 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363373&action=review > > >> Source/WebCore/ChangeLog:8 > >> + Just store in HTMLImageElement. An extra pointer isn't going to affect the memory use here. > > > > I’m leaning towards you’re right, but I just can’t shake my intuition that images are the most used subresource on the internet today (now there is more than one way to load an image and I don’t know the breakdown in usage for that). This thought prevents me from reviewing this patch without seeing some numbers or reassurance that you actually thought this through because you’re increasing the size of each <img> by 8 bytes. Yes, I know there is overhead for the HashMap, but we pay it once no matter how many images a page has and, for now, how many images in loaded in subframes. > > This is not something we need to get data to figure out. > We know for a matter of fact, the most resource consuming thing about an > image is its backing store, not the image element itself. This is vacuously true (how many 8 byte images do you encounter now a days? Maybe still 1x1 spacer images? I’m not even talking about uncompressed, yet) and does not help me better understand when we should micro-optimize memory size and when not to. > In general, this kind of byte-by-byte counting we historically did turned > out to be pretty silly exercise. I’m going remember this quote. > In today's web, the most of memory is going to text nodes, JS heap, CA > layers, and other more memory hungry objects. > > >> Source/WebCore/ChangeLog:9 > >> + If anything, we should worry about m_editableImage and m_pendingClonedAttachmentID instead. > > > > Why? What is so different about these things? At least the latter is conditionally compiled in so only platforms that enable it pay the cost. > > m_editableImage is definitely compiled on all platforms now, and nobody > really uses it right now. > It's definitely not used in Safari and most of other web browsers. m_editableImage was the least important bit in that sentence 1512460 7 rniwa 2019-03-04 19:26:21 -0800 (In reply to Daniel Bates from comment #6) > (In reply to Ryosuke Niwa from comment #3) > > Comment on attachment 363373 [details] > > Fixes the bug > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=363373&action=review > > > > >> Source/WebCore/ChangeLog:8 > > >> + Just store in HTMLImageElement. An extra pointer isn't going to affect the memory use here. > > > > > > I’m leaning towards you’re right, but I just can’t shake my intuition that images are the most used subresource on the internet today (now there is more than one way to load an image and I don’t know the breakdown in usage for that). This thought prevents me from reviewing this patch without seeing some numbers or reassurance that you actually thought this through because you’re increasing the size of each <img> by 8 bytes. Yes, I know there is overhead for the HashMap, but we pay it once no matter how many images a page has and, for now, how many images in loaded in subframes. > > > > This is not something we need to get data to figure out. > > We know for a matter of fact, the most resource consuming thing about an > > image is its backing store, not the image element itself. > > This is vacuously true (how many 8 byte images do you encounter now a days? > Maybe still 1x1 spacer images? I’m not even talking about uncompressed, yet) > and does not help me better understand when we should micro-optimize memory > size and when not to. 1x1 spacer images used to be popular but not anymore. I think we've over-optimized WebCore by thinking all these tiny increase matters. > > In today's web, the most of memory is going to text nodes, JS heap, CA > > layers, and other more memory hungry objects. > > > > >> Source/WebCore/ChangeLog:9 > > >> + If anything, we should worry about m_editableImage and m_pendingClonedAttachmentID instead. > > > > > > Why? What is so different about these things? At least the latter is conditionally compiled in so only platforms that enable it pay the cost. > > > > m_editableImage is definitely compiled on all platforms now, and nobody > > really uses it right now. > > It's definitely not used in Safari and most of other web browsers. > > m_editableImage was the least important bit in that sentence I don't get what your point is but my point stands that HTMLImageElement has other pointers that are way less frequently used than m_pictureElement. In particular, m_editableImage, is currently never used by any WebKit client. If we're worried about the memory usage of pointers in HTMLImageElement, we should be worrying about m_editableImage, not m_pictureElement. HTMLImageElement also has things like m_bestFitImageURL, m_currentSrc, m_parsedUsemap, m_form, m_formSetByParser, all of which are rarely used. Again, we should be worrying about these pointers if we're truly worried about the size of HTMLImageElement. 363373 2019-03-01 14:09:23 -0800 2019-03-04 13:55:51 -0800 Fixes the bug bug-195228-20190301140922.patch text/plain 3001 rniwa SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDI0MjI4NykKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE4IEBACisyMDE5LTAzLTAxICBSeW9zdWtl IE5pd2EgIDxybml3YUB3ZWJraXQub3JnPgorCisgICAgICAgIGdQaWN0dXJlT3duZXJNYXAgaXMg dW5uZWNlc3NhcnkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTE5NTIyOAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIEp1c3Qgc3RvcmUgaW4gSFRNTEltYWdlRWxlbWVudC4gQW4gZXh0cmEgcG9pbnRlciBpc24n dCBnb2luZyB0byBhZmZlY3QgdGhlIG1lbW9yeSB1c2UgaGVyZS4KKyAgICAgICAgSWYgYW55dGhp bmcsIHdlIHNob3VsZCB3b3JyeSBhYm91dCBtX2VkaXRhYmxlSW1hZ2UgYW5kIG1fcGVuZGluZ0Ns b25lZEF0dGFjaG1lbnRJRCBpbnN0ZWFkLgorCisgICAgICAgICogaHRtbC9IVE1MSW1hZ2VFbGVt ZW50LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkhUTUxJbWFnZUVsZW1lbnQ6OnBpY3R1cmVFbGVt ZW50IGNvbnN0KToKKyAgICAgICAgKFdlYkNvcmU6OkhUTUxJbWFnZUVsZW1lbnQ6OnNldFBpY3R1 cmVFbGVtZW50KToKKyAgICAgICAgKiBodG1sL0hUTUxJbWFnZUVsZW1lbnQuaDoKKwogMjAxOS0w My0wMSAgUm9iIEJ1aXMgIDxyYnVpc0BpZ2FsaWEuY29tPgogCiAgICAgICAgIEFkanVzdCBYTUxI dHRwUmVxdWVzdCBDb250ZW50LVR5cGUgaGFuZGxpbmcKSW5kZXg6IFNvdXJjZS9XZWJDb3JlL2h0 bWwvSFRNTEltYWdlRWxlbWVudC5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvaHRt bC9IVE1MSW1hZ2VFbGVtZW50LmNwcAkocmV2aXNpb24gMjQyMjc2KQorKysgU291cmNlL1dlYkNv cmUvaHRtbC9IVE1MSW1hZ2VFbGVtZW50LmNwcAkod29ya2luZyBjb3B5KQpAQCAtNjcsOSArNjcs NiBAQCBXVEZfTUFLRV9JU09fQUxMT0NBVEVEX0lNUEwoSFRNTEltYWdlRWxlCiAKIHVzaW5nIG5h bWVzcGFjZSBIVE1MTmFtZXM7CiAKLXR5cGVkZWYgSGFzaE1hcDxjb25zdCBIVE1MSW1hZ2VFbGVt ZW50KiwgV2Vha1B0cjxIVE1MUGljdHVyZUVsZW1lbnQ+PiBQaWN0dXJlT3duZXJNYXA7Ci1zdGF0 aWMgUGljdHVyZU93bmVyTWFwKiBnUGljdHVyZU93bmVyTWFwID0gbnVsbHB0cjsKLQogSFRNTElt YWdlRWxlbWVudDo6SFRNTEltYWdlRWxlbWVudChjb25zdCBRdWFsaWZpZWROYW1lJiB0YWdOYW1l LCBEb2N1bWVudCYgZG9jdW1lbnQsIEhUTUxGb3JtRWxlbWVudCogZm9ybSkKICAgICA6IEhUTUxF bGVtZW50KHRhZ05hbWUsIGRvY3VtZW50KQogICAgICwgbV9pbWFnZUxvYWRlcigqdGhpcykKQEAg LTQ1NCwyNSArNDUxLDEyIEBAIHZvaWQgSFRNTEltYWdlRWxlbWVudDo6dXBkYXRlRWRpdGFibGVJ bWEKIAogSFRNTFBpY3R1cmVFbGVtZW50KiBIVE1MSW1hZ2VFbGVtZW50OjpwaWN0dXJlRWxlbWVu dCgpIGNvbnN0CiB7Ci0gICAgaWYgKCFnUGljdHVyZU93bmVyTWFwIHx8ICFnUGljdHVyZU93bmVy TWFwLT5jb250YWlucyh0aGlzKSkKLSAgICAgICAgcmV0dXJuIG51bGxwdHI7Ci0gICAgYXV0byBy ZXN1bHQgPSBnUGljdHVyZU93bmVyTWFwLT5nZXQodGhpcyk7Ci0gICAgaWYgKCFyZXN1bHQpCi0g ICAgICAgIGdQaWN0dXJlT3duZXJNYXAtPnJlbW92ZSh0aGlzKTsKLSAgICByZXR1cm4gcmVzdWx0 LmdldCgpOworICAgIHJldHVybiBtX3BpY3R1cmVFbGVtZW50LmdldCgpOwogfQogICAgIAogdm9p ZCBIVE1MSW1hZ2VFbGVtZW50OjpzZXRQaWN0dXJlRWxlbWVudChIVE1MUGljdHVyZUVsZW1lbnQq IHBpY3R1cmVFbGVtZW50KQogewotICAgIGlmICghcGljdHVyZUVsZW1lbnQpIHsKLSAgICAgICAg aWYgKGdQaWN0dXJlT3duZXJNYXApCi0gICAgICAgICAgICBnUGljdHVyZU93bmVyTWFwLT5yZW1v dmUodGhpcyk7Ci0gICAgICAgIHJldHVybjsKLSAgICB9Ci0gICAgCi0gICAgaWYgKCFnUGljdHVy ZU93bmVyTWFwKQotICAgICAgICBnUGljdHVyZU93bmVyTWFwID0gbmV3IFBpY3R1cmVPd25lck1h cCgpOwotICAgIGdQaWN0dXJlT3duZXJNYXAtPmFkZCh0aGlzLCBtYWtlV2Vha1B0cigqcGljdHVy ZUVsZW1lbnQpKTsKKyAgICBtX3BpY3R1cmVFbGVtZW50ID0gbWFrZVdlYWtQdHIocGljdHVyZUVs ZW1lbnQpOwogfQogICAgIAogdW5zaWduZWQgSFRNTEltYWdlRWxlbWVudDo6d2lkdGgoYm9vbCBp Z25vcmVQZW5kaW5nU3R5bGVzaGVldHMpCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxJ bWFnZUVsZW1lbnQuaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxJbWFn ZUVsZW1lbnQuaAkocmV2aXNpb24gMjQyMjc2KQorKysgU291cmNlL1dlYkNvcmUvaHRtbC9IVE1M SW1hZ2VFbGVtZW50LmgJKHdvcmtpbmcgY29weSkKQEAgLTE4NSw2ICsxODUsOCBAQCBwcml2YXRl OgogICAgIGJvb2wgbV9oYWROYW1lQmVmb3JlQXR0cmlidXRlQ2hhbmdlZCB7IGZhbHNlIH07IC8v IEZJWE1FOiBXZSBvbmx5IG5lZWQgdGhpcyBiZWNhdXNlIHBhcnNlQXR0cmlidXRlKCkgY2FuJ3Qg c2VlIHRoZSBvbGQgdmFsdWUuCiAKICAgICBSZWZQdHI8RWRpdGFibGVJbWFnZVJlZmVyZW5jZT4g bV9lZGl0YWJsZUltYWdlOworICAgIFdlYWtQdHI8SFRNTFBpY3R1cmVFbGVtZW50PiBtX3BpY3R1 cmVFbGVtZW50OworCiAjaWYgRU5BQkxFKEFUVEFDSE1FTlRfRUxFTUVOVCkKICAgICBTdHJpbmcg bV9wZW5kaW5nQ2xvbmVkQXR0YWNobWVudElEOwogI2VuZGlmCg==