-
Notifications
You must be signed in to change notification settings - Fork 611
async multithreading support #147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,15 +85,17 @@ public override void Save(MiniProfiler profiler) | |
| } | ||
|
|
||
| SaveTimings(timings, conn); | ||
| if (profiler.ClientTimings != null && profiler.ClientTimings.Timings != null && profiler.ClientTimings.Timings.Any()) | ||
|
|
||
| var clientTimings = profiler.ClientTimings?.Timings; | ||
| if (clientTimings != null && clientTimings.Any()) | ||
| { | ||
| // set the profilerId (isn't needed unless we are storing it) | ||
| profiler.ClientTimings.Timings.ForEach(x => | ||
| foreach(var x in clientTimings) | ||
| { | ||
| x.MiniProfilerId = profiler.Id; | ||
| x.Id = Guid.NewGuid(); | ||
| }); | ||
| SaveClientTimings(profiler.ClientTimings.Timings, conn); | ||
| } | ||
| SaveClientTimings(clientTimings, conn); | ||
|
Member
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. +1 - good cleanup cleanup on these. |
||
| } | ||
| } | ||
| } | ||
|
|
@@ -139,7 +141,7 @@ INSERT INTO MiniProfilerTimings | |
| } | ||
| } | ||
|
|
||
| private void SaveClientTimings(List<ClientTiming> timings, DbConnection conn) | ||
| private void SaveClientTimings(IReadOnlyList<ClientTiming> timings, DbConnection conn) | ||
| { | ||
| const string sql = @" | ||
| INSERT INTO MiniProfilerClientTimings | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,12 +14,26 @@ public class ClientTimings | |
| { | ||
| private const string TimingPrefix = "clientPerformance[timing]["; | ||
| private const string ProbesPrefix = "clientProbes["; | ||
| private readonly object _lockObject = new object(); | ||
| private List<ClientTiming> _timings; | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the list of client side timings | ||
| /// </summary> | ||
| [DataMember(Order = 2)] | ||
| public List<ClientTiming> Timings { get; set; } | ||
| public List<ClientTiming> Timings | ||
| { | ||
| get | ||
| { | ||
| lock(_lockObject) | ||
| return _timings == null ? null : new List<ClientTiming>(_timings); | ||
|
Member
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. This creates a duplicate list on every access, that's a lot of allocations for the safety. |
||
| } | ||
| set | ||
| { | ||
| lock (_lockObject) | ||
| _timings = value == null ? null : new List<ClientTiming>(value); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the redirect count. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| #if NET45 | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Runtime.CompilerServices; | ||
| using System.Runtime.Remoting.Messaging; | ||
| using System.Runtime.Serialization; | ||
|
|
||
| namespace StackExchange.Profiling.Helpers.Net45 | ||
| { | ||
| /// <summary> | ||
| /// Implements interface similar to AsyncLocal which is not available until .Net 4.6. | ||
| /// | ||
| /// The implementation is inspired by Ambient Context Logic from here: | ||
| /// https://github.com/mehdime/DbContextScope/blob/master/Mehdime.Entity/Implementations/DbContextScope.cs | ||
| /// Unlike DbContextScope's implementation this implementation doesn't support serialization/deserialization | ||
| /// ths not allowing to cross app domain barrier. | ||
| /// </summary> | ||
| internal class AsyncLocal<T> | ||
|
Author
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. a polyfill for AsyncLocal which is not available before .Net 4.6 |
||
| where T : class | ||
| { | ||
| public AsyncLocal() | ||
| { | ||
| } | ||
|
|
||
| public AsyncLocal( | ||
| T obj | ||
| ) | ||
| { | ||
| Value = obj; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets or Sets the value. | ||
| /// </summary> | ||
| public T Value | ||
| { | ||
| get | ||
| { | ||
| return (CallContext.LogicalGetData(_id) as ObjectRef)?.Ref; | ||
| } | ||
|
|
||
| set | ||
| { | ||
| CallContext.LogicalSetData(_id, new ObjectRef { Ref = value }); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Identifies this instance of <see cref="AsyncLocal"/> in CallContext. | ||
| /// </summary> | ||
| private readonly string _id = Guid.NewGuid().ToString(); | ||
|
|
||
| [Serializable] | ||
| private class ObjectRef : MarshalByRefObject, ISerializable | ||
| { | ||
| // The special constructor is used to deserialize values. | ||
| public ObjectRef() | ||
| { | ||
| } | ||
|
|
||
| // The special constructor is used to deserialize values. | ||
| public ObjectRef( | ||
| SerializationInfo info, | ||
| StreamingContext context | ||
| ) | ||
| { | ||
| } | ||
|
|
||
| public void GetObjectData(SerializationInfo info, StreamingContext context) | ||
| { | ||
| } | ||
|
|
||
| public T Ref { get; set; } | ||
| } | ||
| } | ||
| } | ||
| #endif | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| <Project Sdk="Microsoft.NET.Sdk" ToolsVersion="15.0"> | ||
| <Project Sdk="Microsoft.NET.Sdk" ToolsVersion="15.0"> | ||
| <PropertyGroup> | ||
| <TargetFrameworks>net45;netstandard1.5</TargetFrameworks> | ||
| <AssemblyName>MiniProfiler.Shared</AssemblyName> | ||
|
|
@@ -22,6 +22,7 @@ | |
| </ItemGroup> | ||
| <ItemGroup> | ||
| <PackageReference Include="NETStandard.Library" Version="1.6.1" /> | ||
| <PackageReference Include="System.Collections.Immutable" Version="1.3.0" /> | ||
|
Member
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. Compared to current, we're taking a package dependency where there is none today (the .Library should move under I'm not even seeing where this is actually used...am I missing a diff?
Author
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. Oops, i should remove it, it is a left over from my experiments with using immutable collections |
||
| </ItemGroup> | ||
| <ItemGroup Condition="'$(TargetFramework)' == 'netstandard1.5'"> | ||
| <PackageReference Include="System.ComponentModel.Primitives" Version="4.3.0" /> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -135,10 +135,9 @@ public Timing Root | |
| { | ||
| var timing = timings.Pop(); | ||
|
|
||
| if (timing.HasChildren) | ||
| var children = timing.Children; | ||
| if (children?.Count > 0) | ||
| { | ||
| var children = timing.Children; | ||
|
|
||
| for (int i = children.Count - 1; i >= 0; i--) | ||
| { | ||
| children[i].ParentTiming = timing; | ||
|
|
@@ -189,7 +188,19 @@ public Timing Root | |
| /// <summary> | ||
| /// Gets or sets points to the currently executing Timing. | ||
| /// </summary> | ||
| public Timing Head { get; set; } | ||
| public Timing Head | ||
| { | ||
| get | ||
| { | ||
| Settings.EnsureProfilerProvider(); | ||
| return Settings.ProfilerProvider.CurrentHead; | ||
|
Author
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. It is possible that the current thread spawns more than one task on separate threads and wait for them. As a result technically it is possible that there would be more than one separate head for a miniprofiler instance (one for each spawned thread). This code offloads managing heads to the profiler provider, so if the provider handles spawning threads (i.e. it relies on AsyncLocal or CallContext) then it should be able to manage head properly. |
||
| } | ||
| set | ||
| { | ||
| Settings.EnsureProfilerProvider(); | ||
| Settings.ProfilerProvider.CurrentHead = value; | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets the ticks since this MiniProfiler was started. | ||
|
|
@@ -329,9 +340,9 @@ public IEnumerable<Timing> GetTimingHierarchy() | |
|
|
||
| yield return timing; | ||
|
|
||
| if (timing.HasChildren) | ||
| var children = timing.Children; | ||
| if (children?.Count > 0) | ||
| { | ||
| var children = timing.Children; | ||
| for (int i = children.Count - 1; i >= 0; i--) timings.Push(children[i]); | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,11 @@ public abstract class BaseProfilerProvider : IProfilerProvider | |
| /// </summary> | ||
| public abstract MiniProfiler GetCurrentProfiler(); | ||
|
|
||
| /// <summary> | ||
| /// GEts or sets the current head timing. This is used by <see cref="MiniProfiler.Head"/>. | ||
| /// </summary> | ||
| public abstract Timing CurrentHead { get; set; } | ||
|
Member
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. Really not a fan of this - this is the Profiler provider, not "things about the profiler" provider. It shouldn't have knowledge or intertwined dependencies of the inner-workings, since implementations of this can and will live outside this library. Generally, this doesn't have any known internal state at all.
Author
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. I understand your concern, my thinking was that there could be more than one head timing in an async scenario. However in the current design whether async scenario is supported is left up to the profiler provider, so it looks like the only way to support is to store current head in the profiler provider. Do you think it would be better to move the AsyncLocal backing the head into mini profiler?
Member
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. The timing is specific to the operation, so yes, possibly making the head AsyncLocal makes the most sense. We'll have to think through if that actually breaks anything though. And the context fetch isn't free either for all requests, so it's not a clear win on the performance side. |
||
|
|
||
| /// <summary> | ||
| /// Sets <paramref name="profiler"/> to be active (read to start profiling) | ||
| /// This should be called once a new MiniProfiler has been created. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,8 @@ | ||
| #if NET45 | ||
| using System; | ||
| using System.Runtime.Remoting.Messaging; | ||
| using System.Web; | ||
| using System.Web; | ||
| using StackExchange.Profiling.Helpers.Net45; | ||
|
Author
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. Since there is AsyncLocal polyfill we can keep the implementation of the provider the same - it will be either using the polyfill or .net version of AsyncLocal
Member
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. This is a pretty drastic change in behavior, from defaulting to 1 per request, to 1 per request or background thread. This would break many expectations in Stack Overflow's code, as an example. |
||
| #else | ||
| using System.Threading; | ||
| #endif | ||
|
|
@@ -13,43 +14,23 @@ namespace StackExchange.Profiling | |
| /// </summary> | ||
| public class DefaultProfilerProvider : BaseProfilerProvider | ||
| { | ||
| #if NET45 | ||
| const string ContextKey = ":miniprofiler:"; | ||
| private readonly AsyncLocal<MiniProfiler> _profiler = new AsyncLocal<MiniProfiler>(); | ||
| private readonly AsyncLocal<Timing> _currentTiming = new AsyncLocal<Timing>(); | ||
|
|
||
| private MiniProfiler Profiler | ||
| { | ||
| get | ||
| { | ||
| if (HttpContext.Current != null) | ||
| { | ||
| return HttpContext.Current?.Items[ContextKey] as MiniProfiler; | ||
| } | ||
| else | ||
| { | ||
| return CallContext.LogicalGetData(ContextKey) as MiniProfiler; | ||
| } | ||
| } | ||
| set | ||
| { | ||
| if (HttpContext.Current != null) | ||
| { | ||
| HttpContext.Current.Items[ContextKey] = value; | ||
| } | ||
| else | ||
| { | ||
| CallContext.LogicalSetData(ContextKey, value); | ||
| } | ||
| } | ||
| get { return _profiler.Value; } | ||
| set { _profiler.Value = value; } | ||
| } | ||
| #else | ||
| private AsyncLocal<MiniProfiler> _profiler = new AsyncLocal<MiniProfiler>(); | ||
|
|
||
| private MiniProfiler Profiler | ||
| /// <summary> | ||
| /// Current head timing. | ||
| /// </summary> | ||
| public override Timing CurrentHead | ||
| { | ||
| get { return _profiler.Value; } | ||
| set { _profiler.Value = value; } | ||
| get { return _currentTiming.Value; } | ||
| set { _currentTiming.Value = value; } | ||
| } | ||
| #endif | ||
|
|
||
| /// <summary> | ||
| /// The name says it all. | ||
|
|
||
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.
clientTiming?.Any() ?? false