Bug 195228 - gPictureOwnerMap is unnecessary
Summary: gPictureOwnerMap is unnecessary
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-01 14:04 PST by Ryosuke Niwa
Modified: 2019-03-04 19:26 PST (History)
9 users (show)

See Also:


Attachments
Fixes the bug (2.93 KB, patch)
2019-03-01 14:09 PST, Ryosuke Niwa
zalan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2019-03-01 14:04:30 PST
We can just store it in HTMLImageElement.
Comment 1 Ryosuke Niwa 2019-03-01 14:09:23 PST
Created attachment 363373 [details]
Fixes the bug
Comment 2 Daniel Bates 2019-03-02 15:30:07 PST
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.
Comment 3 Ryosuke Niwa 2019-03-04 13:46:59 PST
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.
Comment 4 Ryosuke Niwa 2019-03-04 14:00:30 PST
Committed r242391: <https://trac.webkit.org/changeset/242391>
Comment 5 Radar WebKit Bug Importer 2019-03-04 14:14:04 PST
<rdar://problem/48576577>
Comment 6 Daniel Bates 2019-03-04 18:00:25 PST
(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
Comment 7 Ryosuke Niwa 2019-03-04 19:26:21 PST
(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.