first stage of async support - async by alexsku · Pull Request #1 · alexsku/dotnet

alexsku · 2016-12-19T18:18:30Z

multithreaded support for timings

alexsku · 2016-12-19T18:20:13Z

Sample.Mvc/Web.config

  -->
  <system.web>
-    <compilation debug="true" targetFramework="4.5">
+    <compilation debug="true" targetFramework="4.6.1">


i needed this to have AsyncLocal

alexsku · 2016-12-19T18:20:59Z

StackExchange.Profiling.Tests/MiniProfilerTest.cs

+        }
+
+        [Test]
+        public async void Current_WhenAsyncMethodReturns_IsCarried(


typo - configureAwait

you can use [Theory] instead of [Test] and [Values]

alexsku · 2016-12-19T18:22:02Z

StackExchange.Profiling.Wcf/ProfilerExtensions.cs

            {
                // This assumes that trivial items do not have any non-trivial children
-                timing.Children.RemoveAll(child => child.IsTrivial);
+                timing.AccessChildren()?.RemoveAll(child => child.IsTrivial);


there are a couple of places where they modify the children timing directly.

alexsku · 2016-12-19T18:22:46Z

StackExchange.Profiling.Wcf/WcfRequestProfilerProvider.cs

            context.Items[WcfCacheKey] = profiler;
        }
+
+        public override Timing GetHead()


this is more to support the new methods, the implementation is not very advanced here since i don't really care about wcf

alexsku · 2016-12-19T18:23:29Z

StackExchange.Profiling/Helpers/ExtensionMethods.cs

+        /// <param name="action">The <see cref="T:System.Action`1"/> delegate to perform on each element of the
+        ///  <see cref="T:System.Collections.Generic.List`1"/>.</param><exception cref="T:System.ArgumentNullException">
+        /// <paramref name="action"/> is null.</exception><exception cref="T:System.InvalidOperationException">An element in the collection has been modified. CautionThis exception is thrown starting with the .NET Framework 4.5. </exception>
+        public static void ForEach<T>(this IList<T> list, Action<T> action)


here i could lock on LockObj as well i guess

alexsku · 2016-12-19T18:24:50Z

StackExchange.Profiling/Helpers/ExtensionMethods.cs

+        /// <param name="list">the list to perform the operations for</param>
+        /// <param name="action">The <see cref="T:System.Action`1"/> delegate to perform on each element of the
+        ///  <see cref="T:System.Collections.Generic.List`1"/>.</param><exception cref="T:System.ArgumentNullException">
+        /// <paramref name="action"/> is null.</exception><exception cref="T:System.InvalidOperationException">An element in the collection has been modified. CautionThis exception is thrown starting with the .NET Framework 4.5. </exception>


i'm not very happy that this method is public, i would rather prefer if it was internal. and an alternative to this method is to update the couple of places which try to modify the list so they don't need ForEach and RemoveAll at all

are these only here because you changed an List to an IList?
If so, then I just think we replace them with traditional for loops

alexsku · 2016-12-19T18:26:24Z

StackExchange.Profiling/Timing.cs

        /// </summary>
        [DataMember(Order = 5)]
-        public List<Timing> Children { get; set; }
+        public IList<Timing> Children


Children now returns a copy of children, for 99% of the cases i think this is the best option

alexsku · 2016-12-19T18:27:34Z

StackExchange.Profiling/Timing.cs

+        /// <summary>
+        /// Gets or sets All sub-steps that occur within this Timing step. Add new children through <see cref="AddChild"/>
+        /// </summary>
+        public IList<Timing> AccessChildren()


this method is for the remaining 1% of the cases where the caller wants to modify Timings list. I'm thinking to replace it with explicit methods RemoveTimings or something like this.

one option is to replace Children with an IImmutableList<T>.
This method only appears to be used to remove all. Perhaps we can just rename this method RemoveChildren()?

I will need to add dependency on collections in order to start using iimmutablelist. I guess using that interface would make sense to emphasize that the list is not modifiable, not modifying those lists is rather pointless since i return copies.

alexsku · 2016-12-19T18:30:17Z

Ideally i think miniprofiler should be split into 2 classes: one class will be responsible for capturing timings. It would provide very limited ways for manipulating captured timings. Then, there would be a way to "complete" the capturing which would return a miniprofiler snapshot that can be modified easily but there would be no multithreaded access to it

pjhuck · 2016-12-19T22:05:41Z

StackExchange.Profiling.Tests/MiniProfilerTest.cs

+        }
+
+        [Test]
+        public async void Current_WhenAsyncMethodReturns_IsCarried(


you can use [Theory] instead of [Test] and [Values]

pjhuck · 2016-12-19T22:07:19Z

StackExchange.Profiling/AsyncWebRequestProfilerProvider.cs

+        }
+
+        private AsyncLocal<MiniProfiler> _currentProfiler = new AsyncLocal<MiniProfiler>();
+        private AsyncLocal<Timing> _currentTiming = new AsyncLocal<Timing>();


