Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2171743002:
Do not persist HttpServerProperties to disk that often. (Closed)
|
Created:
4 years, 5 months ago by Bence Modified:
4 years, 4 months ago Reviewers:
Ryan Hamilton CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not persist HttpServerProperties to disk that often.
Add some heuristics to SetAlternativeServices that prevents a
ScheduleUpdatePrefsOnNetworkThread() call after each incoming Alt-Svc header,
based on the time of the last update, and the expiration times of the current
and new in-memory entries. (Note that the current in-memory entry might be
different from the entry last persisted to disk.)
Repeated requests to the same server typically result in identical Alt-Svc
announcements, which become different when the max-age field (in seconds from
now) gets converted into a base::Time expiration time. Alternative services do
not need to be persisted to disk with every incoming Alt-Svc header if other
fields (scheme, host, port) are identical.
BUG=554643
Committed: https://crrev.com/4b91d834629c5b01ae73b58e33b7b63cedde871d
Cr-Commit-Position: refs/heads/master@{#408273}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Re: #7. #
Total comments: 2
Patch Set 3 : Rebase. #Patch Set 4 : Re: #13. #
Messages
Total messages: 24 (15 generated)
The CQ bit was checked by bnc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bnc@chromium.org changed reviewers: + rch@chromium.org
Ryan: PTAL. Thank you.
https://codereview.chromium.org/2171743002/diff/1/net/http/http_server_proper... File net/http/http_server_properties_impl.cc (right): https://codereview.chromium.org/2171743002/diff/1/net/http/http_server_proper... net/http/http_server_properties_impl.cc:412: bool changed; I think you can simplify this slightly: bool changed = true; if (it != alternative_service_map_.end()) { DCHECK(!it->second.empty()); if (it->second.size() == alternative_service_info_vector.size()) { changed = false; const base::Time now = base::Time::Now(); .... } } https://codereview.chromium.org/2171743002/diff/1/net/http/http_server_proper... net/http/http_server_properties_impl.cc:430: new_time - now > 2 * (old_time - now)) { nit: Can you add a comment before the if(...) which says (in english) what these three conditions are: 1) expires sooner 2) expires less than 12 hours from now 3) new expiration is more than twice as long as old expiration That being said, I wonder a bout about #2. Imagine some site which is doing 30 minute expiration (who knows why). We'll persist every time. How about making #2 be something like: 2) new expiration is less than half as long as old expiration https://codereview.chromium.org/2171743002/diff/1/net/http/http_server_proper... File net/http/http_server_properties_manager.cc (right): https://codereview.chromium.org/2171743002/diff/1/net/http/http_server_proper... net/http/http_server_properties_manager.cc:37: const int64_t kAlternativeServicesUpdatePrefsS = 3600; This is one hour, right? That seems like a LONG time. On mobile, that's forever. https://codereview.chromium.org/2171743002/diff/1/net/http/http_server_proper... net/http/http_server_properties_manager.cc:224: base::Time::Now()); Hm. If nothing has changed, why bother persisting again?
The CQ bit was checked by bnc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL. Thank you. https://codereview.chromium.org/2171743002/diff/1/net/http/http_server_proper... File net/http/http_server_properties_impl.cc (right): https://codereview.chromium.org/2171743002/diff/1/net/http/http_server_proper... net/http/http_server_properties_impl.cc:412: bool changed; On 2016/07/21 22:27:05, Ryan Hamilton wrote: > I think you can simplify this slightly: > > bool changed = true; > if (it != alternative_service_map_.end()) { > DCHECK(!it->second.empty()); > if (it->second.size() == alternative_service_info_vector.size()) { > changed = false; > const base::Time now = base::Time::Now(); > .... > } > } Done. https://codereview.chromium.org/2171743002/diff/1/net/http/http_server_proper... net/http/http_server_properties_impl.cc:430: new_time - now > 2 * (old_time - now)) { On 2016/07/21 22:27:04, Ryan Hamilton wrote: > nit: Can you add a comment before the if(...) which says (in english) what these > three conditions are: > 1) expires sooner > 2) expires less than 12 hours from now > 3) new expiration is more than twice as long as old expiration > > That being said, I wonder a bout about #2. Imagine some site which is doing 30 > minute expiration (who knows why). We'll persist every time. How about making #2 > be something like: > > 2) new expiration is less than half as long as old expiration Your proposed (2) is redundant with (1), so I'll just scratch it. Also, if we do persist every five minutes anyway, there's not much value in (3) either, so I'll scratch that too. https://codereview.chromium.org/2171743002/diff/1/net/http/http_server_proper... File net/http/http_server_properties_manager.cc (right): https://codereview.chromium.org/2171743002/diff/1/net/http/http_server_proper... net/http/http_server_properties_manager.cc:37: const int64_t kAlternativeServicesUpdatePrefsS = 3600; On 2016/07/21 22:27:05, Ryan Hamilton wrote: > This is one hour, right? That seems like a LONG time. On mobile, that's forever. Well, then how about five minutes? Then I can change it to milliseconds to match the other two. https://codereview.chromium.org/2171743002/diff/1/net/http/http_server_proper... net/http/http_server_properties_manager.cc:89: last_update_prefs_from_cache_(base::Time::Now()) { Oops. https://codereview.chromium.org/2171743002/diff/1/net/http/http_server_proper... net/http/http_server_properties_manager.cc:224: base::Time::Now()); On 2016/07/21 22:27:05, Ryan Hamilton wrote: > Hm. If nothing has changed, why bother persisting again? Because old_time in HttpServerPropertiesImpl is the expiration time of the entry in memory, not of the one on disk. Imagine a server advertising max-age of one week. If there is a request every three days, old_time is four days in the future, new_time is seven, so |changed| is false, and we will never ever ever persist to disk. On desktop, Chrome could be running for months. Then there is a crash or someone trips in the power chord, and we do not have a valid Alt-Svc entry on disk for the next startup. https://codereview.chromium.org/2171743002/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_manager_unittest.cc (left): https://codereview.chromium.org/2171743002/diff/20001/net/http/http_server_pr... net/http/http_server_properties_manager_unittest.cc:769: TEST_P(HttpServerPropertiesManagerTest, SetAlternativeServicesEmpty) { I'm removing this test because now even empty => empty updates are persisted if more than five minutes have passed since the last update or if it's the first one. I could change SetAlternativeServices return type to an enum to address this, but this should be quite rare, I'm not sure it's worth the complexity.
https://codereview.chromium.org/2171743002/diff/1/net/http/http_server_proper... File net/http/http_server_properties_impl.cc (right): https://codereview.chromium.org/2171743002/diff/1/net/http/http_server_proper... net/http/http_server_properties_impl.cc:430: new_time - now > 2 * (old_time - now)) { On 2016/07/22 16:28:54, Bence wrote: > On 2016/07/21 22:27:04, Ryan Hamilton wrote: > > nit: Can you add a comment before the if(...) which says (in english) what > these > > three conditions are: > > 1) expires sooner > > 2) expires less than 12 hours from now > > 3) new expiration is more than twice as long as old expiration > > > > That being said, I wonder a bout about #2. Imagine some site which is doing 30 > > minute expiration (who knows why). We'll persist every time. How about making > #2 > > be something like: > > > > 2) new expiration is less than half as long as old expiration > > Your proposed (2) is redundant with (1), so I'll just scratch it. > > Also, if we do persist every five minutes anyway, there's not much value in (3) > either, so I'll scratch that too. That's true. :) It makes me wonder if we would be better to have: 1) sooner than old / 2 2) later than old * 2 The current state of the CL has some buffering/delay logic in two places... Every 5 min in the manager, and *now* here, but only if things changed enough. This feel odd. See my comment in the manager, perhaps we can just get rid of the 5 minute thing completely? https://codereview.chromium.org/2171743002/diff/1/net/http/http_server_proper... File net/http/http_server_properties_manager.cc (right): https://codereview.chromium.org/2171743002/diff/1/net/http/http_server_proper... net/http/http_server_properties_manager.cc:224: base::Time::Now()); On 2016/07/22 16:28:54, Bence wrote: > On 2016/07/21 22:27:05, Ryan Hamilton wrote: > > Hm. If nothing has changed, why bother persisting again? > > Because old_time in HttpServerPropertiesImpl is the expiration time of the entry > in memory, not of the one on disk. Imagine a server advertising max-age of one > week. If there is a request every three days, old_time is four days in the > future, new_time is seven, so |changed| is false, and we will never ever ever > persist to disk. On desktop, Chrome could be running for months. Then there is > a crash or someone trips in the power chord, and we do not have a valid Alt-Svc > entry on disk for the next startup. I'm not sure this bothers me. If the user only hits this site every 3 days, and hits *no other* sites which might cause HttpServerProperties to be persisted, then it's really no big deal if this happens. That being said, in this example, won't we persist on the first request (day 0), and then we crash on day 3, and then write again on day 6? https://codereview.chromium.org/2171743002/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_manager.cc (right): https://codereview.chromium.org/2171743002/diff/20001/net/http/http_server_pr... net/http/http_server_properties_manager.cc:91: base::TimeDelta::FromSeconds(kAlternativeServicesUpdatePrefsMs)) { Should this be "FromMilliseconds()"? (Here and elsewhere)
The CQ bit was checked by bnc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL. Thank you. https://codereview.chromium.org/2171743002/diff/1/net/http/http_server_proper... File net/http/http_server_properties_impl.cc (right): https://codereview.chromium.org/2171743002/diff/1/net/http/http_server_proper... net/http/http_server_properties_impl.cc:430: new_time - now > 2 * (old_time - now)) { On 2016/07/22 23:31:35, Ryan Hamilton wrote: > On 2016/07/22 16:28:54, Bence wrote: > > On 2016/07/21 22:27:04, Ryan Hamilton wrote: > > > nit: Can you add a comment before the if(...) which says (in english) what > > these > > > three conditions are: > > > 1) expires sooner > > > 2) expires less than 12 hours from now > > > 3) new expiration is more than twice as long as old expiration > > > > > > That being said, I wonder a bout about #2. Imagine some site which is doing > 30 > > > minute expiration (who knows why). We'll persist every time. How about > making > > #2 > > > be something like: > > > > > > 2) new expiration is less than half as long as old expiration > > > > Your proposed (2) is redundant with (1), so I'll just scratch it. > > > > Also, if we do persist every five minutes anyway, there's not much value in > (3) > > either, so I'll scratch that too. > > That's true. :) It makes me wonder if we would be better to have: > > 1) sooner than old / 2 > 2) later than old * 2 > > The current state of the CL has some buffering/delay logic in two places... > Every 5 min in the manager, and *now* here, but only if things changed enough. > This feel odd. See my comment in the manager, perhaps we can just get rid of the > 5 minute thing completely? Done. https://codereview.chromium.org/2171743002/diff/1/net/http/http_server_proper... File net/http/http_server_properties_manager.cc (right): https://codereview.chromium.org/2171743002/diff/1/net/http/http_server_proper... net/http/http_server_properties_manager.cc:224: base::Time::Now()); On 2016/07/22 23:31:35, Ryan Hamilton wrote: > On 2016/07/22 16:28:54, Bence wrote: > > On 2016/07/21 22:27:05, Ryan Hamilton wrote: > > > Hm. If nothing has changed, why bother persisting again? > > > > Because old_time in HttpServerPropertiesImpl is the expiration time of the > entry > > in memory, not of the one on disk. Imagine a server advertising max-age of > one > > week. If there is a request every three days, old_time is four days in the > > future, new_time is seven, so |changed| is false, and we will never ever ever > > persist to disk. On desktop, Chrome could be running for months. Then there > is > > a crash or someone trips in the power chord, and we do not have a valid > Alt-Svc > > entry on disk for the next startup. > > I'm not sure this bothers me. If the user only hits this site every 3 days, and > hits *no other* sites which might cause HttpServerProperties to be persisted, > then it's really no big deal if this happens. That being said, in this example, > won't we persist on the first request (day 0), and then we crash on day 3, and > then write again on day 6? All right then. I think you convinced me that the added complexity is not worth it. I'm reverting this timing logic.
The CQ bit was checked by rch@chromium.org
lgtm Looks great!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Do not persist HttpServerProperties to disk that often. Add some heuristics to SetAlternativeServices that prevents a ScheduleUpdatePrefsOnNetworkThread() call after each incoming Alt-Svc header, based on the time of the last update, and the expiration times of the current and new in-memory entries. (Note that the current in-memory entry might be different from the entry last persisted to disk.) Repeated requests to the same server typically result in identical Alt-Svc announcements, which become different when the max-age field (in seconds from now) gets converted into a base::Time expiration time. Alternative services do not need to be persisted to disk with every incoming Alt-Svc header if other fields (scheme, host, port) are identical. BUG=554643 ========== to ========== Do not persist HttpServerProperties to disk that often. Add some heuristics to SetAlternativeServices that prevents a ScheduleUpdatePrefsOnNetworkThread() call after each incoming Alt-Svc header, based on the time of the last update, and the expiration times of the current and new in-memory entries. (Note that the current in-memory entry might be different from the entry last persisted to disk.) Repeated requests to the same server typically result in identical Alt-Svc announcements, which become different when the max-age field (in seconds from now) gets converted into a base::Time expiration time. Alternative services do not need to be persisted to disk with every incoming Alt-Svc header if other fields (scheme, host, port) are identical. BUG=554643 Committed: https://crrev.com/4b91d834629c5b01ae73b58e33b7b63cedde871d Cr-Commit-Position: refs/heads/master@{#408273} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4b91d834629c5b01ae73b58e33b7b63cedde871d Cr-Commit-Position: refs/heads/master@{#408273} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
