MHTML generation: discard serialized resources by the measure they are encoded.

MHTML generation: discard serialized resources by the measure they are encoded.

This change update MHTML generation code to avoid having both serialized and
encoded copies of resources in memory. As pages being saved to MHTML can get to
many MB in size this change avoids a peak in memory usage where both serialized
and encoded versions of the same resource existed.

This specific optimization is local to WebFrameSerializer but as it required
replacing a wtf::Vector with a wtf::Deque as the container of resources that
caused the need to update a few other files.

BUG=658556
Committed: https://crrev.com/65ad0174d081840215c19f69ab819f19fbc667b9
Cr-Commit-Position: refs/heads/master@{#441567}

[carlosk, 2016-12-30 02:27:06]

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

[commit-bot: I haz the power, 2016-12-30 02:27:19]

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

[carlosk, 2016-12-30 03:05:04]

carlosk@chromium.org changed reviewers:
+ dcheng@chromium.org, lukasza@chromium.org

[carlosk, 2016-12-30 03:05:04]

dcheng@, lukasza@: PTAL.

[carlosk, 2016-12-30 03:08:47]

https://codereview.chromium.org/2605413002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebFrameSerializer.cpp (right):

https://codereview.chromium.org/2605413002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:312: resources.takeFirst(),
*output->mutableData());
To make this clear: my assumption is that these 2 calls to takeFirst (here and
below) will not cause any copies due to move semantics.

[commit-bot: I haz the power, 2016-12-30 03:57:28]

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

[commit-bot: I haz the power, 2016-12-30 03:57:29]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-12-30 17:39:17]

This CL LGTM.

I wonder if further improvements (to keep at most one serialized and/or
mhtml-encoded resource in memory) might be achieved by refactoring serialization
to have a kind of a streaming/async API - maybe FrameSerializer's constructor
doesn't need to take and populate a Vector<SerializedResource>&, but instead can
call a method of Delegate& whenever a new serialized resource is available. 
There are some caveats though:

- I am not sure how this async/streaming proposal interacts with writing to the
file from the IO thread (this is easier without backpressure [to slow things
down if IO thread doesn't keep up], but then we are back to high memory
consumption).

- I am not sure if we want to add extra complexity without having a way to
automatically test memory consumption to verify improvements and prevent
regressions.

https://codereview.chromium.org/2605413002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebFrameSerializer.cpp (right):

https://codereview.chromium.org/2605413002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:312: resources.takeFirst(),
*output->mutableData());
On 2016/12/30 03:08:47, carlosk wrote:
> To make this clear: my assumption is that these 2 calls to takeFirst (here and
> below) will not cause any copies due to move semantics.

I think that because SerializedResource doesn't define a move constructor or a
move assignment operator, then there are going to be copies.  OTOH, the thing
copied would be the ref-counted pointed (not the actual contents of the
SharedBuffer), so this shouldn't be a problem, right?

One way to avoid a copy here would be to pass resources.first() and then
explicitly remove the element via resources.removeFirst().  OTOH, 1) I am not
sure if this is worth it and 2) there might still be a copy happening when
appending to the deque in FrameSerializer (not sure about this).

At any rate, I think the current code is fine.

[dcheng, 2017-01-03 22:31:16]

https://codereview.chromium.org/2605413002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebFrameSerializer.cpp (right):

https://codereview.chromium.org/2605413002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:312: resources.takeFirst(),
*output->mutableData());
On 2016/12/30 17:39:16, Łukasz Anforowicz wrote:
> On 2016/12/30 03:08:47, carlosk wrote:
> > To make this clear: my assumption is that these 2 calls to takeFirst (here
and
> > below) will not cause any copies due to move semantics.
> 
> I think that because SerializedResource doesn't define a move constructor or a
> move assignment operator, then there are going to be copies.  OTOH, the thing
> copied would be the ref-counted pointed (not the actual contents of the
> SharedBuffer), so this shouldn't be a problem, right?
> 
> One way to avoid a copy here would be to pass resources.first() and then
> explicitly remove the element via resources.removeFirst().  OTOH, 1) I am not
> sure if this is worth it and 2) there might still be a copy happening when
> appending to the deque in FrameSerializer (not sure about this).
> 
> At any rate, I think the current code is fine.

SerializedResource will probably have a compiler generated move ctor, but it's
not clear to me the benefit will be that large. If it becomes significant, I'm
sure we can figure out what to do =)

I assume you've tested that this Deque is the only thing with references to the
SerializedResource? Since the data buffer is refcounted, this won't end up
giving us much gain if some other code is retaining references to the data
buffers.

[carlosk, 2017-01-05 01:34:57]

Thanks.