pjhuck · 2016-12-19T22:07:32Z

StackExchange.Profiling/AsyncWebRequestProfilerProvider.cs

+            }
+        }
+
+        protected override Timing CurrentHead {


pjhuck · 2016-12-19T22:07:41Z

StackExchange.Profiling/BaseProfilerProvider.cs

        /// </summary>
        public abstract MiniProfiler GetCurrentProfiler();

+        public abstract Timing GetHead();


documentation

I'm not sure I understand why these methods are necessary.

Since the MiniProfiler.Current is stored in the exact same manner as Head. Seems like you could continue to use MiniProfiler.Current.Head?

the documentation is provided in the interface

pjhuck · 2016-12-19T22:54:53Z

StackExchange.Profiling/Helpers/ExtensionMethods.cs

+        /// <param name="list">the list to perform the operations for</param>
+        /// <param name="action">The <see cref="T:System.Action`1"/> delegate to perform on each element of the
+        ///  <see cref="T:System.Collections.Generic.List`1"/>.</param><exception cref="T:System.ArgumentNullException">
+        /// <paramref name="action"/> is null.</exception><exception cref="T:System.InvalidOperationException">An element in the collection has been modified. CautionThis exception is thrown starting with the .NET Framework 4.5. </exception>


are these only here because you changed an List to an IList?
If so, then I just think we replace them with traditional for loops

pjhuck · 2016-12-19T23:00:39Z

StackExchange.Profiling/Timing.cs

+        /// <summary>
+        /// Gets or sets All sub-steps that occur within this Timing step. Add new children through <see cref="AddChild"/>
+        /// </summary>
+        public IList<Timing> AccessChildren()


one option is to replace Children with an IImmutableList<T>.
This method only appears to be used to remove all. Perhaps we can just rename this method RemoveChildren()?

pjhuck · 2016-12-19T23:25:42Z

StackExchange.Profiling/Timing.cs

@@ -299,6 +325,7 @@ internal void RemoveCustomTiming(string category, CustomTiming customTiming)
        }

        private readonly object _lockObject = new object();


Not your code, but I don't understand the locking around CustomTimingList.

The existing GetCustomTimingList acquires the same lock twice, and protects access to the dictionary. But the underlying values in the dictionary (lists of custom timings) are not protected by the lock. I don't see why they would need to protect the dictionary but the lists.

I think the existing method acquires _lockObject first and then the specific list if it is not null, i dont think this makes sense, since there are many cases where racing might occur.

alexsku · 2016-12-20T01:48:26Z

StackExchange.Profiling/AsyncWebRequestProfilerProvider.cs

+            }
+        }
+
+        private readonly AsyncLocal<MiniProfiler> _currentProfiler = new AsyncLocal<MiniProfiler>();


btw, if we wanted to stay compatible with .net 4 we could use our RevAsyncLocal here instead. functionally it is pretty much the same and available as long as CallContext.SetLogicalData is available.

If we are to suggest a pr for miniprofiler v4 we definitely will need to think about a substitution for AsyncLocal since they try to be backward compatible there

