Improved performance of the Instance property

[YohDeadfall]

As discussed in npgsql/npgsql#1840.

/cc @roji

[airbreather]

Do you have any sorts of measurements to justify this change?

As I'd mentioned in that other thread:

uncontended locks probably won't harm performance all that much, given what kinds of things usually happen to your CPU caches immediately after getting GeometryServiceProvider.Instance

To expand on that, I suspect that the difference between lock and volatile can be measured in nanoseconds to low microseconds, whereas it seems common for operations that consume it to last milliseconds.

Back to the context where this came up, you could cut the time spent in GeometryServiceProvider.Instance roughly in half by simply accessing it at most once (the default mode of UseNetTopologySuite will access it twice), and performance-sensitive callers can cut that down even further by providing their own instances.

Further, is that even a performance-sensitive method? I'm not familiar with INpgsqlTypeMapper, but it sounds to me like that extension method is the kind of thing that would typically be called fewer than a dozen times in a single application, likely before the CPU caches even have anything worth saving.

Also, aren't we talking about a database client anyway? Don't all of these things tend to happen very shortly before the CPU goes to sleep for a few milliseconds waiting on network I/O?

I'm not yet convinced that there's a situation where the cost of lock is significant enough, relative to the cost of volatile, to justify sitting here seriously thinking about the ramifications of this optimization... not because I think it's likely that there's anything horribly wrong here, but rather because lock is just so good at what it does.

[airbreather]

Foreword: I haven't actually thought this through completely.

I don't actually think we need this to be volatile, and if we think that this property is going to be "hot", then we actually don't really want it to be volatile. Every other read/write of this field occurs immediately after a Monitor.Enter call, which triggers a full memory barrier itself, so the volatile semantics seem to only affect the hot path.

So it seems to me that the worst that can happen without volatile is that every CPU comes in here with a stale cache and observes an uninitialized value, then they all hit the lock, then one of them initializes it properly (slow), then every other CPU sees the value from the other thread one-by-one (fast), then for the rest of the application every access is as fast as it can possibly be.

It seems like that's also the worst-case for volatile, except that at the end, every access for the rest of the application still has to go through a partial memory barrier. I don't actually see a window where volatile helps at all, compared to no volatile...

[roji]

The running time of lock vs. volatile isn't really the important factor - it's the contention that lock entails and which volatile doesn't. So if a lot of threads are reading the property at the same time, they will all block, accessing it one by one. This is especially problematic in an initialization --interactive scenario where you know writing will never (or very rarely) occur.

That's why it's important for the read to not lock..

[YohDeadfall]

@roji already said what the idea is, but FYI locks also use memory barriers internally on a multiprocessor machine and interlocked too.

[airbreather]

no need for the = instance part of this

[YohDeadfall]

Yeah, fixed.

[airbreather]

it bothers me that the entire change can't just be to change the top of the file to this, because Lazy<T> was only added in .NET 4.0...

private static readonly Lazy<IGeometryServices> DefaultInstance =
    new Lazy<IGeometryServices>(ReflectInstance);

private static volatile IGeometryServices _instance;

public static IGeometryServices Instance
{
    get => _instance ?? DefaultInstance.Value;
    set => _instance = value ?? throw new ArgumentNullException(nameof(value));
}

[YohDeadfall]

It's a bit heavy thing which isn't used internally by Microsoft. Stack Overflow has stopped using it too. LazyInitializer is much better.

Should I squash commits?

[airbreather]

It still doesn't make me happy with the state of GeometryServiceProvider in general, and I'm personally not convinced that either there will ever be enough lock contention over a field access for this to provide a significant improvement, or this is a more appropriate solution than the consumer doing their own thing or using the NTS version.

That said, I have no technical issues with this, and I'm OK with merging it.

@FObermaier do you have any concerns?

[airbreather]

It's a bit heavy thing which isn't used internally by Microsoft. Stack Overflow has stopped using it too. LazyInitializer is much better.

ILSpy suggests that Lazy<T> is used by plenty of Microsoft's internal stuff. It's designed for situations that look exactly like ours: heavy initialization that might not even be used, and very frequent accesses after initialization.

Should I squash commits?

Doesn't matter to me, personally.

[FObermaier]

I know Lazy<T> but I'm a bit confused about LazyInitializer, what is it, where is it used?

[airbreather]

I know Lazy<T> but I'm a bit confused about LazyInitializer, what is it, where is it used?

Here's a bit about it. I'm not seeing much usage of it compared to Lazy<T>.

Edit: it's also a 4.0+ thing, so we can't use that for GeoAPI either, at least not without keeping "something" like the current stuff around for the net20, net35-client, and net35-cf targets.

[YohDeadfall]

It's a static class which is used for lazy initializations but without allocating memory to store the state, i.e. you choose where to store a synchronization object and an initializing value. In the current case it makes sense because there is a setter with the lock inside.

[FObermaier]

@YohDeadfall, I assume you left the lock in the code because it has been there in the first place? For some reason I can't tell from the version history if it has been there ever since. I assume the initial goal was to prevent running the initalization code more than once. but ... who cares if it does.

We need to make sure that we have the Instance member always returning a properly initialized member. If there are other reasons for the lock that I'm not aware of, I'd hesitate to introduce a restriction on the frameworks we can use, so it'd be either Lazy<T> or the code as is.

[airbreather]

I assume you left the lock in the code because it has been there in the first place? For some reason I can't tell from the version history if it has been there ever since. I assume the initial goal was to prevent running the initalization code more than once. but ... who cares if it does.

I think it's to avoid this:

1. Thread 1 reads Instance as null and starts initialization.
2. While thread 1 is reading every single assembly in the app domain, thread 2 sets Instance explicitly.
3. After thread 2 has set Instance, thread 1 finds a different one and overwrites what thread 2 has set.

Without the lock, it's possible for a library consumer to set an explicit value that gets replaced by an auto-fetched value which may not be appropriate.

I'd hesitate to introduce a restriction on the frameworks we can use, so it'd be either Lazy<T> or the code as is.

Lazy<T> is also missing from net20, net35-client, and net35-cf.

One thing we could do is have everybody but those three target frameworks use the Lazy<T> idea, and then have those three target frameworks keep doing the lock thing: e405106.

If we wanted to do that fancy LazyInitializer thing, then that also rules out portable40-net40+sl5+win8+wp8+wpa81 and portable40-net403+sl5+win8+wp8+wpa81.

[YohDeadfall]

Without the lock, it's possible for a library consumer to set an explicit value that gets replaced by an auto-fetched value which may not be appropriate.

Right, but it can be removed (only in the setter) if Interlocked.CompareExchange is used by the initializer to set the instance field. Will do tomorrow.

If there are other reasons for the lock that I'm not aware of, I'd hesitate to introduce a restriction on the frameworks we can use, so it'd be either Lazy<T> or the code as is.

You can't use Lazy<T> because of the setter. `Lazy doesn't have it. The synchronization object will be used by bootstrappers internally since they can't use the property directly. I think it would be better to share my bootstrapper design idea in a new issue.

[airbreather]

You can't use Lazy<T> because of the setter.

#40

[YohDeadfall]

Ah, that way (: From my point of view having a Lazy<T> and it's internal state in memory only to initialize a value isn't a good idea.

[airbreather]

@FObermaier I've reviewed this. I'd like to merge it. Is that OK with you?

[FObermaier]

@airbreather , no I don't have objections. @YohDeadfall, @airbreather and @roji, thanks for your input.