Remove duplication of encoded image data

Remove duplication of encoded image data

Now there are at least two encoded image data: one is in Image/ImageResource as
SharedBuffer and the other is in DeferredImageDecoder as SkRWBuffer. This CL
removes former when possible (non-icon bitmaps), and generate the SharedBuffer
from the SkRWBuffer if needed.

Design Doc: https://docs.google.com/document/d/1v0yTAZ6wkqX2U_M6BNIGUJpM1s0TIw1VsqpxoL7aciY/edit?usp=sharing

BUG=618623
TEST=blink_platform_unittests --gtest_filter=BitmapImageTest.*:ImageDecoderTest.*

Committed: https://crrev.com/36f4eb81d4a791a8e3dfea4ba2418465d30bcc90
Cr-Commit-Position: refs/heads/master@{#406238}

[hajimehoshi, 2016-06-09 09:36:08]

Description was changed from

==========
WIP: Remove duplication of encoded image data

BUG=
==========

to

==========
WIP: Remove duplication of encoded image data

BUG=
TEST=blink_platform_unittests
--gtest_filter=BitmapImageTest.*:ImageDecoderTest.*
==========

[hajimehoshi, 2016-06-09 09:42:44]

Description was changed from

==========
WIP: Remove duplication of encoded image data

BUG=
TEST=blink_platform_unittests
--gtest_filter=BitmapImageTest.*:ImageDecoderTest.*
==========

to

==========
WIP: Remove duplication of encoded image data

BUG=618623
TEST=blink_platform_unittests
--gtest_filter=BitmapImageTest.*:ImageDecoderTest.*
==========

[hajimehoshi, 2016-06-09 11:46:10]

Description was changed from

==========
WIP: Remove duplication of encoded image data

BUG=618623
TEST=blink_platform_unittests
--gtest_filter=BitmapImageTest.*:ImageDecoderTest.*
==========

to

==========
Remove duplication of encoded image data

Now there are at least two encoded image data: one is in Image/ImageResource as
SharedBuffer and the other is in DeferredImageDecoder as SkRWBuffer. This CL
removes former when possible (non-icon bitmaps), and generate the SharedBuffer
from the SkRWBuffer if needed.

BUG=618623
TEST=blink_platform_unittests
--gtest_filter=BitmapImageTest.*:ImageDecoderTest.*
==========

[hajimehoshi, 2016-06-09 11:56:35]

Description was changed from

==========
Remove duplication of encoded image data

Now there are at least two encoded image data: one is in Image/ImageResource as
SharedBuffer and the other is in DeferredImageDecoder as SkRWBuffer. This CL
removes former when possible (non-icon bitmaps), and generate the SharedBuffer
from the SkRWBuffer if needed.

BUG=618623
TEST=blink_platform_unittests
--gtest_filter=BitmapImageTest.*:ImageDecoderTest.*
==========

to

==========
Remove duplication of encoded image data

Now there are at least two encoded image data: one is in Image/ImageResource as
SharedBuffer and the other is in DeferredImageDecoder as SkRWBuffer. This CL
removes former when possible (non-icon bitmaps), and generate the SharedBuffer
from the SkRWBuffer if needed.

TODO: Deref m_data in ImageResource

BUG=618623
TEST=blink_platform_unittests
--gtest_filter=BitmapImageTest.*:ImageDecoderTest.*
==========

[hajimehoshi, 2016-06-09 11:59:15]

Description was changed from

==========
Remove duplication of encoded image data

Now there are at least two encoded image data: one is in Image/ImageResource as
SharedBuffer and the other is in DeferredImageDecoder as SkRWBuffer. This CL
removes former when possible (non-icon bitmaps), and generate the SharedBuffer
from the SkRWBuffer if needed.

TODO: Deref m_data in ImageResource

BUG=618623
TEST=blink_platform_unittests
--gtest_filter=BitmapImageTest.*:ImageDecoderTest.*
==========

to

==========
Remove duplication of encoded image data

Now there are at least two encoded image data: one is in Image/ImageResource as
SharedBuffer and the other is in DeferredImageDecoder as SkRWBuffer. This CL
removes former when possible (non-icon bitmaps), and generate the SharedBuffer
from the SkRWBuffer if needed.

TODO: Deref m_data in ImageResource
TODO: Check ImageResource::isSafeToUnlock

BUG=618623
TEST=blink_platform_unittests
--gtest_filter=BitmapImageTest.*:ImageDecoderTest.*
==========

[hajimehoshi, 2016-06-10 13:18:12]

Description was changed from

==========
Remove duplication of encoded image data

Now there are at least two encoded image data: one is in Image/ImageResource as
SharedBuffer and the other is in DeferredImageDecoder as SkRWBuffer. This CL
removes former when possible (non-icon bitmaps), and generate the SharedBuffer
from the SkRWBuffer if needed.

TODO: Deref m_data in ImageResource
TODO: Check ImageResource::isSafeToUnlock

BUG=618623
TEST=blink_platform_unittests
--gtest_filter=BitmapImageTest.*:ImageDecoderTest.*
==========

to

==========
Remove duplication of encoded image data

Now there are at least two encoded image data: one is in Image/ImageResource as
SharedBuffer and the other is in DeferredImageDecoder as SkRWBuffer. This CL
removes former when possible (non-icon bitmaps), and generate the SharedBuffer
from the SkRWBuffer if needed.

BUG=618623
TEST=blink_platform_unittests
--gtest_filter=BitmapImageTest.*:ImageDecoderTest.*
==========

[hajimehoshi, 2016-06-10 13:22:47]

hajimehoshi@chromium.org changed reviewers:
+ scroggo@chromium.org, tasak@google.com, yhirano@chromium.org

[hajimehoshi, 2016-06-10 13:24:13]

hajimehoshi@chromium.org changed reviewers:
+ haraken@chromium.org
- yhirano@chromium.org

[hajimehoshi, 2016-06-10 13:25:37]

hajimehoshi@chromium.org changed reviewers:
+ fmalita@chromium.org

[hajimehoshi, 2016-06-10 13:30:28]

PTAL

https://codereview.chromium.org/2054643003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (left):

https://codereview.chromium.org/2054643003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:130: 
I think it is safe to remove since this was introduced with purgable buffer, and
now an Image holds the encode image data just temporarily and can generate
encoded image data from SkRWBuffer on the fly.

[f(malita), 2016-06-10 14:26:43]

https://codereview.chromium.org/2054643003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2054643003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:389:
updateImageAndClearBuffer();
My understanding of the current impl is that image resources (resources in
general) are cached in discardable memory for reuse.

What are the caching/data sharing implications of this change for

a) multiple image instances with the same source

b) same-source images with non-overlapping lifespans - say across page reload or
navigation.

If I understand this CL correctly, we're now moving the data to the image
decoder at the expense of the resource cache.

[hajimehoshi, 2016-06-13 09:10:47]

Thank you!

https://codereview.chromium.org/2054643003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2054643003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:389:
updateImageAndClearBuffer();
On 2016/06/10 14:26:43, f(malita) wrote:
> My understanding of the current impl is that image resources (resources in
> general) are cached in discardable memory for reuse.
> 
> What are the caching/data sharing implications of this change for
> 
> a) multiple image instances with the same source

I think this doesn't happen with this CL.

> 
> b) same-source images with non-overlapping lifespans - say across page reload
or
> navigation.

ImageResource has an blink::Image as a RefPtr and the Image should be owned by
another object in this case.

> 
> If I understand this CL correctly, we're now moving the data to the image
> decoder at the expense of the resource cache.

Did you mean the resource cache is ResourceFetcher::cachedResource
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Res...
?

I think this CL doesn't affect the cache resource management. So, there is no
performance regression.

[hajimehoshi, 2016-06-13 09:12:03]

hajimehoshi@chromium.org changed reviewers:
+ hiroshige@chromium.org

