[DESIGN] Damianedwards/date header perf #167

DamianEdwards · 2015-08-19T06:06:12Z

Design review please.

@halter73 @lodejard @davidfowl @Tratcher

dnfclas · 2015-08-19T06:06:16Z

Hi @DamianEdwards, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

Tragetaschen · 2015-08-19T08:24:58Z

src/Microsoft.AspNet.Server.Kestrel/Http/DateHeaderValueManager.cs

+                    {
+                        // Immediately assign the date value and start the timer again
+                        _dateValue = DateTime.UtcNow.ToString("r");
+                        _dateValueTimer = new Timer(UpdateDateValue, state: null, dueTime: _timerInterval, period: _timerInterval);


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.

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.

_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?

Why not use dueTime: TimeSpan.Zero to run the handler immediately

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 in GetDateHeaderValue 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

We use a single native timer, supplied by the VM, to schedule all managed timers in the AppDomain.

So in the spirit of

Why not create the timer in the constructor

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 drop PumpTimer and the locks altogether?

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.

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 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.

Process suspension type things? If is a requirement LGTM

Yep, that was one of the scenarios.

That requirement makes this solution rather elegant 👍

benaadams · 2015-08-19T19:29:27Z

Added extra bit to: #169

halter73 · 2015-08-20T18:23:32Z

I like the design 👍

muratg · 2015-09-09T15:17:37Z

@DamianEdwards Merge or close?

Tratcher · 2015-09-09T15:19:27Z

src/Microsoft.AspNet.Server.Kestrel/Http/FrameHeaders.Generated.cs

        {
            _Server = new[] { "Kestrel" };
-            _Date = new[] { DateTime.UtcNow.ToString("r") };
+            _Date = new[] { dateHeaderValue };


This needs to be set at the end of the response rather than the beginning. Things like the error handling middleware will clear the response headers.

Yeah, we'll do that as a separate change.

DamianEdwards · 2015-09-09T15:22:28Z

@muratg I'll close it and resubmit after rebasing and adding tests

muratg · 2015-09-09T16:09:29Z

👍

muratg · 2015-09-18T15:35:55Z

⌚

halter73 · 2015-09-18T17:29:42Z

#207 will conflict with this.

lodejard · 2015-09-18T18:24:37Z

If they're set at the end the app can't see or change them... Should we change the middleware to preserve the server and re-apply the current time?

Tratcher · 2015-09-18T18:35:32Z

Date is required and you can't trust misc middleware to handle it correctly. You can honor their value if they set it, but the server needs to set it at the end if it's missing.

Server is optional, we don't really care if it gets cleared.

- 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 ticks) - #163

davidfowl · 2015-09-24T05:34:09Z

src/Microsoft.AspNet.Server.Kestrel/Infrastructure/ISystemClock.cs

+    /// <summary>
+    /// Abstracts the system clock to facilitate testing.
+    /// </summary>
+    public interface ISystemClock


Closed this one, look at #220 instead

dnfclas added the cla-already-signed label Aug 19, 2015

Tragetaschen reviewed Aug 19, 2015
View reviewed changes

Tratcher reviewed Sep 9, 2015
View reviewed changes

DamianEdwards force-pushed the damianedwards/date-header-perf branch from 757d158 to 098e04a Compare September 9, 2015 18:59

lodejard force-pushed the dev branch from 6f8c040 to c50aec1 Compare September 17, 2015 18:31

DamianEdwards force-pushed the damianedwards/date-header-perf branch from 098e04a to 4da048e Compare September 23, 2015 22:31

DamianEdwards added 2 commits September 23, 2015 17:42

Added a test

2e19444

Added more comments and tests for DateHeaderValueManager

e92ee0b

DamianEdwards closed this Sep 24, 2015

davidfowl reviewed Sep 24, 2015
View reviewed changes

[DESIGN] Damianedwards/date header perf #167

[DESIGN] Damianedwards/date header perf #167

Uh oh!

Conversation

DamianEdwards commented Aug 19, 2015

Uh oh!

dnfclas commented Aug 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benaadams commented Aug 19, 2015

Uh oh!

halter73 commented Aug 20, 2015

Uh oh!

muratg commented Sep 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DamianEdwards commented Sep 9, 2015

Uh oh!

muratg commented Sep 9, 2015

Uh oh!

muratg commented Sep 18, 2015

Uh oh!

halter73 commented Sep 18, 2015

Uh oh!

lodejard commented Sep 18, 2015

Uh oh!

Tratcher commented Sep 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!