-
Notifications
You must be signed in to change notification settings - Fork 523
Conversation
Hi @DamianEdwards, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
private readonly ISystemClock _systemClock; | ||
private readonly TimeSpan _timeWithoutRequestsUntilIdle; | ||
private readonly TimeSpan _timerInterval; | ||
private readonly uint _timerTicksWithoutRequestsUntilIdle; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double
? and _timerTicksSinceLastRequest can be double
as well to avoid casting issues & equality check between int
and uint
. And equality check can become comparison (greater than) to avoid double precision issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TimeSpan
? And increment by _timerInterval
in the timer. (plus >=
instead of ==
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even better :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, but I did it a different way to what you suggested 😄
Updated |
d4e2d5a
to
9b9d521
Compare
Rebased on #207 |
With this change we see throughput go from ~244,000 RPS (with the changes in #207) to ~273,000 RPS. That's a nearly 12% increase in throughput. |
9b9d521
to
93bf994
Compare
ping @halter73 @lodejard @davidfowl |
var now = _systemClock.UtcNow; | ||
|
||
// See http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.18 for required format of Date header | ||
_dateValue = now.ToString("r"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move "r" to Constants class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you really want me to do that? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're not going to do that, at least move http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.18
to a resource string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK doing that, it's actually kinda nice.
- Doing it on each request is expensive - The Timer is started when the first request comes in and fires every second - Every request flips a bool so the Timer knows requests are coming in - The Timer stops itself after a period of no requests coming in (10 seconds) - #163
53643b6
to
a7b65ef
Compare
Updated again |
|
#163
Uses a Timer to update a shared date string that is sent for the "Date" response header.
The Timer doesn't start until a request comes in, then shuts off after an idle period of no requests, and finally starts again if requests come in again.
NOTE: This is now rebased on #207 so until that gets merged, this PR contains all those changes too.