SPDY - persist SPDY settings.

net/spdy/spdy_session.cc

Index: net/spdy/spdy_session.cc
===================================================================
--- net/spdy/spdy_session.cc	(revision 127896)
+++ net/spdy/spdy_session.cc	(working copy)
@@ -123,6 +123,33 @@
   DISALLOW_COPY_AND_ASSIGN(NetLogSpdySessionParameter);
 };
 
+class NetLogSpdySettingParameter : public NetLog::EventParameters {
+ public:
+  explicit NetLogSpdySettingParameter(spdy::SpdySettingsIds id,
+                                      uint8 flags,
+                                      uint32 value)
+      : id_(id),
+        flags_(flags),
+        value_(value) {
+  }
+
+  virtual Value* ToValue() const {
+    DictionaryValue* dict = new DictionaryValue();
+    dict->SetInteger("id", id_);
+    dict->SetInteger("flags", flags_);
+    dict->SetInteger("value", value_);
+    return dict;
+  }
+
+ private:
+  ~NetLogSpdySettingParameter() {}
+  const spdy::SpdySettingsIds id_;
+  const uint8 flags_;
+  const uint32 value_;
+
+  DISALLOW_COPY_AND_ASSIGN(NetLogSpdySettingParameter);
+};
+
 class NetLogSpdySettingsParameter : public NetLog::EventParameters {
  public:
   explicit NetLogSpdySettingsParameter(const spdy::SpdySettings& settings)
@@ -1332,19 +1359,17 @@
                             uint8 flags,
                             uint32 value) {
   HandleSetting(id, value);
-  spdy::SettingsFlagsAndId flags_and_id(flags, id);
-  // TODO(rtenneti): persist SpdySetting.
-  // http_server_properties_->SetSpdySetting(
-  //    host_port_pair(), std::make_pair(flags_and_id, value));
 
+  spdy::SettingsFlagsAndValue flags_and_value(flags, value);
+  http_server_properties_->SetSpdySetting(
+      host_port_pair(), id, flags_and_value);

[Ryan Hamilton, 2012/03/21 16:24:41]

This API seems a bit suboptimal.  instead of taking flags_and_value which
required to caller to construct them, could we change SetSpdySetting to tkae all
three?

[ramant (doing other things), 2012/03/23 04:11:25]

Done.

+
   received_settings_ = true;
 
-  // Log the settings.
-  spdy::SpdySettings settings;
-  settings.insert(settings.end(), std::make_pair(flags_and_id, value));
+  // Log the setting.
   net_log_.AddEvent(

[Ryan Hamilton, 2012/03/21 16:24:41]

Hm.  This means that we'll end up with 1 log entry for each setting in a
settings frame?  That seems a bit annoying, though not the end of the world.  It
does seem like live would be easier if buffered spdy framer buffered up the
whole settings frame.  perhaps in another cl.

[ramant (doing other things), 2012/03/23 04:11:25]

+1. Will create a bug for buffering.

[Ryan Hamilton, 2012/03/23 15:25:38]

Thanks.

-      NetLog::TYPE_SPDY_SESSION_RECV_SETTINGS,
-      make_scoped_refptr(new NetLogSpdySettingsParameter(settings)));
+      NetLog::TYPE_SPDY_SESSION_RECV_SETTING,
+      make_scoped_refptr(new NetLogSpdySettingParameter(id, flags, value)));
 }
 
 bool SpdySession::Respond(const spdy::SpdyHeaderBlock& headers,
@@ -1678,72 +1703,59 @@
 }
 
 void SpdySession::SendSettings() {
-  // Note:  we're copying the settings here, so that we can potentially modify
-  // the settings for the field trial.  When removing the field trial, make
-  // this a reference to the const SpdySettings again.
-  spdy::SpdySettings settings =
+  // Note: we can potentially modify the settings for the field trial.

[Ryan Hamilton, 2012/03/21 16:24:41]

I think I would remove this comment.  It doesn't seem to add much value, since
the field trial code is 4 lines below.

[ramant (doing other things), 2012/03/23 04:11:25]

Done.

+  const spdy::SettingsMap& settings_map =
       http_server_properties_->GetSpdySettings(host_port_pair());
-  if (settings.empty())
+  if (settings_map.empty())
     return;
 
-  typedef std::map<uint32, spdy::SpdySetting> SpdySettingsMap;
-  SpdySettingsMap unique_settings;
-
   // Record Histogram Data and Apply the SpdyCwnd FieldTrial if applicable.
-  for (spdy::SpdySettings::iterator i = settings.begin(),
-           end = settings.end(); i != end; ++i) {
-    const uint32 id = i->first.id();
-    const uint32 val = i->second;
+  for (spdy::SettingsMap::const_iterator i = settings_map.begin(),

[Ryan Hamilton, 2012/03/21 16:24:41]

Since this is a map, I don't think you need to loop over all the elements. 
Instead, you could simply look to see if the map has an entry for
spdy::SETTINGS_CURRENT_CWND.  If it does, then check to see if the value is the
expected value.  If not, set it to the new value and re-persist.

[ramant (doing other things), 2012/03/23 04:11:25]

Thanks for the above.

Done.

+           end = settings_map.end(); i != end; ++i) {
+    const uint32 id = i->first;

[Ryan Hamilton, 2012/03/21 16:24:41]

nit: should the type of id be spdy::SpdySettingsIds?

[ramant (doing other things), 2012/03/23 04:11:25]

Done.

+    const uint32 val = i->second.second;
     switch (id) {
       case spdy::SETTINGS_CURRENT_CWND:
         uint32 cwnd = 0;
         cwnd = ApplyCwndFieldTrialPolicy(val);
         UMA_HISTOGRAM_CUSTOM_COUNTS("Net.SpdySettingsCwndSent",
-                                    cwnd,
-                                    1, 200, 100);
+                                    cwnd, 1, 200, 100);
         if (cwnd != val) {
-          spdy::SettingsFlagsAndId new_id(spdy::SETTINGS_FLAG_PLEASE_PERSIST,
-                                          id);
-          i->second = cwnd;
-          i->first = new_id;
-          spdy::SpdySetting setting(new_id, val);
-          // TODO(rtenneti): Persist SpdySetting.
-          // http_server_properties_->SetSpdySetting(host_port_pair(), setting);
-          unique_settings[id] = setting;
-          continue;
+          spdy::SettingsFlagsAndValue flags_and_value(
+              spdy::SETTINGS_FLAG_PLEASE_PERSIST, cwnd);
+          http_server_properties_->SetSpdySetting(
+              host_port_pair(), id, flags_and_value);
         }
+        break;
     }
-    unique_settings[id] = *i;
   }
 
-  HandleSettings(settings);
+  const spdy::SettingsMap& settings_map_new =
+      http_server_properties_->GetSpdySettings(host_port_pair());
 
+  spdy::SpdySettings settings;

[Ryan Hamilton, 2012/03/21 16:24:41]

Oh, here we're using SpdySettings as a list.  How come?

[ramant (doing other things), 2012/03/23 04:11:25]

Would like to a submit a separate CL to delete all uses of spdy::SpdySettings
from our code and the common code with server. 

SpdyFramer's CreateSettings takes SpdySettings. Will file a bug (TODO) to delete
spdy::SpdySettings and use spdy::SettingsMap everywhere.

Hope that is ok.

[Ryan Hamilton, 2012/03/23 15:25:38]

Sounds good.

+  for (spdy::SettingsMap::const_iterator i = settings_map_new.begin(),
+           end = settings_map_new.end(); i != end; ++i) {
+    const uint32 id = i->first;
+    const uint8 flags = i->second.first;
+    const uint32 val = i->second.second;
+    HandleSetting(id, val);
+    spdy::SettingsFlagsAndId flags_and_id(flags, id);
+    settings.push_back(spdy::SpdySetting(flags_and_id, val));
+  }
+
   net_log_.AddEvent(
       NetLog::TYPE_SPDY_SESSION_SEND_SETTINGS,
       make_scoped_refptr(new NetLogSpdySettingsParameter(settings)));
 
-  spdy::SpdySettings sorted_settings;
-  for (SpdySettingsMap::iterator it = unique_settings.begin();
-       unique_settings.end() != it;
-       ++it) {
-    sorted_settings.push_back(it->second);
-  }
-
   // Create the SETTINGS frame and send it.
   DCHECK(buffered_spdy_framer_.get());
   scoped_ptr<spdy::SpdySettingsControlFrame> settings_frame(
-      buffered_spdy_framer_->CreateSettings(sorted_settings));
+      buffered_spdy_framer_->CreateSettings(settings));
   sent_settings_ = true;
   QueueFrame(settings_frame.get(), 0, NULL);
 }
 
