Use Lazy<T> for GeometryServiceProvider.Instance. #40
Conversation
On platforms that support it, anyway.
| private static IGeometryServices _instance; | ||
| #if HAS_SYSTEM_LAZY_T | ||
| private static readonly Lazy<IGeometryServices> ReflectedInstance = new Lazy<IGeometryServices>(ReflectInstance); | ||
| #else | ||
| private static readonly object LockObject = new object(); |
There was a problem hiding this comment.
A Lazy<T> already has a lock inside it when it's initialized using that constructor.
There was a problem hiding this comment.
With this change, LockObject is only used on runtimes TargetFrameworks that don't have access to Lazy<T> (net20, net35-client, net35-cf).
There was a problem hiding this comment.
Yeah, you're right. You use lock around ReflectInstance and not in it.
|
@FObermaier please take a look, this meets the goal you'd mentioned here:
Specifically |
|
@YohDeadfall, @roji, do you have any objections using this PR to address your problem? |
|
The test isn't representative because:
|
The actions taken in the benchmark that I'd run are intended to create a best-case situation to showcase the claimed benefits that removing Would you like a complete, buildable, runnable version of the sample that you could experiment with on your own to test a hypothesis that you have about something in here tainting the benchmark? If so, I'd be happy to oblige: GeoAPI.zip. Note that you have to build the GeoAPI project directly before IntelliSense will work in
Other than the couple of additional commits that were added after I'd gathered the above numbers, I made every effort for If you'd like, I can re-run it with those two additional commits added (or you can do so yourself). Otherwise, the above measurements suggest that any effect from the "other optimizations" contributes a benefit that's smaller than the measured standard deviation of these samples; at best, if the other changes do have an impact, I have no evidence that their combined impact is stronger than 2 milliseconds per application instance. |
|
I mostly have made benchmarks, but due to temporary unavailability of NuGet I can't restore packages and finish them. When it's done, I push them into a new branch in my form and post a link to it. |
|
If you want to run the benchmark on multiple cores, use xunit.perfromance with which you'll be able to construct threads, set affinity and start them before measurement. Each thread should wait for something ( As promised I have made a benchmark to test two scenarios:
public class AutoInitialized
{
[Benchmark]
public IGeometryServices Locks()
{
UseLocks.Reset();
return UseLocks.Instance;
}
[Benchmark]
public IGeometryServices PoolRequest39()
{
UsePoolRequest39.Reset();
return UsePoolRequest39.Instance;
}
[Benchmark]
public IGeometryServices Lazy()
{
UseLazy.Reset();
return UseLazy.Instance;
}
[Benchmark]
public IGeometryServices LazyInitializer()
{
UseLazyInitializer.Reset();
return UseLazyInitializer.Instance;
}
}
public class UserInitialized
{
private static readonly DummyGeometryServices s_dummy = new DummyGeometryServices();
[GlobalSetup]
public void Setup()
{
UseLocks.Instance = s_dummy;
UsePoolRequest39.Instance = s_dummy;
UseLazy.Instance = s_dummy;
UseLazyInitializer.Instance = s_dummy;
}
[Benchmark]
public IGeometryServices Locks()
=> UseLocks.Instance;
[Benchmark]
public IGeometryServices PoolRequest39()
=> UsePoolRequest39.Instance;
[Benchmark]
public IGeometryServices Lazy()
=> UseLazy.Instance;
[Benchmark]
public IGeometryServices LazyInitializer()
=> UseLazyInitializer.Instance;
}
public static class UseLocks
{
private static volatile IGeometryServices s_instance;
private static readonly object s_lock = new object();
public static IGeometryServices Instance
{
get
{
lock (s_lock)
{
return s_instance ?? (s_instance = ReflectInstance());
}
}
set
{
lock (s_lock)
{
s_instance = value;
}
}
}
public static void Reset()
=> s_instance = null;
private static IGeometryServices ReflectInstance()
}
// etc.The full code can be found in 921a147. EnvironmentAutoInitialized
UserInitialized
ConslusionAs you can see there is no big difference between pool request #39, |
|
I guess I'm missing the point of this benchmark that you've run. Unless I'm misunderstanding it, it seems to suggest that implementing either #39 or #40, compared to doing nothing, can save us about 16 nanoseconds per access of
The point of my initial benchmark was to prove (to myself, at least) that there may be a problem to solve. Personally, I find it very difficult to propose a solution to a problem that hasn't been proven to exist. This is especially the case when it comes to performance. Especially when there are alternative solutions being proposed. So before I could allow myself to submit a pull request, I needed to submit something that proves the existence of a measurable problem, and that this is a solution to that problem. I recognize that the approach I took to simulate contention for the lock is not as robust and precise as it would need to be for a different situation. However, I do not see any reason to believe that the results it generated are invalid, nor do I see any reason to continue throwing benchmark results around: @FObermaier specifically asked if you had any objections to using this PR to address your problem, so if you don't have objections on the grounds of speed, then it seems to me that there's no reason to bring up any benchmarks. However, this part of your response does seem to stand out as an objection on another ground, and I wish you'd spent more time elaborating on it:
It sounds like you're claiming that this one is incorrect in some way; can you please help me understand what your objection is? If possible, please try to meet the following criteria, to try to avoid holding up a solution for too much longer:
|
|
Looks like we didn't understand each other correctly. Okay, I'll try to explain myself better :)
The idea of my benchmark to know execution time of each scenario. With the numbers we have now it's possbile to know how long it would take when multiple threads are running. For example, for the
I'm pretty sure that you know that
Okay, you want to have a bootstrapper (see #41). It should work like the current initialization algorithm, but with an user provided factory. How can you do that using private static volatile IGeometryServices s_instance;
private static readonly object s_lock = new pbject();
public static bool Initialize(Func<IGeometryServices> factory)
{
if (s_instance != null)
return false;
lock (s_lock)
return Interlocked.CompareExchange(ref s_instance, factory(); null) == null;
}
private static IGeometryServices InitializeUsingReflection()
{
lock (s_lock)
{
var instance = s_instance;
if (instance != null) return instance;
// other stuff
}
}In the code above only one factory can be executed ( Have I answered all your questions? /cc @roji |
|
I haven't had time to follow this discussion in detail... To be honest, this seems like a huge over-complicated conversation about something that seems quite simple to me... I'll try to go back to the basics.
I think everyone agrees that locking on every read is not a good situation to be in (I may be wrong). If so, then are we just discussing Lazy vs. simple volatile? |
I've been figuring that the combination of #40 and #41 can be done like so: private static readonly Lazy<IGeometryServices> ReflectedInstance = new Lazy<IGeometryService>(ReflectInstance);
private static volatile IGeometryServices _instance;
public static IGeometryServices Instance
{
get => _instance ?? ReflectedInstance.Value;
set => _instance = value ?? throw new ArgumentNullException(nameof(value));
}
public static bool SetExplicitValueIfNotYetSetExplicitly(IGeometryServices newValue) =>
Interlocked.CompareExchange(ref _instance,
newValue ?? throw new ArgumentNullException(nameof(newValue)),
null) == null;When the instance value is When the instance value is non-
Reflection-based initializer should only run if someone accesses the getter before the value has been set explicitly.
I agree. I'm surprised it's gotten as deep as it has.
It's not a "singleton" in the traditional sense, because there can be any arbitrary number of instances at once. I also question the "frequently-accessed" part of this...
Doing so here, when I had to really reach to prove that any kind of problem might exist (
Well the first and second are the same, but yes, .NET Framework 2.0, 3.5 Client, and 3.5 Compact Framework cannot benefit from this change. And yeah there are two state variables, but I don't think we can get away with fewer variables in our own code... we trade away an
I agree.
re: "just volatile", it's not quite enough: without any locking, multiple threads entering the getter in the "we need to initialize it" path at the same time will race to initialize the result, and different threads might wind up overwriting the instance with a different result; different calls may therefore different instances without something done explicitly to stop that. There are ways to address this problem.
Close enough.
I hope not. I hope that what we're doing is discussing this PR as opposed to doing nothing, since the timeline on this one has been, as I see it:
One thing that I've been trying not to discuss is the best way that |
|
I'm sorry I don't have time for a more complete response but here's a quick one on 2 points... Will be more available later to continue the conversation.
I may be wrong, but the code in #39 first looks at To me it seems like a simple implementation of double-checked locking, which is fairly standard for singletons (or singleton-like situations like this). As I wrote above, I don't really have that much against using Lazy here, but as long as we're discussing synchronization intricacies it may good to clearly state the scenario which you think will fail if the code in #39 is used.
That's fair enough. I have little (or more precisely no) knowledge of NTS and how frequently this field actually gets read - I was under the impression that it isn't trivial. And any sort of a global singleton-like value automatically feels like it could become a bottleneck in some user scenarios. Once again, I think the techniques for avoiding contention are well-known and tested, but if you feel this field really doesn't get accessed enough to care, then there's no reason to look at |
Sorry, I may have misinterpreted your comment. When you said "just volatile", I thought you may have been referring to something other than #39.
I don't think such a scenario exists. I believe that #39 is an acceptable solution (or, well, it was when I had reviewed it previously; since then, more lines were changed), and my previous comment said as much. OK, if I'm being completely honest, I was a little annoyed at the changes that weren't related to making the getter avoid a See the timeline in my previous comment for why I opened #40 despite that.
I agree, but I wanted to get a "fix" in anyway to hopefully show that I'm willing to help address the honest concerns that you bring to the table. |
Oh, my bad... I think I should have been more explicit. I was only referring to the volatile+locking writes solution in #39.
I'm with you there - I always prefer PRs (or at least commits) to do just one change.
Sure, I really didn't think otherwise at any point - the discussion was valuable and it was interesting to see the benchmarks that came out of it... I guess that we have the following options at this point:
I personally vote for merging #39 (after changes) due to the reasons given above - it has some non-major advantages over #40 and is just as effective. But ultimately the decision is obviously @FObermaier and @airbreather's... |
|
Abandoned in favor of #39. |
On platforms that support it, anyway.
I got uncomfortable with how much discussion there was about performance, compared to how few measurements were being thrown around. So I threw all the implementations we've been discussing and ran a benchmark. Here were the results:
The specific implementations:
NoChangeis... well, no change.PR39is from Improved performance of the Instance property #39LazyTis this PR.LazyInitializer:LazyInitializer.EnsureInitialized(ref _instance, ref _initialized, ref _lockObject2, ReflectInstance);, where_lockObject2is a non-volatilenon-readonlyobjectfield pre-initialized tonew object().Appendix A has the exact details about how I ran this, but the short version is that the timings are from running 1024 concurrent threads, each of which repeats 1024 times: "fetch
GeometryServiceProvider.Instance, throw if it wasnull, then yield".This was designed to show the absolute worst case of contention that I can think of. Both the absolute and the relative numbers may be interesting for discussion.
Finally, note that
net20,net35-client, andnet35-cfwould sit on theNoChangelines, other targets would sit on theLazyTlines, and the .NET Standard ones would be on theLazyTline with "Preinit = true".Appendix A: Benchmark program
DummyGeometryServicesis a dummy implementation ofIGeometryServicesin the benchmark EXE.