Persist dynamically learned SPDY settings (like CWND).

Persist dynamically learned SPDY settings (like CWND).

BUG=70652
R=willchan
TEST=network unittests, browser unit tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110143

[ramant (doing other things), 2011-11-02 17:10:50]

Hi willchan,
  This is the work-in-progress. Locally, I am done with plumbing changes. The
following is how the data got persisted in my Preferences file.

  Will submit for a full review later. Would appreciate if you could take a high
level review.

thanks,
raman


  "spdy": {
      "settings": {
         "clients1.google.com:443": [ {
            "flags": 2,
            "id": 4,
            "value": 100
         } ],
         "clients4.google.com:443": [ {
            "flags": 2,
            "id": 5,
            "value": 32
         }, {
            "flags": 2,
            "id": 6,
            "value": 0
         } ],
...
      }
   },

[willchan no longer on Chromium, 2011-11-03 20:49:00]

What happens when a SPDY server doesn't send a SETTINGS frame? Do we not cache
that the server supports SPDY?

On 2011/11/02 17:10:50, ramant wrote:
> Hi willchan,
>   This is the work-in-progress. Locally, I am done with plumbing changes. The
> following is how the data got persisted in my Preferences file.
> 
>   Will submit for a full review later. Would appreciate if you could take a
high
> level review.
> 
> thanks,
> raman
> 
> 
>   "spdy": {
>       "settings": {
>          "clients1.google.com:443": [ {
>             "flags": 2,
>             "id": 4,
>             "value": 100
>          } ],
>          "clients4.google.com:443": [ {
>             "flags": 2,
>             "id": 5,
>             "value": 32
>          }, {
>             "flags": 2,
>             "id": 6,
>             "value": 0
>          } ],
> ...
>       }
>    },

[ramant (doing other things), 2011-11-04 17:30:31]

On 2011/11/03 20:49:00, willchan wrote:
> What happens when a SPDY server doesn't send a SETTINGS frame? Do we not cache
> that the server supports SPDY?

SetSpdySettings doesn't do anything (we could make a small change to return true
or false to indicate if we need to update prefs or not).

Currently, we don't change "server supports SPDY" property in the above case.
Should we change that property? thanks.

[ramant (doing other things), 2011-11-04 17:32:55]

On 2011/11/04 17:30:31, ramant wrote:
> On 2011/11/03 20:49:00, willchan wrote:
> > What happens when a SPDY server doesn't send a SETTINGS frame? Do we not
cache
> > that the server supports SPDY?
> 
> SetSpdySettings doesn't do anything (we could make a small change to return
true
> or false to indicate if we need to update prefs or not).
> 
> Currently, we don't change "server supports SPDY" property in the above case.
> Should we change that property? thanks.

What do you think of adding an enum to ScheduleUpdateSpdyPrefsOnIO to specify
which property it should set (if it is supportspdy or spdy settings), thus
avoiding unnecessary work.

[ramant (doing other things), 2011-11-04 18:26:23]

On 2011/11/04 17:32:55, ramant wrote:

> What do you think of adding an enum to ScheduleUpdateSpdyPrefsOnIO to specify
> which property it should set (if it is supportspdy or spdy settings), thus
> avoiding unnecessary work.

Please ignore the above suggestion. Should we have separate timers and update
methods for SpdySettings? (combined updating of SpdyServer and SpdySettings
properties into one set of methods). thanks.

[ramant (doing other things), 2011-11-04 18:48:37]

Hi willchan,
  Made the changes not to persist spdy settings if we heavn't updates the prefs
in the cache.

[willchan no longer on Chromium, 2011-11-04 22:22:07]

On 2011/11/04 18:48:37, ramant wrote:
> Hi willchan,
>   Made the changes not to persist spdy settings if we heavn't updates the
prefs
> in the cache.

I sort of feel we should restructure our prefs. It should perhaps be named
something like http_server_properties. Then we'd have a dictionary for each
server. For each server, we'd have a dictionary of key/values. supports_spdy,
spdy_settings, etc. would be keys. What do you think? I'll go review everything
else while we discuss this.

[ramant (doing other things), 2011-11-04 22:40:22]

On 2011/11/04 22:22:07, willchan wrote:
> On 2011/11/04 18:48:37, ramant wrote:
> > Hi willchan,
> >   Made the changes not to persist spdy settings if we heavn't updates the
> prefs
> > in the cache.
> 
> I sort of feel we should restructure our prefs. It should perhaps be named
> something like http_server_properties. Then we'd have a dictionary for each
> server. For each server, we'd have a dictionary of key/values. supports_spdy,
> spdy_settings, etc. would be keys. What do you think? I'll go review
everything
> else while we discuss this.