alexsku · 2016-12-20T01:49:14Z

StackExchange.Profiling/ClientTimings.cs

+        /// </summary>  
        [DataMember(Order = 2)]
-        public List<ClientTiming> Timings { get; set; }
+        public List<ClientTiming> Timings


i'm not sure mt access is possible for custom timings, but i guess it makes sense to do just in case

seems like we should the same here as what we do for Timings.Children - regarding IReadOnlyCollection or ImmutableArray

alexsku · 2016-12-20T01:57:25Z

StackExchange.Profiling/Timing.cs

        /// </summary>
        [DataMember(Order = 5)]
-        public List<Timing> Children { get; set; }
+        public List<Timing> Children


i think using IImmutableList for _children is an interesting idea. That would have indicated to the caller that the collection is immutable and make accessing _children naturally thread safe.

I think adding children might be a bit more expensive since the tree is going to be built underneath but perhaps the downside is not that high, do you think we should make the change? I guess we (nor miniprofiler v4) are not going to support .net below 4.5 (where Immutable would not be available)

It looks like in a single threaded scenario immutablelist is significantly underperforming comparing to list: https://ayende.com/blog/164739/immutable-collections-performance

hmm yeah, the ImmutableList perf isn't great because inside it's actually a tree.
But they have an ImmutableArray which is faster to read, but more expensive to write http://stackoverflow.com/a/28621409/830724

Anyway, I think perhaps the best thing to do is to have this return an IReadOnlyList or IReadOnlyCollection that way it is obvious that the caller shouldn't mutate it. If we really cared about performance we could return an ImmutableArray or IReadOnlyList here and cache the result, so we don't have to create the copy on every access.

alexsku · 2016-12-21T02:54:26Z

StackExchange.Profiling/ClientTimings.cs