-void SpdySession::HandleSettings(const spdy::SpdySettings& settings) {
-  for (spdy::SpdySettings::const_iterator i = settings.begin(),
-           end = settings.end(); i != end; ++i) {
-    HandleSetting(i->first.id(), i->second);
-  }
-}
-
 void SpdySession::HandleSetting(uint32 id, uint32 value) {
   switch (id) {
     case spdy::SETTINGS_MAX_CONCURRENT_STREAMS:
@@ -1897,35 +1909,31 @@
 
   if (received_settings_) {
     // Enumerate the saved settings, and set histograms for it.
-    const spdy::SpdySettings& settings =
+    const spdy::SettingsMap& settings_map =
         http_server_properties_->GetSpdySettings(host_port_pair());
 
-    spdy::SpdySettings::const_iterator it;
-    for (it = settings.begin(); it != settings.end(); ++it) {
-      const spdy::SpdySetting setting = *it;
-      switch (setting.first.id()) {
+    spdy::SettingsMap::const_iterator it;
+    for (it = settings_map.begin(); it != settings_map.end(); ++it) {
+      const uint32 id = it->first;
+      const uint32 val = it->second.second;

[Ryan Hamilton, 2012/03/21 16:24:41]

Nice cleanup.  Much more readable.

nit: should id's type be spdy::SpdySettingsIds instead?

[ramant (doing other things), 2012/03/23 04:11:25]

Done.

+      switch (id) {
         case spdy::SETTINGS_CURRENT_CWND:
           // Record several different histograms to see if cwnd converges
           // for larger volumes of data being sent.
           UMA_HISTOGRAM_CUSTOM_COUNTS("Net.SpdySettingsCwnd",
-                                      setting.second,
-                                      1, 200, 100);
+                                      val, 1, 200, 100);
           if (bytes_received_ > 10 * 1024) {
             UMA_HISTOGRAM_CUSTOM_COUNTS("Net.SpdySettingsCwnd10K",
-                                        setting.second,
-                                        1, 200, 100);
+                                        val, 1, 200, 100);
             if (bytes_received_ > 25 * 1024) {
               UMA_HISTOGRAM_CUSTOM_COUNTS("Net.SpdySettingsCwnd25K",
-                                          setting.second,
-                                          1, 200, 100);
+                                          val, 1, 200, 100);
               if (bytes_received_ > 50 * 1024) {
                 UMA_HISTOGRAM_CUSTOM_COUNTS("Net.SpdySettingsCwnd50K",
-                                            setting.second,
-                                            1, 200, 100);
+                                            val, 1, 200, 100);
                 if (bytes_received_ > 100 * 1024) {
                   UMA_HISTOGRAM_CUSTOM_COUNTS("Net.SpdySettingsCwnd100K",
-                                              setting.second,
-                                              1, 200, 100);
+                                              val, 1, 200, 100);
                 }
               }
             }
@@ -1933,13 +1941,11 @@
           break;
         case spdy::SETTINGS_ROUND_TRIP_TIME:
           UMA_HISTOGRAM_CUSTOM_COUNTS("Net.SpdySettingsRTT",
-                                      setting.second,
-                                      1, 1200, 100);
+                                      val, 1, 1200, 100);
           break;
         case spdy::SETTINGS_DOWNLOAD_RETRANS_RATE:
           UMA_HISTOGRAM_CUSTOM_COUNTS("Net.SpdySettingsRetransRate",
-                                      setting.second,
-                                      1, 100, 50);
+                                      val, 1, 100, 50);
           break;
       }
     }