[hajimehoshi, 2016-06-13 09:12:42]

+hiroshige

[f(malita), 2016-06-13 10:32:58]

On 2016/06/13 09:10:47, hajimehoshi wrote:
> Did you mean the resource cache is ResourceFetcher::cachedResource
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Res...
> ?
> 
> I think this CL doesn't affect the cache resource management. So, there is no
> performance regression.

Yeah, I'm thinking of MemoryCache and ResourceFetcher interactions: AFAIU we
cache (Image)Resources in MemoryCache, and reuse them when possible (to allow
reloads).  E.g. ResourceFetcher::requestResource() goes through that cache.

With this change, it seems that we might clear the data buffer of cached
resources.

Try this with the CL applied:

1) open www.google.com
2) reload the page

The logo disappears and I'm getting the broken image icon.  Reloading repeatedly
alternates between showing the correct logo and the broken image icon.

I'm only vaguely familiar with resource loading/caching, I think an expert
should definitely take a look at this.

[f(malita), 2016-06-13 10:34:52]

On 2016/06/13 10:32:58, f(malita) wrote:
> Yeah, I'm thinking of MemoryCache and ResourceFetcher interactions: AFAIU we
> cache (Image)Resources in MemoryCache, and reuse them when possible (to allow
> reloads).

*to avoid reloads

[hajimehoshi, 2016-06-13 12:10:10]

On 2016/06/13 10:32:58, f(malita) wrote:
> On 2016/06/13 09:10:47, hajimehoshi wrote:
> > Did you mean the resource cache is ResourceFetcher::cachedResource
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Res...
> > ?
> > 
> > I think this CL doesn't affect the cache resource management. So, there is
no
> > performance regression.
> 
> Yeah, I'm thinking of MemoryCache and ResourceFetcher interactions: AFAIU we
> cache (Image)Resources in MemoryCache, and reuse them when possible (to allow
> reloads).  E.g. ResourceFetcher::requestResource() goes through that cache.
> 
> With this change, it seems that we might clear the data buffer of cached
> resources.
> 
> Try this with the CL applied:
> 
> 1) open http://www.google.com
> 2) reload the page
> 
> The logo disappears and I'm getting the broken image icon.  Reloading
repeatedly
> alternates between showing the correct logo and the broken image icon.
> 
> I'm only vaguely familiar with resource loading/caching, I think an expert
> should definitely take a look at this.

Ugh, I've confirmed the logo disappeared as you pointed out. Thank you. This is
unexpected and I'll investigate what's going on.

[hajimehoshi, 2016-06-15 10:55:57]

On 2016/06/13 12:10:10, hajimehoshi wrote:
> On 2016/06/13 10:32:58, f(malita) wrote:
> > On 2016/06/13 09:10:47, hajimehoshi wrote:
> > > Did you mean the resource cache is ResourceFetcher::cachedResource
> > >
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Res...
> > > ?
> > > 
> > > I think this CL doesn't affect the cache resource management. So, there is
> no
> > > performance regression.
> > 
> > Yeah, I'm thinking of MemoryCache and ResourceFetcher interactions: AFAIU we
> > cache (Image)Resources in MemoryCache, and reuse them when possible (to
allow
> > reloads).  E.g. ResourceFetcher::requestResource() goes through that cache.
> > 
> > With this change, it seems that we might clear the data buffer of cached
> > resources.
> > 
> > Try this with the CL applied:
> > 
> > 1) open http://www.google.com
> > 2) reload the page
> > 
> > The logo disappears and I'm getting the broken image icon.  Reloading
> repeatedly
> > alternates between showing the correct logo and the broken image icon.
> > 
> > I'm only vaguely familiar with resource loading/caching, I think an expert
> > should definitely take a look at this.
> 
> Ugh, I've confirmed the logo disappeared as you pointed out. Thank you. This
is
> unexpected and I'll investigate what's going on.

Fixed. PTAL

[hiroshige, 2016-06-15 13:16:26]

I think ImageResource::destroyDecodedDataIfPossible() should also be modified;
currently, it can clear |m_image| as decoded data. ImageResource will re-create
|m_image| and call |setData(m_data, ...)| if needed after |m_image| is cleared.
However, this CL clears |m_data|, so we cannot do the same thing.
This might cause test failures on bots (e.g. Web GL conformance tests which
sometimes catch Resource/MemoryCache-related bugs).

Another suspicious thing is InspectorPageAgent::cachedResourceContent().
It obtains data by Resource::resourceBuffer() i.e. |Resource::m_data|, so
Inspector might be unable to obtain data  of ImageResource after this CL.
Perhaps we have to modify InspectorPageAgent::cachedResourceContent()?

https://codereview.chromium.org/2054643003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2054643003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:354: sizeAvailable =
m_image->setData(m_data, allDataReceived);
Isn't it problematic if |setData(m_data)| is called here after we clear |m_data|
in finish()?

[hiroshige, 2016-06-15 13:27:01]

https://codereview.chromium.org/2054643003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (left):

https://codereview.chromium.org/2054643003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:130: 
On 2016/06/10 13:30:28, hajimehoshi wrote:
> I think it is safe to remove since this was introduced with purgable buffer,
and
> now an Image holds the encode image data just temporarily and can generate
> encoded image data from SkRWBuffer on the fly.

So can we add
|CHECK(!m_data || m_image)|
here?

[hajimehoshi, 2016-06-16 09:43:20]

https://codereview.chromium.org/2054643003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2054643003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:354: sizeAvailable =
m_image->setData(m_data, allDataReceived);
On 2016/06/15 13:16:25, hiroshige (slow) wrote:
> Isn't it problematic if |setData(m_data)| is called here after we clear
|m_data|
> in finish()?

Actually |m_data| is null here after m_data.clear() at L390. In this case, is
encoded image data in |m_image| different from |m_data|? If so, why should we
call |setData| here?

[hajimehoshi, 2016-06-16 09:47:41]

https://codereview.chromium.org/2054643003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2054643003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:354: sizeAvailable =
m_image->setData(m_data, allDataReceived);
On 2016/06/16 09:43:20, hajimehoshi wrote:
> On 2016/06/15 13:16:25, hiroshige (slow) wrote:
> > Isn't it problematic if |setData(m_data)| is called here after we clear
> |m_data|
> > in finish()?
> 
> Actually |m_data| is null here after m_data.clear() at L390. In this case, is
> encoded image data in |m_image| different from |m_data|? If so, why should we
> call |setData| here?

s/In this case,/Aside from |m_data.clear()|,/

[hajimehoshi, 2016-06-16 12:11:42]

On 2016/06/15 13:16:26, hiroshige (slow) wrote:
> I think ImageResource::destroyDecodedDataIfPossible() should also be modified;
> currently, it can clear |m_image| as decoded data. ImageResource will
re-create
> |m_image| and call |setData(m_data, ...)| if needed after |m_image| is
cleared.
> However, this CL clears |m_data|, so we cannot do the same thing.
> This might cause test failures on bots (e.g. Web GL conformance tests which
> sometimes catch Resource/MemoryCache-related bugs).

Hmm, I still don't understand why setData is called with encoded data after
purging decoded image. Is it assumed that Image might not have encoded image
data?

> Another suspicious thing is InspectorPageAgent::cachedResourceContent().
> It obtains data by Resource::resourceBuffer() i.e. |Resource::m_data|, so
> Inspector might be unable to obtain data  of ImageResource after this CL.
> Perhaps we have to modify InspectorPageAgent::cachedResourceContent()?

I think this can be fixed by make Resource::resourceBuffer() virtual and
override it for ImageResource to return m_image->data().

[hajimehoshi, 2016-06-17 09:25:15]

PTAL

https://codereview.chromium.org/2054643003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (left):

