Small tracing improvements to MHTML generation code.

Small tracing improvements to MHTML generation code.

The main objective of this change is to add tracing to the
RenderFrameImpl::OnWriteMHTMLToDiskComplete so that one can see in trace
report the delay introduced by the two thread hops for doing IO in the
file thread: from the "main" to the "file" and then back to "main".

This lead to a few related changes:
- The thread hops are avoided when not actually needed.
- As a consequence the WriteMHTMLToDisk function was slightly simplified.
- And a few outdated code comments were updated.

BUG=645686
Committed: https://crrev.com/d9ff2b43b1dc1fba22fd16a913c979de09687223
Cr-Commit-Position: refs/heads/master@{#431027}

[carlosk, 2016-11-09 02:09:54]

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

[commit-bot: I haz the power, 2016-11-09 02:10: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-11-09 02:10:26]

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

[carlosk, 2016-11-09 02:10:27]

lukasza@, nasko@: PTAL.

[carlosk, 2016-11-09 02:11:12]

Description was changed from

==========
mall tracing improvements to MHTML generation code.

The main objective of this change is to add tracing to the
RenderFrameImpl::OnWriteMHTMLToDiskComplete so that one can see in trace
report the delay introduced by the two thread hops for doing IO in the
file thread: from the "main" to the "file" and then back to "main".

This lead to a few related changes:
- The thread hops are avoided when not actually needed.
- As a consequence the WriteMHTMLToDisk function was slightly simplified.
- And a few outdated code comments were updated.

BUG=645686
==========

to

==========
Small tracing improvements to MHTML generation code.

The main objective of this change is to add tracing to the
RenderFrameImpl::OnWriteMHTMLToDiskComplete so that one can see in trace
report the delay introduced by the two thread hops for doing IO in the
file thread: from the "main" to the "file" and then back to "main".

This lead to a few related changes:
- The thread hops are avoided when not actually needed.
- As a consequence the WriteMHTMLToDisk function was slightly simplified.
- And a few outdated code comments were updated.

BUG=645686
==========

[commit-bot: I haz the power, 2016-11-09 03:47:13]

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

[commit-bot: I haz the power, 2016-11-09 03:47:14]

Dry run: This issue passed the CQ dry run.

[Łukasz Anforowicz, 2016-11-09 17:12:24]

LGTM if my understanding of the change (see the comment below) is accurate.

https://codereview.chromium.org/2490853002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2490853002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:5431: main_thread_use_time, success);
So - we are saying now that it is not necessary to call |file.Close()| before
sending FrameHostMsg_SerializeAsMHTMLResponse, *if* there was no data to be
written to the file.  In this case file.Close will be implicitly closed by
base::File::~File destructor (after sending
FrameHostMsg_SerializeAsMHTMLResponse), but this is okay - when no data is
written, the browser doesn't need to wait for the file flush/close to complete
(wait before giving the file handle to the next frame / renderer).

Is this accurate?

[carlosk, 2016-11-09 18:10:32]

Thanks.

https://codereview.chromium.org/2490853002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2490853002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:5431: main_thread_use_time, success);
On 2016/11/09 17:12:24, Łukasz Anforowicz wrote:
> So - we are saying now that it is not necessary to call |file.Close()| before
> sending FrameHostMsg_SerializeAsMHTMLResponse, *if* there was no data to be
> written to the file.  In this case file.Close will be implicitly closed by
> base::File::~File destructor (after sending
> FrameHostMsg_SerializeAsMHTMLResponse), but this is okay - when no data is
> written, the browser doesn't need to wait for the file flush/close to complete
> (wait before giving the file handle to the next frame / renderer).
> 
> Is this accurate?

Yes, that was my assumption. I considered closing before this call here but as
no data would have been written to disk it seemed unnecessary. 

But this is indeed an assumption, so maybe I should add it just to be sure.
WDYT?

[Łukasz Anforowicz, 2016-11-09 18:27:27]

https://codereview.chromium.org/2490853002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2490853002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:5431: main_thread_use_time, success);
On 2016/11/09 18:10:32, carlosk wrote:
> On 2016/11/09 17:12:24, Łukasz Anforowicz wrote:
> > So - we are saying now that it is not necessary to call |file.Close()|
before
> > sending FrameHostMsg_SerializeAsMHTMLResponse, *if* there was no data to be
> > written to the file.  In this case file.Close will be implicitly closed by
> > base::File::~File destructor (after sending
> > FrameHostMsg_SerializeAsMHTMLResponse), but this is okay - when no data is
> > written, the browser doesn't need to wait for the file flush/close to
complete
> > (wait before giving the file handle to the next frame / renderer).
> > 
> > Is this accurate?
> 
> Yes, that was my assumption. I considered closing before this call here but as
> no data would have been written to disk it seemed unnecessary. 
> 
> But this is indeed an assumption, so maybe I should add it just to be sure.
> WDYT?

I am okay with not calling file.Close if no data is written.  I think adding a
comment explaining this might be useful to future maintainers.  For a moment I
wondered whether the file might be opened in exclusive mode (i.e. not passing
FILE_SHARE_WRITE or anything like this on Windows), but we go through the
C/POSIX open call, so it doesn't seem to be the case.

OTOH, I wonder what is a performance impact of calling file.Close on a file
handle that wasn't written to.  If the impact is small, then maybe we can insert
a call to file.Close before calling OnWriteMHTMLToDiskComplete.  This might make
the code easier to understand.  Also, I am not sure why
ThreadRestrictions::AssertIOAllowed() didn't shout before you've moved file IO
to the separate thread - maybe calling file.Close would make it again slightly
more difficult to turn on enforcement of AssertIOAllowed.

As I said - I am fine with the CL either way (1) land as-is + comment explaining
why file.Close can be called *after* IPC OR 2) add a call to file.Close before
calling OnWriteMHTMLToDiskComplete).

