Issue 471313002: Add an Update method on QuicAlarm which contains a granularity. Also

Issue 471313002: Add an Update method on QuicAlarm which contains a granularity. Also (Closed)

Created:
6 years, 4 months ago by ramant (doing other things)

Modified:
6 years, 4 months ago

Reviewers:
Ian Swett, wtc, Ryan Hamilton

CC:
chromium-reviews, cbentzel+watch_chromium.org, Ian Swett, wtc, dmz

Base URL:
https://chromium.googlesource.com/chromium/src.git@Final_0814_2

Project:
chromium

Visibility:
Public.

More Reviews

Description

Add an Update method on QuicAlarm which contains a granularity. Also change the idle network timeout to be 1 second shorter on the client and 1 second longer on the server. Change saves ~2% of CPU according to an EndToEndTest profile. Merge internal change: 73125905 R=rch@chromium.org

Patch Set 1 #

Total comments: 3

Created: 6 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+147 lines, -21 lines)			Patch
M	net/quic/quic_alarm.h	View	1 chunk	+5 lines, -0 lines	0 comments	Download
M	net/quic/quic_alarm.cc	View	1 chunk	+13 lines, -0 lines	3 comments	Download
M	net/quic/quic_alarm_test.cc	View	1 chunk	+19 lines, -0 lines	0 comments	Download
M	net/quic/quic_connection.cc	View	8 chunks	+22 lines, -18 lines	0 comments	Download
M	net/quic/quic_connection_helper_test.cc	View	1 chunk	+45 lines, -0 lines	0 comments	Download
M	net/quic/quic_connection_test.cc	View	1 chunk	+4 lines, -1 line	0 comments	Download
M	net/quic/quic_session_test.cc	View	1 chunk	+3 lines, -2 lines	0 comments	Download
M	net/tools/quic/quic_epoll_connection_helper_test.cc	View	1 chunk	+36 lines, -0 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

wtc

Patch set 1 LGTM. https://codereview.chromium.org/471313002/diff/1/net/quic/quic_alarm.cc File net/quic/quic_alarm.cc (right): https://codereview.chromium.org/471313002/diff/1/net/quic/quic_alarm.cc#newcode31 net/quic/quic_alarm.cc:31: if (!deadline.IsInitialized()) { Just curious: ...

6 years, 4 months ago (2014-08-15 19:03:54 UTC) #3

ramant (doing other things)

https://codereview.chromium.org/471313002/diff/1/net/quic/quic_alarm.cc File net/quic/quic_alarm.cc (right): https://codereview.chromium.org/471313002/diff/1/net/quic/quic_alarm.cc#newcode31 net/quic/quic_alarm.cc:31: if (!deadline.IsInitialized()) { On 2014/08/15 19:03:54, wtc wrote: > ...

6 years, 4 months ago (2014-08-15 19:18:33 UTC) #4

Ian Swett

6 years, 4 months ago (2014-08-15 20:11:54 UTC) #5

Message was sent while issue was closed.

lgtm

It is intended as an optimization, but I Update sometimes more accurately
reflects what the code is doing, which is somewhat nice.

https://codereview.chromium.org/471313002/diff/1/net/quic/quic_alarm.cc
File net/quic/quic_alarm.cc (right):

https://codereview.chromium.org/471313002/diff/1/net/quic/quic_alarm.cc#newco...
net/quic/quic_alarm.cc:31: if (!deadline.IsInitialized()) {
On 2014/08/15 19:18:33, ramant wrote:
> On 2014/08/15 19:03:54, wtc wrote:
> > 
> > Just curious: why do we need this behavior when we already provide a public
> > Cancel() method?
> 
> Ian: would appreciate your reasoning (is it a convenient method?)

It is convenient, and semantically more accurate, but most importantly it allows
the granularity optimization of not canceling and setting an alarm when the time
changes a small amount.  This can save a lot of CPU, as mentioned.

Expand Messages | Collapse Messages