On 2016/12/30 17:39:17, Łukasz Anforowicz wrote:
> This CL LGTM.
> 
> I wonder if further improvements (to keep at most one serialized and/or
> mhtml-encoded resource in memory) might be achieved by refactoring
serialization
> to have a kind of a streaming/async API - maybe FrameSerializer's constructor
> doesn't need to take and populate a Vector<SerializedResource>&, but instead
can
> call a method of Delegate& whenever a new serialized resource is available. 
> There are some caveats though:
> 
> - I am not sure how this async/streaming proposal interacts with writing to
the
> file from the IO thread (this is easier without backpressure [to slow things
> down if IO thread doesn't keep up], but then we are back to high memory
> consumption).

Yes, the fact this is written to file in another thread complicates things. To
limit memory usage we'd need some sort of shared memory space to at least to
allow both threads to know the amount/size of in-memory data waiting to the
written. I don't think that's possible solely with IPCs but they could be used
for holding the data itself.

> - I am not sure if we want to add extra complexity without having a way to
> automatically test memory consumption to verify improvements and prevent
> regressions.

Might also make sense to have those but we can have a clue about this just by
looking at some UMA data that states the final MHTML file sizes. And often they
do get considerable big, as in tens of MB. This might even already be available
from performance tests that track memory or experiments data.

This is also somewhat related to another potential change that would allow
resources to be serialized in chunks so to avoid holding the main renderer
thread for too long: https://crbug.com/658552).

https://codereview.chromium.org/2605413002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebFrameSerializer.cpp (right):

https://codereview.chromium.org/2605413002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebFrameSerializer.cpp:312: resources.takeFirst(),
*output->mutableData());
On 2017/01/03 22:31:16, dcheng wrote:
> On 2016/12/30 17:39:16, Łukasz Anforowicz wrote:
> > On 2016/12/30 03:08:47, carlosk wrote:
> > > To make this clear: my assumption is that these 2 calls to takeFirst (here
> and
> > > below) will not cause any copies due to move semantics.
> > 
> > I think that because SerializedResource doesn't define a move constructor or
a
> > move assignment operator, then there are going to be copies.  OTOH, the
thing
> > copied would be the ref-counted pointed (not the actual contents of the
> > SharedBuffer), so this shouldn't be a problem, right?
> > 
> > One way to avoid a copy here would be to pass resources.first() and then
> > explicitly remove the element via resources.removeFirst().  OTOH, 1) I am
not
> > sure if this is worth it and 2) there might still be a copy happening when
> > appending to the deque in FrameSerializer (not sure about this).
> > 
> > At any rate, I think the current code is fine.
> 
> SerializedResource will probably have a compiler generated move ctor, but it's
> not clear to me the benefit will be that large. If it becomes significant, I'm
> sure we can figure out what to do =)

I also think it should have an auto-generated move constructor. Otherwise the
copy cost should be low: it would copy a KURL, a Mime type string and the
pointer data to the SharedBuffer.

My initial implementation used first() and removeFirst() but this version is
just shorter. I see no problem in reverting back to that if you think it might
be better.

> I assume you've tested that this Deque is the only thing with references to
the
> SerializedResource? Since the data buffer is refcounted, this won't end up
> giving us much gain if some other code is retaining references to the data
> buffers.

I don't know for sure what you mean as "tested" for this. But by investigating
the code of WebFrameSerializer and FrameSerializer I can see these
SerializedResource pointers are not stored long-term anywhere else beyond inside
this collection.

[dcheng, 2017-01-05 01:37:27]

LGTM

[carlosk, 2017-01-05 01:45:48]

The CQ bit was checked by carlosk@chromium.org

[commit-bot: I haz the power, 2017-01-05 01:46:20]

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, 2017-01-05 03:35:12]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1483580748063560, "parent_rev":
"c99e2651530d975ee52a3a339dd32f11165f476d", "commit_rev":
"7f4fccaeb5ca35e27e0c5f5e74bd3a6b28f962da"}

[commit-bot: I haz the power, 2017-01-05 03:35:39]

Description was changed from

==========
MHTML generation: discard serialized resources by the measure they are encoded.

This change update MHTML generation code to avoid having both serialized and
encoded copies of resources in memory. As pages being saved to MHTML can get to
many MB in size this change avoids a peak in memory usage where both serialized
and encoded versions of the same resource existed.

This specific optimization is local to WebFrameSerializer but as it required
replacing a wtf::Vector with a wtf::Deque as the container of resources that
caused the need to update a few other files.

BUG=658556
==========

to

==========
MHTML generation: discard serialized resources by the measure they are encoded.

This change update MHTML generation code to avoid having both serialized and
encoded copies of resources in memory. As pages being saved to MHTML can get to
many MB in size this change avoids a peak in memory usage where both serialized
and encoded versions of the same resource existed.

This specific optimization is local to WebFrameSerializer but as it required
replacing a wtf::Vector with a wtf::Deque as the container of resources that
caused the need to update a few other files.

BUG=658556

Review-Url: https://codereview.chromium.org/2605413002
==========

[commit-bot: I haz the power, 2017-01-05 03:35:40]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2017-01-05 03:38:15]

Description was changed from

==========
MHTML generation: discard serialized resources by the measure they are encoded.

This change update MHTML generation code to avoid having both serialized and
encoded copies of resources in memory. As pages being saved to MHTML can get to
many MB in size this change avoids a peak in memory usage where both serialized
and encoded versions of the same resource existed.

This specific optimization is local to WebFrameSerializer but as it required
replacing a wtf::Vector with a wtf::Deque as the container of resources that
caused the need to update a few other files.

BUG=658556

Review-Url: https://codereview.chromium.org/2605413002
==========

to

==========
MHTML generation: discard serialized resources by the measure they are encoded.

This change update MHTML generation code to avoid having both serialized and
encoded copies of resources in memory. As pages being saved to MHTML can get to
many MB in size this change avoids a peak in memory usage where both serialized
and encoded versions of the same resource existed.

This specific optimization is local to WebFrameSerializer but as it required
replacing a wtf::Vector with a wtf::Deque as the container of resources that
caused the need to update a few other files.

BUG=658556

Committed: https://crrev.com/65ad0174d081840215c19f69ab819f19fbc667b9
Cr-Commit-Position: refs/heads/master@{#441567}
==========

[commit-bot: I haz the power, 2017-01-05 03:38:16]

Patchset 1 (id:??) landed as
https://crrev.com/65ad0174d081840215c19f69ab819f19fbc667b9
Cr-Commit-Position: refs/heads/master@{#441567}