The above sounds great to me.

[willchan no longer on Chromium, 2011-11-04 22:44:38]

The change you made in profile_impl_io_data.h should probably also be reflected
in off_the_record_profile_io_data.h.

I read everything other than chrome/browser/net/ changes, since those will
change due to our now agreed upon refactor of how we store the preferences. When
you get those done, let me know. Everything else seems great.

[ramant (doing other things), 2011-11-05 02:57:20]

On 2011/11/04 22:22:07, willchan wrote:
> On 2011/11/04 18:48:37, ramant wrote:
> > Hi willchan,
> >   Made the changes not to persist spdy settings if we heavn't updates the
> prefs
> > in the cache.
> 
> I sort of feel we should restructure our prefs. It should perhaps be named
> something like http_server_properties. Then we'd have a dictionary for each
> server. For each server, we'd have a dictionary of key/values. supports_spdy,
> spdy_settings, etc. would be keys. What do you think? I'll go review
everything
> else while we discuss this.
Hi willchan,
  What do you think of the following for spdy preferences?

thanks,
raman

   "spdy": {
      "servers": [
         "www.google.com:443": {
            "supports_spdy": 1,
            "settings": [ {
               "id": 5,
               "value": 32
            }, {
               "id": 6,
               "value": 0
            } ],
            "alternate-protocol": {
            },
         },
         "plus.google.com:443": {
         },
      ],
      other non-server properties,
   }

[ramant (doing other things), 2011-11-05 03:07:59]

Another option is to keep alternate-protocol props separate:

   "spdy": {
      "servers": {
         "www.google.com:443": {
            "supports_spdy": 1,
            "settings": [ {
               "id": 5,
               "value": 32
            }, {
               "id": 6,
               "value": 0
            } ],
         },
         "plus.google.com:443": {
         },
      },
      "alternate-protocol": {
         "www.google.com:443": {
            port: 1233,
            protocol: ...
         }
      },
   }

[ramant (doing other things), 2011-11-05 03:21:29]

One more options is

   "http_server_properties": {
      "spdy": {
         "www.google.com:443": {
            "supports_spdy": 1,
            "settings": [ {
               "id": 5,
               "value": 32
            }, {
               "id": 6,
               "value": 0
            } ],
         },
         "plus.google.com:443": {
         },
      },
      "alternate-protocol": {
         "www.google.com:443": {
            port: 1233,
            protocol: ...
         }
      },

[ramant (doing other things), 2011-11-07 23:13:25]

The following is another option:

   "http_server_properties": {

      "www.google.com:443": {

         "supports_spdy": 1,

         "settings": [
            {
               "id": 5,
               "value": 32
            },
            {
               "id": 6,
               "value": 0
            },
         ],

         "alternate-protocol": {
            port: 1233,
            protocol: ...
         },
      },

      "plus.google.com:443": {
...
      },
   }

[willchan no longer on Chromium, 2011-11-07 23:15:07]

I like this.

On Mon, Nov 7, 2011 at 3:13 PM, <rtenneti@chromium.org> wrote:

> The following is another option:
>
>   "http_server_properties": {
>
>
>      "www.google.com:443": {
>
>         "supports_spdy": 1,
>
>         "settings": [
>            {
>               "id": 5,
>               "value": 32
>            },
>            {
>               "id": 6,
>               "value": 0
>            },
>         ],
>
>         "alternate-protocol": {
>
>            port: 1233,
>            protocol: ...
>         },
>      },
>
>      "plus.google.com:443": {
> ...
>      },
>   }
>
>
>
>
http://codereview.chromium.**org/8423028/<http://codereview.chromium.org/8423...
>

[ramant (doing other things), 2011-11-08 01:14:20]

hi willchan,
  I am thinking of adding the following structure/map to
http_server_properties_impl.h and whenever we persist the data, will write the
following structure. Will be doing a small surgery to the code. 

  The cost with this approach is whenever a server's props change, we need to
flatten the following map and set that as http_properties_prefs as Pref.

  Hope this approach is ok.

thanks,
raman

  struct ServerPref {
    bool supports_spdy;
    spdy::SpdySettings* settings;  
    PortAlternateProtocolPair* alternate_prootocol;

  };
  typedef base::hash_map<HostPortPair, ServerPref> ServerPrefMap;
  ServerPrefMap server_pref_map_;

[ramant (doing other things), 2011-11-10 01:16:50]

Hi willchan,
  Would appreciate if you could take a look at HttpServerPropertiesManager*.*
They are the only files, I had changed since the last review (tried the
valgrind, asan trybots in Patch Set 10).

thanks,
raman

[willchan no longer on Chromium, 2011-11-10 19:00:59]

I'm going to finish this up this afternoon. Ping me if I haven't gotten to it.
Here's a minor nit first.

http://codereview.chromium.org/8423028/diff/35001/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager.h (right):

http://codereview.chromium.org/8423028/diff/35001/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.h:127: struct
RefCountedListValue
Now that base::Owned() is available, please use it instead of all these
refcounted objects.

[ramant (doing other things), 2011-11-10 19:06:33]

hi willchan,
  Fixed the valgrind errors and added tests for multiple servers.

thanks,
raman

[willchan no longer on Chromium, 2011-11-11 01:46:54]

Sorry, I didn't get to finish this. The new changes seem very reasonable to me.
I'll finish this tomorrow.

[ramant (doing other things), 2011-11-11 02:20:57]

On 2011/11/11 01:46:54, willchan wrote:
> Sorry, I didn't get to finish this. The new changes seem very reasonable to
me.
> I'll finish this tomorrow.

No problem. Fixed couple of bugs in the unittests to fix leaks caught by
valgrind.

thanks much,
raman

[willchan no longer on Chromium, 2011-11-11 23:00:01]

Changes seem quite reasonable! I'm just waiting on the base::Owned changes now.

http://codereview.chromium.org/8423028/diff/38027/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager_unittest.cc (right):

http://codereview.chromium.org/8423028/diff/38027/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager_unittest.cc:219: EXPECT_EQ(1U,
spdy_settings_ret.size());
ASSERT_EQ

