[Sync] Finch Experiment : Enable compression between sync client and server

sync/internal_api/http_bridge.cc

Index: sync/internal_api/http_bridge.cc
diff --git a/sync/internal_api/http_bridge.cc b/sync/internal_api/http_bridge.cc
index c64822d5836d2e729d6f7bca00069e5eebbd7c80..0b6467a30485beb546f030e16d19419fc7c5a605 100644
--- a/sync/internal_api/http_bridge.cc
+++ b/sync/internal_api/http_bridge.cc
@@ -5,6 +5,7 @@
 #include "sync/internal_api/public/http_bridge.h"
 
 #include "base/message_loop/message_loop.h"
+#include "base/metrics/field_trial.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
@@ -37,6 +38,49 @@ void LogTimeout(bool timed_out) {
 
 }  // namespace
 
+bool IsSyncHttpContentCompressionEnabled() {
+  const std::string group_name =
+      base::FieldTrialList::FindFullName("SyncHttpContentCompression");
+  return group_name == "Enabled";
+}
+
+bool GzipCompress(std::string source, std::string* dest) {
+  z_stream stream = {0};
+  int result = deflateInit2(&stream, Z_DEFAULT_COMPRESSION, Z_DEFLATED,
+                            // 16 is added to produce a gzip header + trailer.
+                            MAX_WBITS + 16,
+                            8,  // memLevel = 8 is default.
+                            Z_DEFAULT_STRATEGY);
+  DCHECK_EQ(Z_OK, result);
+  size_t out_size = deflateBound(&stream, source.size());
+  scoped_ptr<char[]> out(new char[out_size]);
+
+  stream.next_in = reinterpret_cast<uint8*>(&source[0]);
+  stream.avail_in = source.size();
+  stream.next_out = reinterpret_cast<uint8*>(out.get());
+  stream.avail_out = out_size;
+
+  result = deflate(&stream, Z_FINISH);
+  if (result != Z_STREAM_END) {
+    return false;
+  }
+  out_size = out_size - stream.avail_out;
+
+  result = deflateEnd(&stream);
+  DCHECK_EQ(Z_OK, result);
+
+  dest->assign(out.get(), out_size);
+  return true;
+}
+
+void RecordSyncContentLengthHistograms(int64 this_content_length,

[Nicolas Zea, 2015/07/28 00:27:19]

nit: I think these variables could be named clearer. How about
compressed_content_length and original_content_length?

[Gang Wu, 2015/07/29 21:09:15]

Done.

+                                       int64 this_original_content_length) {
+  // Upload UMA data

[Nicolas Zea, 2015/07/28 00:27:19]

no need for this comment (all UMA histograms result in uploading data
eventually. And to be accurate, this doesn't actually immediately result in the
upload, but simply records the data for later upload).

[Gang Wu, 2015/07/29 21:09:15]

Done.

+  UMA_HISTOGRAM_COUNTS("Sync.ContentLengthCompressed", this_content_length);
+  UMA_HISTOGRAM_COUNTS("Sync.ContentLengthOriginal",
+                       this_original_content_length);
+}
+
 HttpBridgeFactory::HttpBridgeFactory(
     const scoped_refptr<net::URLRequestContextGetter>& request_context_getter,
     const NetworkTimeUpdateCallback& network_time_update_callback,
@@ -203,19 +247,30 @@ void HttpBridge::MakeAsynchronousPost() {
       base::Bind(&HttpBridge::OnURLFetchTimedOut, this));
 
   DCHECK(request_context_getter_.get());
+  fetch_state_.start_time = base::Time::Now();
   fetch_state_.url_poster =
       net::URLFetcher::Create(url_for_request_, net::URLFetcher::POST, this)
           .release();
   fetch_state_.url_poster->SetRequestContext(request_context_getter_.get());
-  fetch_state_.url_poster->SetUploadData(content_type_, request_content_);
   fetch_state_.url_poster->SetExtraRequestHeaders(extra_headers_);
+
+  std::string content_to_be_sent = request_content_;

[Nicolas Zea, 2015/07/28 00:27:19]

I'd rather not pay the cost of a copy on all requests, even when we don't have
the feature enabled. How about pulling the SetUploadData call into the if
statements, and only declaring this variable within the if (IsSync...) body?

It would be something like:
int compressed_content_size = 0;
if (IsSyncHttp...) {
  std::string compress_content;
  GzipCompress(...);
  compressed_content_size = ...
  ...AddExtraRequestHeader...
  ...SetUploadData...)
} else {
  ...
}
RecordSyncContentLengthHistogram(...);

[Gang Wu, 2015/07/29 21:09:15]

Done.

+  if (IsSyncHttpContentCompressionEnabled()) {
+    GzipCompress(request_content_, &content_to_be_sent);
+    fetch_state_.url_poster->AddExtraRequestHeader("Content-Encoding: gzip");
+  } else {
+    fetch_state_.url_poster->AddExtraRequestHeader("Accept-Encoding: deflate");
+  }
+  fetch_state_.url_poster->SetUploadData(content_type_, content_to_be_sent);
+  RecordSyncContentLengthHistograms(content_to_be_sent.size(),
+                                    request_content_.size());
+
   fetch_state_.url_poster->AddExtraRequestHeader(base::StringPrintf(
       "%s: %s", net::HttpRequestHeaders::kUserAgent, user_agent_.c_str()));
   fetch_state_.url_poster->SetLoadFlags(net::LOAD_BYPASS_CACHE |
                                         net::LOAD_DISABLE_CACHE |
                                         net::LOAD_DO_NOT_SAVE_COOKIES |
                                         net::LOAD_DO_NOT_SEND_COOKIES);
-  fetch_state_.start_time = base::Time::Now();
 
   fetch_state_.url_poster->Start();
 }
@@ -315,6 +370,17 @@ void HttpBridge::OnURLFetchComplete(const net::URLFetcher* source) {
   fetch_state_.response_headers = source->GetResponseHeaders();
   UpdateNetworkTime();
 
+  int64 compressed_content_length = fetch_state_.response_content.size();
+  int64 original_content_length = compressed_content_length;
+  if (fetch_state_.response_headers &&
+      fetch_state_.response_headers->HasHeaderValue("content-encoding",
+                                                    "gzip")) {
+    compressed_content_length =
+        fetch_state_.response_headers->GetContentLength();
+  }
+  RecordSyncContentLengthHistograms(compressed_content_length,

[Nicolas Zea, 2015/07/28 00:27:19]

Does it make sense to record the request separately from the response (in
different histograms)?

[Gang Wu, 2015/07/29 21:09:15]

Yes, since request is smaller than response, so separate then is better.

+                                    original_content_length);
+
   // End of the line for url_poster_. It lives only on the IO loop.
   // We defer deletion because we're inside a callback from a component of the
   // URLFetcher, so it seems most natural / "polite" to let the stack unwind.