Bug 195228 – gPictureOwnerMap is unnecessary

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

We can just store it in HTMLImageElement.

[Ryosuke Niwa]

Created attachment 363373 [details]
Fixes the bug

[Daniel Bates]

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.

> 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.

[Ryosuke Niwa]

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.
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.

[Ryosuke Niwa]

Committed r242391: <https://trac.webkit.org/changeset/242391>

[Radar WebKit Bug Importer]

<rdar://problem/48576577>

[Daniel Bates]

(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

[Ryosuke Niwa]

(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.