http://codereview.chromium.org/8423028/diff/38027/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager_unittest.cc:278: EXPECT_EQ(1U,
spdy_settings_ret.size());
ASSERT_EQ, otherwise the next line may crash.

[ramant (doing other things), 2011-11-12 07:08:42]

Hi willchan,
  Made the changes you had suggested. Would appreciate if you could take a look
Owned calls if I have used them properly or not.

thanks very much,
raman

http://codereview.chromium.org/8423028/diff/38027/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager_unittest.cc (right):

http://codereview.chromium.org/8423028/diff/38027/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager_unittest.cc:219: EXPECT_EQ(1U,
spdy_settings_ret.size());
On 2011/11/11 23:00:01, willchan wrote:
> ASSERT_EQ

Done.

http://codereview.chromium.org/8423028/diff/38027/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager_unittest.cc:278: EXPECT_EQ(1U,
spdy_settings_ret.size());
On 2011/11/11 23:00:01, willchan wrote:
> ASSERT_EQ, otherwise the next line may crash.

Done.

[willchan no longer on Chromium, 2011-11-15 15:44:21]

lgtm

http://codereview.chromium.org/8423028/diff/46003/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager.cc (right):

http://codereview.chromium.org/8423028/diff/46003/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:162: void
HttpServerPropertiesManager::ClearSpdySettings() {
DCHECK(BrowserThread...)?

http://codereview.chromium.org/8423028/diff/46003/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:218: VLOG(1) << "Malformed
http_server_properties for server: " << server_str;
There's been a lot of cracking down on LOGs. Can you use DVLOG instead? That way
this won't bloat the official builds.

http://codereview.chromium.org/8423028/diff/46003/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager.h (right):

http://codereview.chromium.org/8423028/diff/46003/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.h:203:
io_spdy_prefs_update_timer_;
Maybe this should be renamed io_prefs_update_timer_. Also update the comment.

[ramant (doing other things), 2011-11-15 16:43:46]

thanks very much willchan. Made all the changes.
-raman

http://codereview.chromium.org/8423028/diff/46003/chrome/browser/net/http_ser...
File chrome/browser/net/http_server_properties_manager.cc (right):

http://codereview.chromium.org/8423028/diff/46003/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:162: void
HttpServerPropertiesManager::ClearSpdySettings() {
On 2011/11/15 15:44:21, willchan wrote:
> DCHECK(BrowserThread...)?

Done.

http://codereview.chromium.org/8423028/diff/46003/chrome/browser/net/http_ser...
chrome/browser/net/http_server_properties_manager.cc:218: VLOG(1) << "Malformed
http_server_properties for server: " << server_str;
On 2011/11/15 15:44:21, willchan wrote:
> There's been a lot of cracking down on LOGs. Can you use DVLOG instead? That
way
> this won't bloat the official builds.

Done.

[commit-bot: I haz the power, 2011-11-15 16:44:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/8423028/51001

[commit-bot: I haz the power, 2011-11-15 19:20:34]

Change committed as 110143