https://codereview.chromium.org/2054643003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:130: 
On 2016/06/15 13:27:01, hiroshige (slow) wrote:
> On 2016/06/10 13:30:28, hajimehoshi wrote:
> > I think it is safe to remove since this was introduced with purgable buffer,
> and
> > now an Image holds the encode image data just temporarily and can generate
> > encoded image data from SkRWBuffer on the fly.
> 
> So can we add
> |CHECK(!m_data || m_image)|
> here?

Done.

https://codereview.chromium.org/2054643003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2054643003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:354: sizeAvailable =
m_image->setData(m_data, allDataReceived);
TBH I'm not confident this is OK. Let me check with DCHECK and bot tests.

[scroggo_chromium, 2016-06-20 19:00:44]

https://codereview.chromium.org/2054643003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2054643003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:194: if
(!hasClientsOrObservers() && !isLoading() && m_image && (m_image->hasOneRef() &&
m_image->isBitmapImage())) {
Are the parentheses around

  m_image->hasOneRef() && m_image->isBitmapImage()

still necessary? It looks like you just have &&'s, so the order should not
matter?

https://codereview.chromium.org/2054643003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:197:
m_image->destroyDecodedData();
This else statement appears to do the exact same thing as the if case. Should
this be one giant if?

https://codereview.chromium.org/2054643003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:512: if (isLoading()) {
nit: My understanding of the style guide is that this brace should not be here.

https://codereview.chromium.org/2054643003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.h (right):

https://codereview.chromium.org/2054643003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:129: SharedBuffer*
cachedData();
Why does this return a raw pointer?

It calls a method that returns a PassRefPtr, and one of the callers then wraps
it back into a PassRefPtr. The other caller presumably doesn't care whether it's
a raw pointer or a PassRefPtr, but it seems like there's no problem with it
being a PassRefPtr?

https://codereview.chromium.org/2054643003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/2054643003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:125:
PassRefPtr<SharedBuffer> DeferredImageDecoder::data()
This method can be slow, especially if it's called regularly. Who calls it?

It looks like the stack trace would be:

ImageResource::resourceBuffer()
BitmapImage::data() (overriding Image::data())
ImageSource::data()

Does this get called often?

https://codereview.chromium.org/2054643003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ImageSource.cpp (right):

https://codereview.chromium.org/2054643003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageSource.cpp:56: return
m_decoder->data();
Shouldn't you check to make sure m_decoder is not NULL first? (Or is it required
that hasData() has already been called?)

[hajimehoshi, 2016-06-22 09:42:30]

Thank you!

https://codereview.chromium.org/2054643003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (left):

https://codereview.chromium.org/2054643003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:358: if (m_image)
Found that there is a case |m_data| is null (ImageResource::finish now clears
|m_data| as of this CL). I've changed the condition.

https://codereview.chromium.org/2054643003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2054643003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:194: if
(!hasClientsOrObservers() && !isLoading() && m_image && (m_image->hasOneRef() &&
m_image->isBitmapImage())) {
On 2016/06/20 19:00:43, scroggo_chromium wrote:
> Are the parentheses around
> 
>   m_image->hasOneRef() && m_image->isBitmapImage()
> 
> still necessary? It looks like you just have &&'s, so the order should not
> matter?

Done.

https://codereview.chromium.org/2054643003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:197:
m_image->destroyDecodedData();
On 2016/06/20 19:00:43, scroggo_chromium wrote:
> This else statement appears to do the exact same thing as the if case. Should
> this be one giant if?

Done.

https://codereview.chromium.org/2054643003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:512: if (isLoading()) {
On 2016/06/20 19:00:43, scroggo_chromium wrote:
> nit: My understanding of the style guide is that this brace should not be
here.

Done.

https://codereview.chromium.org/2054643003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/BitmapImage.h (right):

https://codereview.chromium.org/2054643003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/BitmapImage.h:129: SharedBuffer*
cachedData();
On 2016/06/20 19:00:43, scroggo_chromium wrote:
> Why does this return a raw pointer?
> 
> It calls a method that returns a PassRefPtr, and one of the callers then wraps
> it back into a PassRefPtr. The other caller presumably doesn't care whether
it's
> a raw pointer or a PassRefPtr, but it seems like there's no problem with it
> being a PassRefPtr?

Done.

https://codereview.chromium.org/2054643003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/2054643003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:125:
PassRefPtr<SharedBuffer> DeferredImageDecoder::data()
On 2016/06/20 19:00:43, scroggo_chromium wrote:
> This method can be slow, especially if it's called regularly. Who calls it?
> 
> It looks like the stack trace would be:
> 
> ImageResource::resourceBuffer()
> BitmapImage::data() (overriding Image::data())
> ImageSource::data()
> 
> Does this get called often?

IIUC this is not called often: Resource::resourceBuffer() is called from
FrameSerializer, the inspector, tests, etc. The situation is limited.

https://codereview.chromium.org/2054643003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/ImageSource.cpp (right):

https://codereview.chromium.org/2054643003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/ImageSource.cpp:56: return
m_decoder->data();
On 2016/06/20 19:00:43, scroggo_chromium wrote:
> Shouldn't you check to make sure m_decoder is not NULL first? (Or is it
required
> that hasData() has already been called?)

Done.

[hajimehoshi, 2016-06-22 11:59:09]

Patchset #8 (id:140001) has been deleted

[scroggo_chromium, 2016-06-22 19:22:49]

lgtm

[haraken, 2016-06-23 00:59:56]

Exciting!

tasak@: Would you measure how much this CL reduces renderer's memory?

[haraken, 2016-06-23 01:26:41]

https://codereview.chromium.org/2054643003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2054643003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:190:
m_image->destroyDecodedData();

Honestly speaking, I don't fully understand changes in this file.

Would you help me understand how m_data, m_image, setDecodedSize and
destroyDecodedData are going to be handled after this CL?

https://codereview.chromium.org/2054643003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/2054643003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:123: if
(PassRefPtr<SharedBuffer> data = cachedData())

PassRefPtr<SharedBuffer> => RefPtr<SharedBuffer>

https://codereview.chromium.org/2054643003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:236:
resetEncodedImageData();

Add a comment and explain what this is doing and why it's important.

https://codereview.chromium.org/2054643003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:237:
DCHECK(!cachedData());

Would it be clearer to rename cachedData to encodedImageData?

https://codereview.chromium.org/2054643003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Image.h (right):

https://codereview.chromium.org/2054643003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Image.h:111: virtual
PassRefPtr<SharedBuffer> data() { return m_encodedImageData; }

Is ScriptStreamer the only reason you want to change the return type to
PassRefPtr? If so, I'd prefer just keeping the old code as is, because it is
important to make sure that Image is the *only* owner of m_encodedImageData. If
the ownership is passed to other callers, we may not be able to release the
underlying SharedBuffer when resetEncodedImageData gets called.

[hajimehoshi, 2016-06-23 09:23:13]

Thank you!

Thanks to hiroshige-san, we found that this CL needs to consider SVGImage since
ImageResouce::m_data's lifetime was changed. I'll work on that soon.

https://codereview.chromium.org/2054643003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2054643003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:190:
m_image->destroyDecodedData();
On 2016/06/23 01:26:40, haraken wrote:
> 
> Honestly speaking, I don't fully understand changes in this file.
> 
> Would you help me understand how m_data, m_image, setDecodedSize and
> destroyDecodedData are going to be handled after this CL?

Before this CL, m_data is a primary data and m_image can always be purged, but
after this CL, m_image is basically never purged and m_data is purged soon after
m_image is completed. That's why destroyDecodedDataForFailedRevalidation now
doesn't clear m_image. IIUC setDecodedSize is synched with m_image and now that
m_image is not cleared, we don't need to call setDecodedSize(0) here.

https://codereview.chromium.org/2054643003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/2054643003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:123: if
(PassRefPtr<SharedBuffer> data = cachedData())
On 2016/06/23 01:26:41, haraken wrote:
> 
> PassRefPtr<SharedBuffer> => RefPtr<SharedBuffer>

