This repository was archived by the owner on Dec 18, 2018. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 521
[DESIGN] Damianedwards/date header perf #167
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
149 changes: 149 additions & 0 deletions
149
src/Microsoft.AspNet.Server.Kestrel/Http/DateHeaderValueManager.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,149 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using System.Threading; | ||
using Microsoft.AspNet.Server.Kestrel.Infrastructure; | ||
|
||
namespace Microsoft.AspNet.Server.Kestrel.Http | ||
{ | ||
/// <summary> | ||
/// Manages the generation of the date header value. | ||
/// </summary> | ||
public class DateHeaderValueManager : IDisposable | ||
{ | ||
private readonly ISystemClock _systemClock; | ||
private readonly TimeSpan _timeWithoutRequestsUntilIdle; | ||
private readonly TimeSpan _timerInterval; | ||
private readonly uint _timerTicksWithoutRequestsUntilIdle; | ||
|
||
private volatile string _dateValue; | ||
private bool _isDisposed = false; | ||
private bool _hadRequestsSinceLastTimerTick = false; | ||
private Timer _dateValueTimer; | ||
private object _timerLocker = new object(); | ||
private int _timerTicksSinceLastRequest; | ||
|
||
/// <summary> | ||
/// Initializes a new instance of the <see cref="DateHeaderValueManager"/> class. | ||
/// </summary> | ||
public DateHeaderValueManager() | ||
: this( | ||
systemClock: new SystemClock(), | ||
timeWithoutRequestsUntilIdle: TimeSpan.FromSeconds(10), | ||
timerInterval: TimeSpan.FromSeconds(1)) | ||
{ | ||
|
||
} | ||
|
||
// Internal for testing | ||
internal DateHeaderValueManager( | ||
ISystemClock systemClock, | ||
TimeSpan timeWithoutRequestsUntilIdle, | ||
TimeSpan timerInterval) | ||
{ | ||
_systemClock = systemClock; | ||
_timeWithoutRequestsUntilIdle = timeWithoutRequestsUntilIdle; | ||
_timerInterval = timerInterval; | ||
|
||
// Calculate the number of timer ticks where no requests are seen before we're considered to be idle. | ||
// Once we're idle, the timer is shutdown to prevent code from running while there are no requests. | ||
// The timer is started again on the next request. | ||
_timerTicksWithoutRequestsUntilIdle = (uint)(_timeWithoutRequestsUntilIdle.TotalMilliseconds / _timerInterval.TotalMilliseconds); | ||
} | ||
|
||
/// <summary> | ||
/// Returns a value representing the current server date/time for use in the HTTP "Date" response header | ||
/// in accordance with http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.18 | ||
/// </summary> | ||
/// <returns>The value.</returns> | ||
public string GetDateHeaderValue() | ||
{ | ||
PumpTimer(); | ||
|
||
// See https://msdn.microsoft.com/en-us/library/az4se3k1(v=vs.110).aspx#RFC1123 for info on the format | ||
// string used here. | ||
// The null-coalesce here is to protect against returning null after Dispose() is called, at which | ||
// point _dateValue will be null forever after. | ||
return _dateValue ?? _systemClock.UtcNow.ToString("r"); | ||
} | ||
|
||
/// <summary> | ||
/// Releases all resources used by the current instance of <see cref="DateHeaderValueManager"/>. | ||
/// </summary> | ||
public void Dispose() | ||
{ | ||
lock (_timerLocker) | ||
{ | ||
if (_dateValueTimer != null) | ||
{ | ||
DisposeTimer(); | ||
} | ||
|
||
_isDisposed = true; | ||
} | ||
} | ||
|
||
private void PumpTimer() | ||
{ | ||
_hadRequestsSinceLastTimerTick = true; | ||
|
||
// If we're already disposed we don't care about starting the timer again. This avoids us having to worry | ||
// about requests in flight during dispose (not that that should actually happen) as those will just get | ||
// SystemClock.UtcNow (aka "the slow way"). | ||
if (!_isDisposed && _dateValueTimer == null) | ||
{ | ||
lock (_timerLocker) | ||
{ | ||
if (!_isDisposed && _dateValueTimer == null) | ||
{ | ||
// Immediately assign the date value and start the timer again. We assign the value immediately | ||
// here as the timer won't fire until the timer interval has passed and we want a value assigned | ||
// inline now to serve requests that occur in the meantime. | ||
_dateValue = _systemClock.UtcNow.ToString("r"); | ||
_dateValueTimer = new Timer(UpdateDateValue, state: null, dueTime: _timerInterval, period: _timerInterval); | ||
} | ||
} | ||
} | ||
} | ||
|
||
// Called by the Timer (background) thread | ||
private void UpdateDateValue(object state) | ||
{ | ||
// See http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.18 for required format of Date header | ||
_dateValue = _systemClock.UtcNow.ToString("r"); | ||
|
||
if (_hadRequestsSinceLastTimerTick) | ||
{ | ||
// We served requests since the last tick, reset the flag and return as we're still active | ||
_hadRequestsSinceLastTimerTick = false; | ||
_timerTicksSinceLastRequest = 0; | ||
return; | ||
} | ||
|
||
// No requests since the last timer tick, we need to check if we're beyond the idle threshold | ||
_timerTicksSinceLastRequest++; | ||
if (_timerTicksSinceLastRequest == _timerTicksWithoutRequestsUntilIdle) | ||
{ | ||
// No requests since idle threshold so stop the timer if it's still running | ||
if (_dateValueTimer != null) | ||
{ | ||
lock (_timerLocker) | ||
{ | ||
if (_dateValueTimer != null) | ||
{ | ||
DisposeTimer(); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
private void DisposeTimer() | ||
{ | ||
_dateValueTimer.Dispose(); | ||
_dateValueTimer = null; | ||
_dateValue = null; | ||
} | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
18 changes: 18 additions & 0 deletions
18
src/Microsoft.AspNet.Server.Kestrel/Infrastructure/ISystemClock.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
|
||
namespace Microsoft.AspNet.Server.Kestrel.Infrastructure | ||
{ | ||
/// <summary> | ||
/// Abstracts the system clock to facilitate testing. | ||
/// </summary> | ||
public interface ISystemClock | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. internal There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Closed this one, look at #220 instead |
||
{ | ||
/// <summary> | ||
/// Retrieves the current system time in UTC. | ||
/// </summary> | ||
DateTimeOffset UtcNow { get; } | ||
} | ||
} |
24 changes: 24 additions & 0 deletions
24
src/Microsoft.AspNet.Server.Kestrel/Infrastructure/SystemClock.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
|
||
namespace Microsoft.AspNet.Server.Kestrel.Infrastructure | ||
{ | ||
/// <summary> | ||
/// Provides access to the normal system clock. | ||
/// </summary> | ||
public class SystemClock : ISystemClock | ||
{ | ||
/// <summary> | ||
/// Retrieves the current system time in UTC. | ||
/// </summary> | ||
public DateTimeOffset UtcNow | ||
{ | ||
get | ||
{ | ||
return DateTimeOffset.UtcNow; | ||
} | ||
} | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why not create the timer in the constructor and just do
Change()
here? Then,_dateValueTimer != null
is an invariant and wouldn't need to be checked all the time.Also: Why not use
dueTime: TimeSpan.Zero
to run the handler immediately, which sets the_dateValue
immediately.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.
_dateValueTimer == null
is the thing determining whether it needs to be switched on again. Could use boolean with_dateValueTimer.Change()
but that may make the logic more complicated?That would queue the callback to run immediately; but doesn't call it as part of the
Timer
constructor so the initialisation of the value would be in a race with the function return; which it would probably always loose. Though that would be fixed by the null coalescing inGetDateHeaderValue
as it would create its own value.However, not sure switching it off during low request periods vs creating a string every second with a single timer has much benefit as the timer isn't necessarily a real "individual" timer? http://referencesource.microsoft.com/#mscorlib/system/threading/timer.cs,29
So in the spirit of
Just switch the timer on in constructor and leave it ticking away until shut down? The side effect would be the GC has to clean up a string every second when the server isn't doing anything; but its happy doing that when the server is fully loaded; and the logic becomes much simpler in
UpdateDateValue
and you can dropPumpTimer
and the locks altogether?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.
Here is a different thought for the entire implementation that completely removes any need for synchronization:
http://docs.libuv.org/en/latest/timer.html
There would be one timer per Kestrel thread, but on the other hand the DateTime string wouldn't be shared between processors and every update is already in the "correct" processor's cache.
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.
The libuv timers are implemented in more or less the same way on linux and win as the .net timer; but you'd pay the added overhead of interop'ing the callback and the code for it would be less clear.
As Kestrel uses multiple (maybe?) libuv threads for socket IO then as you say, this would update in different cpu caches. However, as its a write once per second, read many times the processor cache would only have to refresh once per second (or once every 100k? executions) so it wouldn't be that significant; and it would put extra book keeping on the socket IO threads.
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.
There was concern over having any code running when the process is idle as it may interfere with mechanisms that perform operations on or in relation to idle processes. This design allows us to run the timer only when there are requests.
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.
Process suspension type things? If is a requirement LGTM
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.
Yep, that was one of the scenarios.
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.
That requirement makes this solution rather elegant 👍