Re: ServerPushDiscovery: Session and Learner classses in common. (issue970001)

[mdst...@google.com]

Sorry to be a bit slow to respond. Mostly nits left; I'm starting to
get a better handle on how this all works, thanks for the comments
already added.


http://page-speed-codereview.appspot.com/970001/diff/70001/mod_spdy/common/server_push_discovery_learner.cc
File mod_spdy/common/server_push_discovery_learner.cc (right):

http://page-speed-codereview.appspot.com/970001/diff/70001/mod_spdy/common/server_push_discovery_learner.cc#newcode55
mod_spdy/common/server_push_discovery_learner.cc:55:
std::sort(significant_adjacents.begin(), significant_adjacents.end());
Can you add a comment explaining why sorting by average_time_from_init
is desirable here?

http://page-speed-codereview.appspot.com/970001/diff/70001/mod_spdy/common/server_push_discovery_learner.cc#newcode84
mod_spdy/common/server_push_discovery_learner.cc:84:
std::pair<std::map<std::string, AdjacentData>::iterator, bool> insertion
=
Something about this code makes me uncomfortable. It looks wrong to me.
On closer inspection I think it's actually right, but maybe it could be
made clearer somehow?

http://page-speed-codereview.appspot.com/970001/diff/70001/mod_spdy/common/server_push_discovery_learner.cc#newcode91
mod_spdy/common/server_push_discovery_learner.cc:91: double
inv_new_total = 1.0 / adjacent_data.hit_count;
Style nit "Eschew abbreviation". How about const double
inverse_hit_count?

http://page-speed-codereview.appspot.com/970001/diff/70001/mod_spdy/common/server_push_discovery_learner.h
File mod_spdy/common/server_push_discovery_learner.h (right):

http://page-speed-codereview.appspot.com/970001/diff/70001/mod_spdy/common/server_push_discovery_learner.h#newcode45
mod_spdy/common/server_push_discovery_learner.h:45: std::vector<Push>
GetPushes(const std::string& master_url);
Doc comment?

http://page-speed-codereview.appspot.com/970001/diff/70001/mod_spdy/common/server_push_discovery_learner.h#newcode52
mod_spdy/common/server_push_discovery_learner.h:52: // a master request
received earlier.
Can you clarify in the doc comment what time_from_init represents?

http://page-speed-codereview.appspot.com/970001/diff/70001/mod_spdy/common/server_push_discovery_learner.h#newcode61
mod_spdy/common/server_push_discovery_learner.h:61:
avg_time_from_init(0) {
Nit: s/avg/average/ please

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#General_Naming_Rules
"eschew abbreviation"

http://page-speed-codereview.appspot.com/970001/diff/70001/mod_spdy/common/server_push_discovery_learner.h#newcode68
mod_spdy/common/server_push_discovery_learner.h:68: bool operator<(const
AdjacentData& other) const {
I don't think I really like having operator< on this struct, especially
because it's not obvious why it should sort on avg_time_from_init rather
than any other field(s). It looks like this is being used for the call
to sort() in the .cc file; can you instead use the 3-argument form of
sort, using a comparator function declared within the .cc file?

http://page-speed-codereview.appspot.com/970001/diff/70001/mod_spdy/common/server_push_discovery_learner.h#newcode77
mod_spdy/common/server_push_discovery_learner.h:77:
std::map<std::string, AdjacentData> adjcaents;
spelling

http://page-speed-codereview.appspot.com/970001/diff/70001/mod_spdy/common/server_push_discovery_learner_test.cc
File mod_spdy/common/server_push_discovery_learner_test.cc (right):

http://page-speed-codereview.appspot.com/970001/diff/70001/mod_spdy/common/server_push_discovery_learner_test.cc#newcode39
mod_spdy/common/server_push_discovery_learner_test.cc:39:
TEST(ServerPushDiscoveryLearnerTest, TrivialYesPush) {
Can we add a test that results in multiple pushes for a URL, and verify
that the come back in the right order? In particular to test that the
calculation done to maintain average_time_from_init is correct.

http://page-speed-codereview.appspot.com/970001/diff/70001/mod_spdy/common/server_push_discovery_session.h
File mod_spdy/common/server_push_discovery_session.h (right):

http://page-speed-codereview.appspot.com/970001/diff/70001/mod_spdy/common/server_push_discovery_session.h#newcode28
mod_spdy/common/server_push_discovery_session.h:28: typedef int64_t
SessionId;
At the top level, the name "SessionId" is ambiguous (it might seem like
it refers to a SpdySession). Can we either change the name to
disambiguate, or perhaps better, move it inside one of the classes
below?

http://page-speed-codereview.appspot.com/970001/diff/70001/mod_spdy/common/server_push_discovery_session.h#newcode46
mod_spdy/common/server_push_discovery_session.h:46: // Creates a new
session. |took_push| denotes if the the initial request
I'm still a little confused what took_push is. Maybe I'm just finding
the name confusing (why "took"?). It looks like took_push is true if
the server's latest response to request_url pushed a non-zero number of
resources...in which case the session gets ignored, basically? Does
this mean that if start pushing one resource for a URL, we can't later
learn to push a second resource as well? Why create a session object at
all if took_push is true? I'm probably just misunderstanding the larger
design at play here.

http://page-speed-codereview.appspot.com/970001/