Done.

https://codereview.chromium.org/2054643003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:236:
resetEncodedImageData();
On 2016/06/23 01:26:40, haraken wrote:
> 
> Add a comment and explain what this is doing and why it's important.
> 

Done.

https://codereview.chromium.org/2054643003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:237:
DCHECK(!cachedData());
On 2016/06/23 01:26:40, haraken wrote:
> 
> Would it be clearer to rename cachedData to encodedImageData?

Hmm, I want to emphasize BitmapImage no longer has a cached encoded image data.
encodedImageData might be confusing. BTW resetEncodedImageData is also confusing
as the same reason, so I'll rename this.

https://codereview.chromium.org/2054643003/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Image.h (right):

https://codereview.chromium.org/2054643003/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Image.h:111: virtual
PassRefPtr<SharedBuffer> data() { return m_encodedImageData; }
On 2016/06/23 01:26:41, haraken wrote:
> 
> Is ScriptStreamer the only reason you want to change the return type to
> PassRefPtr? If so, I'd prefer just keeping the old code as is, because it is
> important to make sure that Image is the *only* owner of m_encodedImageData.
If
> the ownership is passed to other callers, we may not be able to release the
> underlying SharedBuffer when resetEncodedImageData gets called.
> 
> 

This change is needed since the child class BitmapImage requires this change:
After this CL, a BitmapImage might return a SharedBuffer which is not owned by a
BitmapImage.

[hajimehoshi, 2016-06-28 08:58:13]

I think all bugs detected by the build bots have been fixed. PTAL

[scroggo_chromium, 2016-06-28 15:33:44]

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (left):

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:357:
m_image->clearImageObserver();
Why is it okay to drop this line? My first thought was that we're about to
delete m_image, which would clear the observer anyway, but some other class
could hold a reference to m_image (e.g. by calling getImage() [1]), and I assume
we still want to clear the observer?

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Ima...

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:344: m_image = nullptr;
It's not obvious to me how this relates to the rest of the change, and I don't
know enough about a multi-part response to judge whether this makes sense.

One place where this seems wrong to me if the following is true:
1. No m_multipartParser
2. We have already appended some data (but not all), so m_image is not NULL, and
neither is m_data.
3. More data is appended in appendData()

This will call updateImage(false).
This method will clear m_image.

This means we'll have to recreate m_image from scratch, and previously we would
have reused the old one.

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:365: if (m_image &&
m_data)
If we have m_data, aren't we guaranteed to also have m_image, since
createImage() would have been called above?

Could you change this to:

if (m_data) {
  DCHECK(m_image);
  sizeAvailable = m_image->setData(m_data, allDataReceived);
}

?

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:128:
PassRefPtr<SharedBuffer> BitmapImage::cachedData()
A couple of thoughts on this method:

It is very simple, and we could almost inline it at the callsites, except that
you want to call it from a test.

I find the name confusing. It's not obvious to me which one is "cached" and
which one is not. I guess we call it cached because Image may throw away its
data?

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:237: // there is an
encoded image data in DefferedImageDecoder on a skia
nit: "there is encoded image data" (no need for "an" here)

nit: *DeferredImageDecoder (one 'f', two 'r's)

nit: I don't think "on a skia side" adds useful information. Maybe it's more
clear to say something like "in DeferredImageDecoder's SkRWBuffer"?

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:239:
resetCachedData();
Just to make sure I understand - this will drop the Image's reference to the
SharedBuffer (m_encodedImageData), but the actual SharedBuffer will only get
freed (thus saving memory) if the caller (e.g. ImageResource) also drops its
reference?

Is it possible the Image will drop its reference, resulting in having to copy
from the SkRWBuffer, in cases where the SharedBuffer is still around, so we
could have skipped copying from the SkRWBuffer without holding onto extra
memory? If so, would it be possible to only reset it if we know that that the
ImageResource *also* dropped its reference? (Or does this CL already do that and
I'm missing something?)

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Image.h (right):

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Image.h:175: void resetCachedData()
{ m_encodedImageData.release(); }
This method has an awkward name. It's a method on Image, which refers to its
data as m_encodedImageData. The name is only relevant here in BitmapImage, which
calls a method called cachedData to retrieve m_encodedImageData.

[hiroshige, 2016-06-29 07:04:06]

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (left):

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:332: clearImage();
Perhaps, we need to call clearImage() here, because clear() is called when an
error occurs. Clearing |m_image| in case of error looks reasonable for me.

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:189:
m_image->destroyDecodedData();
I think we have to clear |m_image| here, because we have to re-create |m_image|
in failed revalidation -- and it means we lack tests for that case. I'll add a
test.

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:196: if
((!hasClientsOrObservers() && !isLoading() && m_image->hasOneRef() &&
m_image->isBitmapImage()) || !errorOccurred())
I'm not sure, but can we simplify this condition to just |!errorOccurred()|?

The condition |!hasClientsOrObservers() && !isLoading() && (!m_image
||m_image->hasOneRef() && m_image->isBitmapImage()))| looks like for disposing
|m_image|, and this CL makes |m_image| to be not disposal.

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:344: m_image = nullptr;
On 2016/06/28 15:33:44, scroggo_chromium wrote:
> It's not obvious to me how this relates to the rest of the change, and I don't
> know enough about a multi-part response to judge whether this makes sense.
> 
> One place where this seems wrong to me if the following is true:
> 1. No m_multipartParser
> 2. We have already appended some data (but not all), so m_image is not NULL,
and
> neither is m_data.
> 3. More data is appended in appendData()
> 
> This will call updateImage(false).
> This method will clear m_image.
> 
> This means we'll have to recreate m_image from scratch, and previously we
would
> have reused the old one.

I think it's better to call clearImage() explicitly when a multipart image needs
to clear |m_image|, than clearing |m_image| here implicitly depending on
|m_image|. (Generally, implicit assumptions on the status of Resources are
frequently wrong...)

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:121:
PassRefPtr<SharedBuffer> BitmapImage::data()
The name data() here (and ImageSource) looks like just an accessor, but actually
it re-creates SharedBuffer if needed. How about renaming it to a name that looks
like more costly?

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:128:
PassRefPtr<SharedBuffer> BitmapImage::cachedData()
On 2016/06/28 15:33:44, scroggo_chromium wrote:
> A couple of thoughts on this method:
> 
> It is very simple, and we could almost inline it at the callsites, except that
> you want to call it from a test.
> 
> I find the name confusing. It's not obvious to me which one is "cached" and
> which one is not. I guess we call it cached because Image may throw away its
> data?
FYI using "cached" prefix near loading code might be confusing; There are
prefixes "cached" in loader code (e.g. |cachedImage|), but most of them are just
due to historical reasons (CachedImage is the old class name of ImageResource)
and don't have much meaning.

[hiroshige, 2016-06-29 07:07:29]

Probably we need a design doc that describe the lifetime of
- |Resource::m_data|,
- |ImageResource::m_image|, and
- |Image::m_encodedImageData|
and interactions with
- BitmapImage/SVGImage,
- Pruning,
- Revalidation, and
- Multipart image.