+            get
+            {
+                lock(_lockObject)
+                    return new List<ClientTiming>(_timings);


a case when _timings is null

if we're going to go w/ ImmutableList for the Miniprofiler.Children, seems like we should do that here too.

alexsku · 2016-12-21T02:54:39Z

StackExchange.Profiling/ClientTimings.cs

+            set
+            {
+                lock(_lockObject)
+                    _timings = new List<ClientTiming>(value);


a case when value is null

what does this comment mean? you need to handle null?

alexsku · 2016-12-21T02:55:35Z

StackExchange.Profiling/Timing.cs

+            {
+                lock(_lockObject)
+                {
+                    return _customTimings.ToDictionary(p => p.Key, p => new List<CustomTiming>(p.Value));


a case when _customTimings is null

alexsku · 2016-12-21T02:55:43Z

StackExchange.Profiling/Timing.cs

+            {
+                lock(_lockObject)
+                {
+                    _customTimings = value.ToDictionary(p => p.Key, p => new List<CustomTiming>(p.Value));


a case when value is null

pjhuck · 2016-12-21T03:56:25Z

StackExchange.Profiling/ClientTimings.cs

+            get
+            {
+                lock(_lockObject)
+                    return new List<ClientTiming>(_timings);


if we're going to go w/ ImmutableList for the Miniprofiler.Children, seems like we should do that here too.

pjhuck · 2016-12-21T03:56:44Z

StackExchange.Profiling/ClientTimings.cs

+            set
+            {
+                lock(_lockObject)
+                    _timings = new List<ClientTiming>(value);


what does this comment mean? you need to handle null?

pjhuck · 2016-12-21T04:17:40Z

StackExchange.Profiling/Timing.cs

        /// </summary>
        [DataMember(Order = 5)]
-        public List<Timing> Children { get; set; }
+        public List<Timing> Children


hmm yeah, the ImmutableList perf isn't great because inside it's actually a tree.
But they have an ImmutableArray which is faster to read, but more expensive to write http://stackoverflow.com/a/28621409/830724

Anyway, I think perhaps the best thing to do is to have this return an IReadOnlyList or IReadOnlyCollection that way it is obvious that the caller shouldn't mutate it. If we really cared about performance we could return an ImmutableArray or IReadOnlyList here and cache the result, so we don't have to create the copy on every access.

pjhuck · 2016-12-21T04:21:16Z

StackExchange.Profiling/Timing.cs

+        {
+            lock (_lockObject)
+            {
+


pjhuck · 2016-12-21T04:22:00Z

StackExchange.Profiling/ClientTimings.cs

+        /// </summary>  
        [DataMember(Order = 2)]
-        public List<ClientTiming> Timings { get; set; }
+        public List<ClientTiming> Timings


seems like we should the same here as what we do for Timings.Children - regarding IReadOnlyCollection or ImmutableArray

alexsku · 2016-12-22T02:05:10Z

StackExchange.Profiling/Timing.cs

        /// </summary>
        [DataMember(Order = 5)]
-        public List<Timing> Children { get; set; }
+        public IReadOnlyList<Timing> Children


so v4 breaks if I use IReadOnlyList there. It breaks because protobuf that is used there doesn't support read only collections. There is an option to use IImmutableList instead (which is supported by protobuf) but then there would be a problem with Dictionary, there is no lightweight implementation of IImmutableDictionary available.

I think the best way is to revert to using List and Dictionary, what do you think?

alexsku · 2016-12-23T06:46:23Z

MiniProfiler.EF6.nuspec

+    <id>MiniProfilerAsync.EF6</id>
+    <version>3.0.12</version>
+    <authors>rev.com</authors>
+    <owners>rev.com</owners>


not sure what else should go here

alexsku · 2016-12-23T06:46:36Z

MiniProfiler.EF6.nuspec

-    <authors>Jarrod Dixon, Yaakov Ellis</authors>
-    <owners>Jarrod Dixon, Yaakov Ellis</owners>
+    <id>MiniProfilerAsync.EF6</id>
+    <version>3.0.12</version>


the version needs to be bumped up i believe

alexsku · 2016-12-23T06:46:50Z

MiniProfiler.EF6.nuspec

-    <version>3.0.11</version>
-    <authors>Jarrod Dixon, Yaakov Ellis</authors>
-    <owners>Jarrod Dixon, Yaakov Ellis</owners>
+    <id>MiniProfilerAsync.EF6</id>


i think it's better if id is different

alexsku · 2016-12-23T06:47:59Z

StackExchange.Profiling.EntityFramework6/Properties/AssemblyInfo.cs

-[assembly: AssemblyVersion("3.0.11")]
-[assembly: AssemblyInformationalVersion("3.0.11")]
+[assembly: AssemblyVersion("3.0.12")]
+[assembly: AssemblyInformationalVersion("3.0.12")]


just in case i bumped up versions of dlls

alexsku · 2017-01-05T08:39:09Z

StackExchange.Profiling/MiniProfiler.cs

-            return new JavaScriptSerializer { MaxJsonLength = Settings.MaxJsonResponseSize };   
+            var res = new JavaScriptSerializer { MaxJsonLength = Settings.MaxJsonResponseSize };
+
+            res.RegisterConverters(new[] { new ReadOnlyDictionaryConverter<IReadOnlyList<CustomTiming>>() });


So this is a total disaster, it looks like JavaScriptSerializer doesn't support natively IReadOnlyDictionary, so i had to implement and use a converter for it, luckily i don't think we much depend on JavaScriptSerializer except for unit tests. @eugenep-rev, am I correct that we don't rely on JavaScriptSerializer?

pjhuck · 2017-01-05T16:26:34Z

StackExchange.Profiling/Timing.cs

                lock(_lockObject)
                {
-                    return _customTimings?.ToDictionary(p => p.Key, p => new List<CustomTiming>(p.Value));
+                    return _customTimings?.ToDictionary(p => p.Key, p => (IReadOnlyList<CustomTiming>)new List<CustomTiming>(p.Value));


I think it would be better to get rid of all these copies on the get (here and Children, and ClientTimings)
I'd rather have explicity Set() methods and have the properties with no setters.

pjhuck

only questions are about assembly and nuget versions.

pjhuck · 2017-01-06T04:43:09Z

MiniProfiler.EF6.nuspec

-    <owners>Jarrod Dixon, Yaakov Ellis</owners>
+    <id>RevMiniProfiler.EF6</id>
+    <version>3.1.0.2</version>
+    <authors>rev.com</authors>


i would keep the original authors and just append rev

pjhuck · 2017-01-06T04:43:49Z

MiniProfiler.EF6.nuspec

-    <authors>Jarrod Dixon, Yaakov Ellis</authors>
-    <owners>Jarrod Dixon, Yaakov Ellis</owners>
+    <id>RevMiniProfiler.EF6</id>
+    <version>3.1.0.2</version>


how did you come up with this version number? why not 3.1.0?

it was 3.1.0 but then i bumped it up while making changes.

pjhuck · 2017-01-06T04:44:53Z

StackExchange.Profiling.EntityFramework6/Properties/AssemblyInfo.cs

-[assembly: AssemblyVersion("3.0.11")]
-[assembly: AssemblyInformationalVersion("3.0.11")]
+[assembly: AssemblyVersion("3.1.0.0")]
+[assembly: AssemblyInformationalVersion("3.1.0.0")]


seems like this should match the version in the nuspec

I don't think this is necessary nor it is common practice as it seems (e.g. MiniProfiler's nuspec doesn't match with the assembly version). In general i found bumping up assembly version too cumbersome since i needed to go and update the versions in the config files

pjhuck · 2017-01-06T04:45:28Z

StackExchange.Profiling.Mvc/Properties/AssemblyInfo.cs

 [assembly: Guid("44f8ac97-8e2c-4dba-adae-d971fb086a19")]
-[assembly: AssemblyVersion("3.0.11")]
-[assembly: AssemblyInformationalVersion("3.0.11")]
+[assembly: AssemblyVersion("3.1.0.0")]


same comment re version matching

pjhuck · 2017-01-06T04:48:05Z

StackExchange.Profiling/Properties/AssemblyInfo.cs

-[assembly: AssemblyVersion("3.1")]
-[assembly: AssemblyInformationalVersion("3.1")]
+[assembly: AssemblyVersion("3.3.0.2")]
+[assembly: AssemblyInformationalVersion("3.3.0.2")]


version should match nuspec

first stage of async support - async

46620d1

multithreaded support for timings

alexsku commented Dec 19, 2016

View reviewed changes

pjhuck reviewed Dec 19, 2016

View reviewed changes

alexsku added 2 commits December 19, 2016 17:41

addressed feedback

c951749

copying custom timings

af8762f

alexsku commented Dec 20, 2016

View reviewed changes

alexsku commented Dec 21, 2016

View reviewed changes

pjhuck approved these changes Dec 21, 2016

View reviewed changes

feedback addressed

983bce4

alexsku commented Dec 22, 2016

View reviewed changes

alexsku added 2 commits December 22, 2016 14:33

reverted from IReadOnlyList to List to be similar to v4

8a6e08f

rev nuget packages for miniprofiler, mvc4, and ef6

cae0b9e

alexsku commented Dec 23, 2016

View reviewed changes

feedback addressed

a8c601a

added support for serializing IReadOnlyDictionary

46e1551

alexsku commented Jan 5, 2017

View reviewed changes

pjhuck reviewed Jan 5, 2017

View reviewed changes

alexsku added 3 commits January 5, 2017 13:06

Added caching for returned copies of collections

fc2b37d

version bumped up

30621b4

uses async provider by default

f936b01

pjhuck approved these changes Jan 6, 2017

View reviewed changes

updated the versions

8a07228

		@@ -299,6 +325,7 @@ internal void RemoveCustomTiming(string category, CustomTiming customTiming)
		}

		private readonly object _lockObject = new object();

Conversation

alexsku commented Dec 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexsku Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

alexsku commented Dec 19, 2016

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!

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!

alexsku Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

alexsku Dec 19, 2016 •

edited

Loading

alexsku Dec 20, 2016 •

edited

Loading