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

third_party/WebKit/Source/web/WebFrameSerializer.cpp

Index: third_party/WebKit/Source/web/WebFrameSerializer.cpp
diff --git a/third_party/WebKit/Source/web/WebFrameSerializer.cpp b/third_party/WebKit/Source/web/WebFrameSerializer.cpp
index 7b5b8b7b0964efb233eeb50d294f1f7704b4962e..5fdf086b987a4235d190e22fae7cf7de62f0db1f 100644
--- a/third_party/WebKit/Source/web/WebFrameSerializer.cpp
+++ b/third_party/WebKit/Source/web/WebFrameSerializer.cpp
@@ -65,6 +65,7 @@
 #include "web/WebLocalFrameImpl.h"
 #include "web/WebRemoteFrameImpl.h"
 #include "wtf/Assertions.h"
+#include "wtf/Deque.h"
 #include "wtf/HashMap.h"
 #include "wtf/HashSet.h"
 #include "wtf/Noncopyable.h"
@@ -285,7 +286,7 @@ WebThreadSafeData WebFrameSerializer::generateMHTMLParts(
   // Serialize.
   TRACE_EVENT_BEGIN0("page-serialization",
                      "WebFrameSerializer::generateMHTMLParts serializing");
-  Vector<SerializedResource> resources;
+  Deque<SerializedResource> resources;
   {
     SCOPED_BLINK_UMA_HISTOGRAM_TIMER(
         "PageSerialization.MhtmlGeneration.SerializationTime.SingleFrame");
@@ -298,26 +299,23 @@ WebThreadSafeData WebFrameSerializer::generateMHTMLParts(
                    "resource count",
                    static_cast<unsigned long long>(resources.size()));
 
-  // Get Content-ID for the frame being serialized.
-  String frameContentID = webDelegate->getContentID(webFrame);
-
-  // Encode serializer's output as MHTML.
+  // Encode serialized resources as MHTML.
   RefPtr<RawData> output = RawData::create();
   {
+    DCHECK(!resources.isEmpty());
     SCOPED_BLINK_UMA_HISTOGRAM_TIMER(
         "PageSerialization.MhtmlGeneration.EncodingTime.SingleFrame");
-    bool isFirstResource = true;
-    for (const SerializedResource& resource : resources) {
+    // Frame is the 1st resource (see FrameSerializer::serializeFrame doc
+    // comment). Frames get a Content-ID header.
+    MHTMLArchive::generateMHTMLPart(
+        boundary, webDelegate->getContentID(webFrame), encodingPolicy,
+        resources.takeFirst(), *output->mutableData());

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

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.

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

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]

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]

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

+    while (!resources.isEmpty()) {
       TRACE_EVENT0("page-serialization",
                    "WebFrameSerializer::generateMHTMLParts encoding");
-      // Frame is the 1st resource (see FrameSerializer::serializeFrame doc
-      // comment). Frames get a Content-ID header.
-      String contentID = isFirstResource ? frameContentID : String();
-
-      MHTMLArchive::generateMHTMLPart(boundary, contentID, encodingPolicy,
-                                      resource, *output->mutableData());
-
-      isFirstResource = false;
+      MHTMLArchive::generateMHTMLPart(boundary, String(), encodingPolicy,
+                                      resources.takeFirst(),
+                                      *output->mutableData());
     }
   }
   return output.release();