Yes, we lack the corresponding doc of the current state :-(, and I think this is
good chance to clarify these issues.

[haraken, 2016-06-29 07:29:32]

On 2016/06/29 07:07:29, hiroshige wrote:
> Probably we need a design doc that describe the lifetime of
> - |Resource::m_data|,
> - |ImageResource::m_image|, and
> - |Image::m_encodedImageData|
> and interactions with
> - BitmapImage/SVGImage,
> - Pruning,
> - Revalidation, and
> - Multipart image.
> 
> Yes, we lack the corresponding doc of the current state :-(, and I think this
is
> good chance to clarify these issues.

Yes :D

[hiroshige, 2016-06-29 08:20:52]

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:189:
m_image->destroyDecodedData();
On 2016/06/29 07:04:06, hiroshige wrote:
> I think we have to clear |m_image| here, because we have to re-create
|m_image|
> in failed revalidation -- and it means we lack tests for that case. I'll add a
> test.

Created the tests. https://codereview.chromium.org/2101103004/

[f(malita), 2016-06-30 18:21:46]

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:226: return data;
Nit: return data.release();

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:124: return data;
Nit: return data.release();

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:125: return
m_source.data();
IIUC we clear the cached data as soon as we can, and m_source.data() allocates a
new SharedBuffer and makes a copy of the decoder data.

So while previously Resource::resourceBuffer() calls used to be always cheap,
now that's no longer the case for lazy-decoded images.  Is this going to impact
performance?

[hajimehoshi, 2016-07-04 10:46:25]

Thank you!

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (left):

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:332: clearImage();
On 2016/06/29 07:04:06, hiroshige wrote:
> Perhaps, we need to call clearImage() here, because clear() is called when an
> error occurs. Clearing |m_image| in case of error looks reasonable for me.

Done.

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:357:
m_image->clearImageObserver();
On 2016/06/28 15:33:44, scroggo_chromium wrote:
> Why is it okay to drop this line? My first thought was that we're about to
> delete m_image, which would clear the observer anyway, but some other class
> could hold a reference to m_image (e.g. by calling getImage() [1]), and I
assume
> we still want to clear the observer?
> 
> [1]
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Ima...

Ah, you're right. I'll revert this function and callers.

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:189:
m_image->destroyDecodedData();
On 2016/06/29 07:04:06, hiroshige wrote:
> I think we have to clear |m_image| here, because we have to re-create
|m_image|
> in failed revalidation -- and it means we lack tests for that case. I'll add a
> test.

Done.

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:196: if
((!hasClientsOrObservers() && !isLoading() && m_image->hasOneRef() &&
m_image->isBitmapImage()) || !errorOccurred())
On 2016/06/29 07:04:06, hiroshige wrote:
> I'm not sure, but can we simplify this condition to just |!errorOccurred()|?
> 
> The condition |!hasClientsOrObservers() && !isLoading() && (!m_image
> ||m_image->hasOneRef() && m_image->isBitmapImage()))| looks like for disposing
> |m_image|, and this CL makes |m_image| to be not disposal.

As we talked offline, let's keep this condition as it is. If we need
refactoring, we'll do it in another CL.

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:226: return data;
On 2016/06/30 18:21:46, f(malita) wrote:
> Nit: return data.release();

Done.

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:344: m_image = nullptr;
On 2016/06/29 07:04:06, hiroshige wrote:
> On 2016/06/28 15:33:44, scroggo_chromium wrote:
> > It's not obvious to me how this relates to the rest of the change, and I
don't
> > know enough about a multi-part response to judge whether this makes sense.
> > 
> > One place where this seems wrong to me if the following is true:
> > 1. No m_multipartParser
> > 2. We have already appended some data (but not all), so m_image is not NULL,
> and
> > neither is m_data.
> > 3. More data is appended in appendData()
> > 
> > This will call updateImage(false).
> > This method will clear m_image.
> > 
> > This means we'll have to recreate m_image from scratch, and previously we
> would
> > have reused the old one.
> 
> I think it's better to call clearImage() explicitly when a multipart image
needs
> to clear |m_image|, than clearing |m_image| here implicitly depending on
> |m_image|. (Generally, implicit assumptions on the status of Resources are
> frequently wrong...)

Done.

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:365: if (m_image &&
m_data)
On 2016/06/28 15:33:44, scroggo_chromium wrote:
> If we have m_data, aren't we guaranteed to also have m_image, since
> createImage() would have been called above?
> 
> Could you change this to:
> 
> if (m_data) {
>   DCHECK(m_image);
>   sizeAvailable = m_image->setData(m_data, allDataReceived);
> }
> 
> ?

Right, now m_image's lifetime should include m_data's. Done.

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:121:
PassRefPtr<SharedBuffer> BitmapImage::data()
On 2016/06/29 07:04:06, hiroshige wrote:
> The name data() here (and ImageSource) looks like just an accessor, but
actually
> it re-creates SharedBuffer if needed. How about renaming it to a name that
looks
> like more costly?

This method is overriding Image::data. Do you mean we should rename Image::data
and its all usages?

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:124: return data;
On 2016/06/30 18:21:46, f(malita) wrote:
> Nit: return data.release();

Done.

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:125: return
m_source.data();
On 2016/06/30 18:21:46, f(malita) wrote:
> IIUC we clear the cached data as soon as we can, and m_source.data() allocates
a
> new SharedBuffer and makes a copy of the decoder data.
> 
> So while previously Resource::resourceBuffer() calls used to be always cheap,
> now that's no longer the case for lazy-decoded images.  Is this going to
impact
> performance?

IIUC the situation where this function is limited: clipboard, serializing to
MHTML or WebGL. Among them, I think only WebGL can be a problem. I'll try to
check this with Telemetry.

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:128:
PassRefPtr<SharedBuffer> BitmapImage::cachedData()
On 2016/06/29 07:04:06, hiroshige wrote:
> On 2016/06/28 15:33:44, scroggo_chromium wrote:
> > A couple of thoughts on this method:
> > 
> > It is very simple, and we could almost inline it at the callsites, except
that
> > you want to call it from a test.
> > 
> > I find the name confusing. It's not obvious to me which one is "cached" and
> > which one is not. I guess we call it cached because Image may throw away its
> > data?
> FYI using "cached" prefix near loading code might be confusing; There are
> prefixes "cached" in loader code (e.g. |cachedImage|), but most of them are
just
> due to historical reasons (CachedImage is the old class name of ImageResource)
> and don't have much meaning.

Removed.

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:237: // there is an
encoded image data in DefferedImageDecoder on a skia
On 2016/06/28 15:33:44, scroggo_chromium wrote:
> nit: "there is encoded image data" (no need for "an" here)
> 
> nit: *DeferredImageDecoder (one 'f', two 'r's)
> 
> nit: I don't think "on a skia side" adds useful information. Maybe it's more
> clear to say something like "in DeferredImageDecoder's SkRWBuffer"?

Done.

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:239:
resetCachedData();
On 2016/06/28 15:33:44, scroggo_chromium wrote:
> Just to make sure I understand - this will drop the Image's reference to the
> SharedBuffer (m_encodedImageData), but the actual SharedBuffer will only get
> freed (thus saving memory) if the caller (e.g. ImageResource) also drops its
> reference?

Yes.

> Is it possible the Image will drop its reference, resulting in having to copy
> from the SkRWBuffer, in cases where the SharedBuffer is still around, so we
> could have skipped copying from the SkRWBuffer without holding onto extra
> memory? If so, would it be possible to only reset it if we know that that the
> ImageResource *also* dropped its reference? (Or does this CL already do that
and
> I'm missing something?)

Hmm, I think it's possible but it'd be very hard since we'd need hack
Resource::m_data without breaking other resource classes (hiroshige, do you
agree?) If we need this, I want to do it in another CL.

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Image.h (right):

https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Image.h:175: void resetCachedData()
{ m_encodedImageData.release(); }
On 2016/06/28 15:33:44, scroggo_chromium wrote:
> This method has an awkward name. It's a method on Image, which refers to its
> data as m_encodedImageData. The name is only relevant here in BitmapImage,
which
> calls a method called cachedData to retrieve m_encodedImageData.

Agreed. Removed this and overrided setData instead.

[hajimehoshi, 2016-07-05 11:59:05]

Description was changed from

==========
Remove duplication of encoded image data

Now there are at least two encoded image data: one is in Image/ImageResource as
SharedBuffer and the other is in DeferredImageDecoder as SkRWBuffer. This CL
removes former when possible (non-icon bitmaps), and generate the SharedBuffer
from the SkRWBuffer if needed.

BUG=618623
TEST=blink_platform_unittests
--gtest_filter=BitmapImageTest.*:ImageDecoderTest.*
==========

to

==========
Remove duplication of encoded image data

Now there are at least two encoded image data: one is in Image/ImageResource as
SharedBuffer and the other is in DeferredImageDecoder as SkRWBuffer. This CL
removes former when possible (non-icon bitmaps), and generate the SharedBuffer
from the SkRWBuffer if needed.

Design Doc:
https://docs.google.com/document/d/1v0yTAZ6wkqX2U_M6BNIGUJpM1s0TIw1VsqpxoL7ac...

BUG=618623
TEST=blink_platform_unittests
--gtest_filter=BitmapImageTest.*:ImageDecoderTest.*
==========

[hajimehoshi, 2016-07-05 12:13:17]

On 2016/06/29 07:07:29, hiroshige wrote:
> Probably we need a design doc that describe the lifetime of
> - |Resource::m_data|,
> - |ImageResource::m_image|, and
> - |Image::m_encodedImageData|
> and interactions with
> - BitmapImage/SVGImage,
> - Pruning,
> - Revalidation, and
> - Multipart image.
> 
> Yes, we lack the corresponding doc of the current state :-(, and I think this
is
> good chance to clarify these issues.

I've written the doc:
https://docs.google.com/document/d/1v0yTAZ6wkqX2U_M6BNIGUJpM1s0TIw1VsqpxoL7ac...

[hajimehoshi, 2016-07-07 11:56:39]

On 2016/06/30 18:21:46, f(malita) wrote:
>
https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):
> 
>
https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/fetch/ImageResource.cpp:226: return data;
> Nit: return data.release();
> 
>
https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):
> 
>
https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:124: return data;
> Nit: return data.release();
> 
>
https://codereview.chromium.org/2054643003/diff/280001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:125: return
> m_source.data();
> IIUC we clear the cached data as soon as we can, and m_source.data() allocates
a
> new SharedBuffer and makes a copy of the decoder data.
> 
> So while previously Resource::resourceBuffer() calls used to be always cheap,
> now that's no longer the case for lazy-decoded images.  Is this going to
impact
> performance?

I've tested with telemetry. The result is
http://www.googledrive.com/host/0BwW8PrCcts4WS1hIdkx4Z1pkZGM.

It’s a little difficult to say there is no performance regression with the
results since I couldn’t get more stable result on my local machine. There seem
regressions for some tests (e.g. Earth.html), but as the code says, the tests
doesn’t access encoded image data so often.

[hajimehoshi, 2016-07-08 02:17:44]

ping

[hiroshige, 2016-07-08 08:28:41]

https://codereview.chromium.org/2054643003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (left):

https://codereview.chromium.org/2054643003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:521:
updateImageAndClearBuffer();
FYI [1] will change this line. Because [1] is to be merged to M-52, this CL
should be rebased after [1].

[hiroshige, 2016-07-08 08:29:27]

https://codereview.chromium.org/2054643003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (left):

https://codereview.chromium.org/2054643003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:521:
updateImageAndClearBuffer();
On 2016/07/08 08:28:41, hiroshige wrote:
> FYI [1] will change this line. Because [1] is to be merged to M-52, this CL
> should be rebased after [1].

Sorry I forgot the link:
[1] https://codereview.chromium.org/2114213002/.

[f(malita), 2016-07-08 15:34:40]

Thanks for verifying perf with Telemetry.

Just a small refactoring question.

https://codereview.chromium.org/2054643003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/2054643003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:194:
m_temporaryEncodedImageData = data;
Do we need m_temporaryEncodedImageData at all?  Looks like we only set it
temporarily here, and dataChanged always clears it.

Why not set the source data upfront, before calling dataChanged() - i.e. would
this work:

bool BitmapImage::setData(PassRefPtr<SharedBuffer> data, bool allDataReceived)
{
    if (!data || data->isEmpty())
        return true;

    m_source.setData(*data, allDataReceived);

    return dataChanged(allDataReceived);
}

[hajimehoshi, 2016-07-11 06:15:08]

Thank you!

https://codereview.chromium.org/2054643003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/2054643003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:194:
m_temporaryEncodedImageData = data;
On 2016/07/08 15:34:40, f(malita) wrote:
> Do we need m_temporaryEncodedImageData at all?  Looks like we only set it
> temporarily here, and dataChanged always clears it.
> 
> Why not set the source data upfront, before calling dataChanged() - i.e. would
> this work:
> 
> bool BitmapImage::setData(PassRefPtr<SharedBuffer> data, bool allDataReceived)
> {
>     if (!data || data->isEmpty())
>         return true;
> 
>     m_source.setData(*data, allDataReceived);
> 
>     return dataChanged(allDataReceived);
> }

Done.

[f(malita), 2016-07-12 17:10:59]

https://codereview.chromium.org/2054643003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/BitmapImage.cpp (right):

https://codereview.chromium.org/2054643003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/BitmapImage.cpp:194:
m_temporaryEncodedImageData = data;
On 2016/07/11 06:15:07, hajimehoshi wrote:
> On 2016/07/08 15:34:40, f(malita) wrote:
> > Do we need m_temporaryEncodedImageData at all?  Looks like we only set it
> > temporarily here, and dataChanged always clears it.
> > 
> > Why not set the source data upfront, before calling dataChanged() - i.e.
would
> > this work:
> > 
> > bool BitmapImage::setData(PassRefPtr<SharedBuffer> data, bool
allDataReceived)
> > {
> >     if (!data || data->isEmpty())
> >         return true;
> > 
> >     m_source.setData(*data, allDataReceived);
> > 
> >     return dataChanged(allDataReceived);
> > }
> 
> Done.

Thanks, LGTM!

[haraken, 2016-07-13 02:18:30]

Mostly looks good.

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (left):

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:399: m_data.clear();

I don't fully understand why you need to move this code into
ImageResource::updateImage.

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:521:
updateImageAndClearBuffer();

Ditto. Why do you need this change?

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:228: return
data.release();

if (m_data)
  return m_data.get();

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:402: m_data.clear();

Add a comment what this is doing. It would be a good idea to add a link to your
document that explains the lifetime of m_data and m_image.

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:897: m_archive =
MHTMLArchive::create(resource->url(), resource->resourceBuffer().get());

You don't need to fix it in this CL but here we should pass in PassRefPtr to
MHTMLArchive::create and pass the ownership down to SharedBufferChunkReader.

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:138: return
sharedBuffer;

sharedBuffer.release()

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ImageSource.h (right):

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageSource.h:73:
PassRefPtr<SharedBuffer> data();

Who uses these methods?

[hajimehoshi, 2016-07-13 07:14:33]

Patchset #21 (id:420001) has been deleted

[hajimehoshi, 2016-07-13 07:36:22]

Thank you!

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (left):

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:399: m_data.clear();
On 2016/07/13 02:18:29, haraken wrote:
> 
> I don't fully understand why you need to move this code into
> ImageResource::updateImage.

m_data.clear() is called in updateImage(true) instead.

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:521:
updateImageAndClearBuffer();
On 2016/07/13 02:18:29, haraken wrote:
> 
> Ditto. Why do you need this change?

As the document says, updateImageAndClearBuffer() ensures that m_image exists
with the current m_data and clears m_data (new data is set to m_data when
finishing the loading).

Found that this is no longer needed (m_image must exist and m_data doesn't).
Removed.

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:228: return
data.release();
On 2016/07/13 02:18:30, haraken wrote:
> 
> if (m_data)
>   return m_data.get();

Done.

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:402: m_data.clear();
On 2016/07/13 02:18:30, haraken wrote:
> 
> Add a comment what this is doing. It would be a good idea to add a link to
your
> document that explains the lifetime of m_data and m_image.
> 

Done.

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:897: m_archive =
MHTMLArchive::create(resource->url(), resource->resourceBuffer().get());
On 2016/07/13 02:18:30, haraken wrote:
> 
> You don't need to fix it in this CL but here we should pass in PassRefPtr to
> MHTMLArchive::create and pass the ownership down to SharedBufferChunkReader.

Done.

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp
(right):

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/DeferredImageDecoder.cpp:138: return
sharedBuffer;
On 2016/07/13 02:18:30, haraken wrote:
> 
> sharedBuffer.release()

Done.

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ImageSource.h (right):

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ImageSource.h:73:
PassRefPtr<SharedBuffer> data();
On 2016/07/13 02:18:30, haraken wrote:
> 
> Who uses these methods?

data() is used by BitmapImage.

hasData() is not used by anyone. Removed.

[haraken, 2016-07-13 07:38:34]

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (left):

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:399: m_data.clear();
On 2016/07/13 07:36:22, hajimehoshi wrote:
> On 2016/07/13 02:18:29, haraken wrote:
> > 
> > I don't fully understand why you need to move this code into
> > ImageResource::updateImage.
> 
> m_data.clear() is called in updateImage(true) instead.

My question is why you moved m_data.clear() into updateImage(true).

It's strange that "updateImageAndClearBuffer" does not clear the buffer.

[hajimehoshi, 2016-07-13 07:59:59]

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (left):

https://codereview.chromium.org/2054643003/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:399: m_data.clear();
On 2016/07/13 07:38:34, haraken wrote:
> On 2016/07/13 07:36:22, hajimehoshi wrote:
> > On 2016/07/13 02:18:29, haraken wrote:
> > > 
> > > I don't fully understand why you need to move this code into
> > > ImageResource::updateImage.
> > 
> > m_data.clear() is called in updateImage(true) instead.
> 
> My question is why you moved m_data.clear() into updateImage(true).
> 
> It's strange that "updateImageAndClearBuffer" does not clear the buffer.

Ah, right. The reason why I moved m_data.clear() to updateImage() is because now
m_data is always cleared when m_image is updated (and all data is received).

On second thought, I reverted this and moved m_data.clear() in updateImage() to
finish() to minimize diff. Done.

[haraken, 2016-07-13 08:07:42]

https://codereview.chromium.org/2054643003/diff/500001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (left):

https://codereview.chromium.org/2054643003/diff/500001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:532:
updateImageAndClearBuffer();

Why can we remove this? Now you're not calling m_data.clear in
updateImage(true), so you need to explicitly call updateImageAndClearBuffer()
here.

[haraken, 2016-07-13 08:13:58]

Discussed offline. LGTM.

[hiroshige, 2016-07-13 09:26:53]

hiroshige@chromium.org changed reviewers:
+ mkwst@chromium.org

[hiroshige, 2016-07-13 09:26:55]

+mkwst@ as a core/fetch OWNER.
(This CL is already l-g-t-m'ed by core OWNERs, but I'd like to have another look
by a core/fetch OWNER because this CL does a structure change)

Added some minor comments.

Please wait for some days to commit this CL, because [1] and [2] contains
conflicting changes and to be merged into stable.
I want [1] and [2] to be committed before this CL and stay on canary/dev for a
day without this CL.
(Optionally I'd like to commit a UMA in [3] before this CL and see the changes.)

[1] https://codereview.chromium.org/2114213002/
[2] https://codereview.chromium.org/2141843003/
[3] https://codereview.chromium.org/2140513002/

https://codereview.chromium.org/2054643003/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (left):

https://codereview.chromium.org/2054643003/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:126: void
ImageResource::didAddClient(ResourceClient* client)
Could you leave |DCHECK(!m_data || m_image);| here for consistency with
addObserver()?

https://codereview.chromium.org/2054643003/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2054643003/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:416: // As encoded image
data can be created from m_image, we don't have to
How about "As encoded image data can be created from m_image (see
ImageResource::resourceBuffer()), ..."?

https://codereview.chromium.org/2054643003/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:419: //
https://docs.google.com/document/d/1v0yTAZ6wkqX2U_M6BNIGUJpM1s0TIw1VsqpxoL7ac...
This doc describing the behavior of ImageResource and Image quite well. Putting
this link at the head of ImageResource.h might be helpful.

https://codereview.chromium.org/2054643003/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:533: DCHECK(m_image);
Probably this assertion is not always true. Anyway let's rebase after
https://codereview.chromium.org/2114213002/.

[hajimehoshi, 2016-07-13 12:25:45]

https://codereview.chromium.org/2054643003/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (left):

https://codereview.chromium.org/2054643003/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:126: void
ImageResource::didAddClient(ResourceClient* client)
On 2016/07/13 09:26:55, hiroshige wrote:
> Could you leave |DCHECK(!m_data || m_image);| here for consistency with
> addObserver()?

Done.

https://codereview.chromium.org/2054643003/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/2054643003/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:416: // As encoded image
data can be created from m_image, we don't have to
On 2016/07/13 09:26:55, hiroshige wrote:
> How about "As encoded image data can be created from m_image (see
> ImageResource::resourceBuffer()), ..."?

Done.

https://codereview.chromium.org/2054643003/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:419: //
https://docs.google.com/document/d/1v0yTAZ6wkqX2U_M6BNIGUJpM1s0TIw1VsqpxoL7ac...
On 2016/07/13 09:26:55, hiroshige wrote:
> This doc describing the behavior of ImageResource and Image quite well.
Putting
> this link at the head of ImageResource.h might be helpful.

Done.

https://codereview.chromium.org/2054643003/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:533: DCHECK(m_image);
On 2016/07/13 09:26:55, hiroshige wrote:
> Probably this assertion is not always true. Anyway let's rebase after
> https://codereview.chromium.org/2114213002/.

Ahh, really? Thanks.

[hajimehoshi, 2016-07-15 06:59:43]

The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-15 06:59:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-15 08:10:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-15 08:10:56]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Mike West, 2016-07-18 08:36:29]

