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

third_party/WebKit/Source/web/WebFrameSerializer.cpp

 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 47 matching lines @@
 #include "public/platform/WebVector.h"
 #include "public/web/WebDataSource.h"
 #include "public/web/WebDocument.h"
 #include "public/web/WebFrame.h"
 #include "public/web/WebFrameSerializerCacheControlPolicy.h"
 #include "public/web/WebFrameSerializerClient.h"
 #include "web/WebFrameSerializerImpl.h"
 #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"
 #include "wtf/Vector.h"
 #include "wtf/text/StringConcatenate.h"
 
 namespace blink {
 
 namespace {
 
@@ skipping 200 matching lines @@
   // Translate arguments from public to internal blink APIs.
   LocalFrame* frame = toWebLocalFrameImpl(webFrame)->frame();
   MHTMLArchive::EncodingPolicy encodingPolicy =
       webDelegate->useBinaryEncoding()
           ? MHTMLArchive::EncodingPolicy::UseBinaryEncoding
           : MHTMLArchive::EncodingPolicy::UseDefaultEncoding;
 
   // Serialize.
   TRACE_EVENT_BEGIN0("page-serialization",
                      "WebFrameSerializer::generateMHTMLParts serializing");
-  Vector<SerializedResource> resources;
+  Deque<SerializedResource> resources;
   {
     SCOPED_BLINK_UMA_HISTOGRAM_TIMER(
         "PageSerialization.MhtmlGeneration.SerializationTime.SingleFrame");
     MHTMLFrameSerializerDelegate coreDelegate(*webDelegate);
     FrameSerializer serializer(resources, coreDelegate);
     serializer.serializeFrame(*frame);
   }
   TRACE_EVENT_END1("page-serialization",
                    "WebFrameSerializer::generateMHTMLParts serializing",
                    "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();
 }
 
 WebThreadSafeData WebFrameSerializer::generateMHTMLFooter(
     const WebString& boundary) {
   TRACE_EVENT0("page-serialization", "WebFrameSerializer::generateMHTMLFooter");
   RefPtr<RawData> buffer = RawData::create();
   MHTMLArchive::generateMHTMLFooter(boundary, *buffer->mutableData());
@@ skipping 30 matching lines @@
     const WebString& baseTarget) {
   // TODO(yosin) We should call |FrameSerializer::baseTagDeclarationOf()|.
   if (baseTarget.isEmpty())
     return String("<base href=\".\">");
   String baseString = "<base href=\".\" target=\"" +
                       static_cast<const String&>(baseTarget) + "\">";
   return baseString;
 }
 
 }  // namespace blink