[nasko, 2016-11-09 18:33:17]

Rubberstamp LGTM based on lukasza@'s review, assuming the comment or close call
are added to the CL.

[carlosk, 2016-11-09 19:16:11]

Thanks for yet another comprehensive review Łukasz! :)

https://codereview.chromium.org/2490853002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2490853002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:5431: main_thread_use_time, success);
On 2016/11/09 18:27:27, Łukasz Anforowicz wrote:
> On 2016/11/09 18:10:32, carlosk wrote:
> > On 2016/11/09 17:12:24, Łukasz Anforowicz wrote:
> > > So - we are saying now that it is not necessary to call |file.Close()|
> before
> > > sending FrameHostMsg_SerializeAsMHTMLResponse, *if* there was no data to
be
> > > written to the file.  In this case file.Close will be implicitly closed by
> > > base::File::~File destructor (after sending
> > > FrameHostMsg_SerializeAsMHTMLResponse), but this is okay - when no data is
> > > written, the browser doesn't need to wait for the file flush/close to
> complete
> > > (wait before giving the file handle to the next frame / renderer).
> > > 
> > > Is this accurate?
> > 
> > Yes, that was my assumption. I considered closing before this call here but
as
> > no data would have been written to disk it seemed unnecessary. 
> > 
> > But this is indeed an assumption, so maybe I should add it just to be sure.
> > WDYT?
> 
> I am okay with not calling file.Close if no data is written.  I think adding a
> comment explaining this might be useful to future maintainers.  For a moment I
> wondered whether the file might be opened in exclusive mode (i.e. not passing
> FILE_SHARE_WRITE or anything like this on Windows), but we go through the
> C/POSIX open call, so it doesn't seem to be the case.
> 
> OTOH, I wonder what is a performance impact of calling file.Close on a file
> handle that wasn't written to.  If the impact is small, then maybe we can
insert
> a call to file.Close before calling OnWriteMHTMLToDiskComplete.  This might
make
> the code easier to understand.  Also, I am not sure why
> ThreadRestrictions::AssertIOAllowed() didn't shout before you've moved file IO
> to the separate thread - maybe calling file.Close would make it again slightly
> more difficult to turn on enforcement of AssertIOAllowed.
> 
> As I said - I am fine with the CL either way (1) land as-is + comment
explaining
> why file.Close can be called *after* IPC OR 2) add a call to file.Close before
> calling OnWriteMHTMLToDiskComplete).

I chose to add the close call as it seem clearer for future maintainers. If IO
on the main thread should become forbidden at some point it would require a
different set of changes (the file object would have to be created by the file
thread for one). I'm not expecting this to be an expensive call given there is
no need for flushing caches.

[carlosk, 2016-11-09 19:16:32]

The CQ bit was checked by carlosk@chromium.org

[carlosk, 2016-11-09 19:16:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
lukasza@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2490853002/#ps20001
(title: "Adding file close call when bypassing the thread hop.")

[commit-bot: I haz the power, 2016-11-09 19:17:08]

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-11-09 20:42:44]

Description was changed from

==========
Small tracing improvements to MHTML generation code.

The main objective of this change is to add tracing to the
RenderFrameImpl::OnWriteMHTMLToDiskComplete so that one can see in trace
report the delay introduced by the two thread hops for doing IO in the
file thread: from the "main" to the "file" and then back to "main".

This lead to a few related changes:
- The thread hops are avoided when not actually needed.
- As a consequence the WriteMHTMLToDisk function was slightly simplified.
- And a few outdated code comments were updated.

BUG=645686
==========

to

==========
Small tracing improvements to MHTML generation code.

The main objective of this change is to add tracing to the
RenderFrameImpl::OnWriteMHTMLToDiskComplete so that one can see in trace
report the delay introduced by the two thread hops for doing IO in the
file thread: from the "main" to the "file" and then back to "main".

This lead to a few related changes:
- The thread hops are avoided when not actually needed.
- As a consequence the WriteMHTMLToDisk function was slightly simplified.
- And a few outdated code comments were updated.

BUG=645686
==========

[commit-bot: I haz the power, 2016-11-09 20:42:47]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-09 21:00:12]

Description was changed from

==========
Small tracing improvements to MHTML generation code.

The main objective of this change is to add tracing to the
RenderFrameImpl::OnWriteMHTMLToDiskComplete so that one can see in trace
report the delay introduced by the two thread hops for doing IO in the
file thread: from the "main" to the "file" and then back to "main".

This lead to a few related changes:
- The thread hops are avoided when not actually needed.
- As a consequence the WriteMHTMLToDisk function was slightly simplified.
- And a few outdated code comments were updated.

BUG=645686
==========

to

==========
Small tracing improvements to MHTML generation code.

The main objective of this change is to add tracing to the
RenderFrameImpl::OnWriteMHTMLToDiskComplete so that one can see in trace
report the delay introduced by the two thread hops for doing IO in the
file thread: from the "main" to the "file" and then back to "main".

This lead to a few related changes:
- The thread hops are avoided when not actually needed.
- As a consequence the WriteMHTMLToDisk function was slightly simplified.
- And a few outdated code comments were updated.

BUG=645686

Committed: https://crrev.com/d9ff2b43b1dc1fba22fd16a913c979de09687223
Cr-Commit-Position: refs/heads/master@{#431027}
==========

[commit-bot: I haz the power, 2016-11-09 21:00:12]

Patchset 2 (id:??) landed as
https://crrev.com/d9ff2b43b1dc1fba22fd16a913c979de09687223
Cr-Commit-Position: refs/heads/master@{#431027}