//Source/core/fetch LGTM. Let's see if those DCHECKs start firing. :)

[hajimehoshi, 2016-07-19 06:15:53]

The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 06:16:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hajimehoshi, 2016-07-19 06:16:21]

Thank you!

hiroshige, are you fine with this change landing?

[hiroshige, 2016-07-19 08:42:10]

lgtm, all related CLs I mentioned have been landed.

Thank you for making this change!

[commit-bot: I haz the power, 2016-07-19 09:45:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-19 09:45:46]

Dry run: This issue passed the CQ dry run.

[hajimehoshi, 2016-07-19 09:46:18]

The CQ bit was checked by hajimehoshi@chromium.org

[hajimehoshi, 2016-07-19 09:46:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
scroggo@chromium.org, fmalita@chromium.org, haraken@chromium.org,
mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2054643003/#ps580001
(title: "(rebase)")

[commit-bot: I haz the power, 2016-07-19 09:46:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 09:59:48]

Description was changed from

==========
Remove duplication of encoded image data

Now there are at least two encoded image data: one is in Image/ImageResource as
SharedBuffer and the other is in DeferredImageDecoder as SkRWBuffer. This CL
removes former when possible (non-icon bitmaps), and generate the SharedBuffer
from the SkRWBuffer if needed.

Design Doc:
https://docs.google.com/document/d/1v0yTAZ6wkqX2U_M6BNIGUJpM1s0TIw1VsqpxoL7ac...

BUG=618623
TEST=blink_platform_unittests
--gtest_filter=BitmapImageTest.*:ImageDecoderTest.*
==========

to

==========
Remove duplication of encoded image data

Now there are at least two encoded image data: one is in Image/ImageResource as
SharedBuffer and the other is in DeferredImageDecoder as SkRWBuffer. This CL
removes former when possible (non-icon bitmaps), and generate the SharedBuffer
from the SkRWBuffer if needed.

Design Doc:
https://docs.google.com/document/d/1v0yTAZ6wkqX2U_M6BNIGUJpM1s0TIw1VsqpxoL7ac...

BUG=618623
TEST=blink_platform_unittests
--gtest_filter=BitmapImageTest.*:ImageDecoderTest.*
==========

[commit-bot: I haz the power, 2016-07-19 09:59:50]

Committed patchset #28 (id:580001)

[commit-bot: I haz the power, 2016-07-19 09:59:58]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-19 10:02:15]

Description was changed from

==========
Remove duplication of encoded image data

Now there are at least two encoded image data: one is in Image/ImageResource as
SharedBuffer and the other is in DeferredImageDecoder as SkRWBuffer. This CL
removes former when possible (non-icon bitmaps), and generate the SharedBuffer
from the SkRWBuffer if needed.

Design Doc:
https://docs.google.com/document/d/1v0yTAZ6wkqX2U_M6BNIGUJpM1s0TIw1VsqpxoL7ac...

BUG=618623
TEST=blink_platform_unittests
--gtest_filter=BitmapImageTest.*:ImageDecoderTest.*
==========

to

==========
Remove duplication of encoded image data

Now there are at least two encoded image data: one is in Image/ImageResource as
SharedBuffer and the other is in DeferredImageDecoder as SkRWBuffer. This CL
removes former when possible (non-icon bitmaps), and generate the SharedBuffer
from the SkRWBuffer if needed.

Design Doc:
https://docs.google.com/document/d/1v0yTAZ6wkqX2U_M6BNIGUJpM1s0TIw1VsqpxoL7ac...

BUG=618623
TEST=blink_platform_unittests
--gtest_filter=BitmapImageTest.*:ImageDecoderTest.*

Committed: https://crrev.com/36f4eb81d4a791a8e3dfea4ba2418465d30bcc90
Cr-Commit-Position: refs/heads/master@{#406238}
==========

[commit-bot: I haz the power, 2016-07-19 10:02:18]

Patchset 28 (id:??) landed as
https://crrev.com/36f4eb81d4a791a8e3dfea4ba2418465d30bcc90
Cr-Commit-Position: refs/heads/master@{#406238}

[haraken, 2016-07-20 16:54:05]

On 2016/07/19 10:02:18, commit-bot: I haz the power wrote:
> Patchset 28 (id:??) landed as
> https://crrev.com/36f4eb81d4a791a8e3dfea4ba2418465d30bcc90
> Cr-Commit-Position: refs/heads/master@{#406238}

I believe this CL dramatically reduced renderer's memory. Would it be possible
to get some numbers?

[hajimehoshi, 2016-07-21 04:12:10]

On 2016/07/20 16:54:05, haraken wrote:
> On 2016/07/19 10:02:18, commit-bot: I haz the power wrote:
> > Patchset 28 (id:??) landed as
> > https://crrev.com/36f4eb81d4a791a8e3dfea4ba2418465d30bcc90
> > Cr-Commit-Position: refs/heads/master@{#406238}
> 
> I believe this CL dramatically reduced renderer's memory. Would it be possible
> to get some numbers?

We confirmed significant memory deduction in a certain set of telemetry tests
(thank you, tasak@-san!).

On the other hand, we couldn't confirm any improvements in chrome performance
dashboard. I guess there is no good metrics for renderer memory usage in the
dashboard (Memory_RendererUsed in the dashboard is a sum of tc_malloc memory
usage and V8 memory usage, which seems not to include PartitionAlloc) but not
sure.

[lhp.peter, 2016-09-25 13:04:20]

Why not make SharedBuffer threadsafe and discard SkRWBuffer from
DeferredImageDecoder? I think it will resolve this problem originally.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Noel Gordon, 2017-02-22 06:37:17]

On 2016/09/25 13:04:20, lhp.peter_gmail.com wrote:
> Why not make SharedBuffer threadsafe

That is a very good question.  Was that considered for this work? 

This work saved memory?  No, it undid a memory regression - it put us back to
where we were before DeferredImageDecoder was added to Blink.

That feature (DeferredImageDecoder) required Blink to send the ShardedBuffer
data (the encoded image data) to another thread, and so it was _copied_ to that
thread, a memory regression.

Would have been lot simpler, and no memory regression, if SharedBuffer was
threadsafe.

[hajimehoshi, 2017-02-22 07:25:30]

On 2016/09/25 13:04:20, lhp.peter_gmail.com wrote:
> Why not make SharedBuffer threadsafe and discard SkRWBuffer from
> DeferredImageDecoder? I think it will resolve this problem originally.
> 
> -- 
> You received this message because you are subscribed to the Google Groups
"Blink
> Reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

As far as I can remember, we didn't want to add any locks for buffers for
performance. SkRWBuffer is special since this doesn't require any locks when it
is read by multiple threads and written by one thread at the same time.
SkRWBuffer is an append-only data storage and appending data chunk doesn't
affect readers.

[lhp.peter, 2017-05-25 12:11:56]

On 2017/02/22 07:25:30, hajimehoshi wrote:
> On 2016/09/25 13:04:20, http://lhp.peter_gmail.com wrote:
> > Why not make SharedBuffer threadsafe and discard SkRWBuffer from
> > DeferredImageDecoder? I think it will resolve this problem originally.
> > 
> > -- 
> > You received this message because you are subscribed to the Google Groups
> "Blink
> > Reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:blink-reviews+unsubscribe@chromium.org.
> 
> As far as I can remember, we didn't want to add any locks for buffers for
> performance. SkRWBuffer is special since this doesn't require any locks when
it
> is read by multiple threads and written by one thread at the same time.
> SkRWBuffer is an append-only data storage and appending data chunk doesn't
> affect readers.

Depending on whether the vectors' capacity needs to be expanded, SharedBuffer
can be divided into several steps.
In step, single thread writing and multi-threaded reading are safe. So only in
the step switch, synchronization should be make. 
This cost is light enough. Moreover, the existing ROBufferSegmentReader also
need to add a wide range of lock when reading.

In addition, SharedBuffer supports random access, while ROBufferSegmentReader
only supports forward or backward access by segment.

[scroggo_chromium, 2017-05-30 20:39:07]

On 2017/05/25 12:11:56, lhp.peter wrote:
> On 2017/02/22 07:25:30, hajimehoshi wrote:
> > On 2016/09/25 13:04:20, http://lhp.peter_gmail.com wrote:
> > > Why not make SharedBuffer threadsafe and discard SkRWBuffer from
> > > DeferredImageDecoder? I think it will resolve this problem originally.
> > > 
> > > -- 
> > > You received this message because you are subscribed to the Google Groups
> > "Blink
> > > Reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:blink-reviews+unsubscribe@chromium.org.
> > 
> > As far as I can remember, we didn't want to add any locks for buffers for
> > performance. SkRWBuffer is special since this doesn't require any locks when
> it
> > is read by multiple threads and written by one thread at the same time.
> > SkRWBuffer is an append-only data storage and appending data chunk doesn't
> > affect readers.
> 
> Depending on whether the vectors' capacity needs to be expanded, SharedBuffer
> can be divided into several steps.
> In step, single thread writing and multi-threaded reading are safe. So only in
> the step switch, synchronization should be make. 

Sorry, I do not follow you.

> This cost is light enough. Moreover, the existing ROBufferSegmentReader also
> need to add a wide range of lock when reading.

Although ROBufferSegmentReader does use a lock, it only uses one for reading. In
practice, there is only one owner of an ROBufferSegmentReader, so there will be
no contention on that lock. (There is a brief note about this in the description
for
https://chromium.googlesource.com/chromium/src/+/d2234904faee943bd987bd38d620...)

If we were to make SharedBuffer threadsafe, wouldn't we have to lock while
reading and writing? I suspect there would be some contention here, leading to
worse performance, but maybe I'm wrong.

> 
> In addition, SharedBuffer supports random access, while ROBufferSegmentReader
> only supports forward or backward access by segment.

Indeed. Many calls access consecutive data, though, so often random access is
not necessary.