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

Don't initialize performance counters by default #1435

Closed
cskujbus opened this Issue Feb 9, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@cskujbus

cskujbus commented Feb 9, 2017

Hi,
testing Npgsql 3.1.9 and 3.2 with the following snippet:

var sw = new Stopwatch();
sw.Start();
using (var conn = new NpgsqlConnection("server=localhost;user id=u;password=p;database=db"))
{
}
sw.Stop();
Console.WriteLine("Conn: {0}ms", sw.ElapsedMilliseconds);

3.1.9 gives about 40ms, 3.2 gives 600ms. At a cold start the difference is even greater.

PostgreSQL version: 9.6
Operating system: Windows 8.1

@roji roji self-assigned this Feb 10, 2017

@roji roji added the performance label Feb 10, 2017

@roji roji modified the milestone: 3.2.1 Feb 10, 2017

@roji roji added the invalid label Feb 10, 2017

@roji

This comment has been minimized.

Show comment
Hide comment
@roji

roji Feb 10, 2017

Member

Your benchmark is flawed in several ways...

First, simply creating an NpgsqlConnection without opening it doesn't do much - there's no I/O to PostgreSQL or anything. I'm assuming you were interested in actually connecting and not just in the instantiation of an NpgsqlConnection.

Second, you can't do a benchmark with just one iteration - that will give you random garbage results. You need to run it a very large number of times. You also need to account for warmup - you need to execute the code several times before starting measurements. There are other important things to be aware of when running benchmarks, and because it's not trivial it's really recommended that you use BenchmarkDotNet for benchmarking, as it takes care of everything for you.

As your benchmark stands, you're probably measuring warmup times as the JIT compiles Npgsql code, which doesn't say much about Npgsql performance. I rewrote your benchmark to run the same code 100000 times, and (unsurprisingly) got sub-millisecond times for both Npgsql 3.1.9 and 3.2.0.

It's great people are looking at Npgsql performance - performance was a key focus of the 3.2.0 release... I'd definitely welcome any reports of problematic behavior with benchmarks.

Member

roji commented Feb 10, 2017

Your benchmark is flawed in several ways...

First, simply creating an NpgsqlConnection without opening it doesn't do much - there's no I/O to PostgreSQL or anything. I'm assuming you were interested in actually connecting and not just in the instantiation of an NpgsqlConnection.

Second, you can't do a benchmark with just one iteration - that will give you random garbage results. You need to run it a very large number of times. You also need to account for warmup - you need to execute the code several times before starting measurements. There are other important things to be aware of when running benchmarks, and because it's not trivial it's really recommended that you use BenchmarkDotNet for benchmarking, as it takes care of everything for you.

As your benchmark stands, you're probably measuring warmup times as the JIT compiles Npgsql code, which doesn't say much about Npgsql performance. I rewrote your benchmark to run the same code 100000 times, and (unsurprisingly) got sub-millisecond times for both Npgsql 3.1.9 and 3.2.0.

It's great people are looking at Npgsql performance - performance was a key focus of the 3.2.0 release... I'd definitely welcome any reports of problematic behavior with benchmarks.

@roji roji closed this Feb 10, 2017

@cskujbus

This comment has been minimized.

Show comment
Hide comment
@cskujbus

cskujbus Feb 11, 2017

Thank you for your answer. I'd just like to add that, of course, I opened the connection and made some queries from the database, but those took the same time with both versions. I did not want to make a very precise benchmark, just make a throw away console application. I noticed that the 3.2 application started slower than the 3.1.9. So I wanted to comment about application startup, sorry, I wasn't clear enough about that.

cskujbus commented Feb 11, 2017

Thank you for your answer. I'd just like to add that, of course, I opened the connection and made some queries from the database, but those took the same time with both versions. I did not want to make a very precise benchmark, just make a throw away console application. I noticed that the 3.2 application started slower than the 3.1.9. So I wanted to comment about application startup, sorry, I wasn't clear enough about that.

@candritzky

This comment has been minimized.

Show comment
Hide comment
@candritzky

candritzky Feb 17, 2017

With 3.2.* I have a > 5(!) seconds delay after the first query results are returned. This is caused by the call to PerformanceCounterCategory.Exists(Counter.DiagnosticsCounterCategory) in the static Counters constructor when the performance counter is not present. The problem can be solved by installing the Npgsql.msi (which creates the Windows performance counter). But I don't want to run the Npgsql.msi installer, I just want to include the Npgsql.dll with my application.

Would be great to have an option/feature switch to turn off Npgsql performance counters completely (without the performance penalty of the PerformanceCounterCategory.Exists call), e. g. through a connection string option.

candritzky commented Feb 17, 2017

With 3.2.* I have a > 5(!) seconds delay after the first query results are returned. This is caused by the call to PerformanceCounterCategory.Exists(Counter.DiagnosticsCounterCategory) in the static Counters constructor when the performance counter is not present. The problem can be solved by installing the Npgsql.msi (which creates the Windows performance counter). But I don't want to run the Npgsql.msi installer, I just want to include the Npgsql.dll with my application.

Would be great to have an option/feature switch to turn off Npgsql performance counters completely (without the performance penalty of the PerformanceCounterCategory.Exists call), e. g. through a connection string option.

@roji

This comment has been minimized.

Show comment
Hide comment
@roji

roji Feb 17, 2017

Member

Thanks for narrowing it down to PerformanceCounterCategory.Exists(), I'll take a look.

Member

roji commented Feb 17, 2017

Thanks for narrowing it down to PerformanceCounterCategory.Exists(), I'll take a look.

@roji roji reopened this Feb 17, 2017

@roji roji added this to the 3.2.2 milestone Feb 17, 2017

@roji roji removed the invalid label Feb 17, 2017

@roji

This comment has been minimized.

Show comment
Hide comment
@roji

roji Feb 17, 2017

Member

Seems like the slow performance is unavoidable: http://stackoverflow.com/questions/2655067/performancecountercategory-exists-is-very-slow-if-category-doesnt-exists

Will continue looking into it, if there's no workaround I'll make perf counters opt-in via a parameter as suggested.

Member

roji commented Feb 17, 2017

Seems like the slow performance is unavoidable: http://stackoverflow.com/questions/2655067/performancecountercategory-exists-is-very-slow-if-category-doesnt-exists

Will continue looking into it, if there's no workaround I'll make perf counters opt-in via a parameter as suggested.

@roji roji changed the title from connection is slow in 3.2 compared to previous version to Performance counters drastically reduce startup time Feb 18, 2017

@roji

This comment has been minimized.

Show comment
Hide comment
@roji

roji Feb 18, 2017

Member

In #1448 a startup of time of 2000ms is reported.

SignalR/SignalR#1158 also describes some pretty disturbing behavior related to checking for the existence of a PerformanceCounterCategory.

Finally, apparently there are some exception issues with initialization (#1447), which should be solvable but still.

Making Performance Counters opt-in based on a new connection string parameter, Use Perf Counters.

Member

roji commented Feb 18, 2017

In #1448 a startup of time of 2000ms is reported.

SignalR/SignalR#1158 also describes some pretty disturbing behavior related to checking for the existence of a PerformanceCounterCategory.

Finally, apparently there are some exception issues with initialization (#1447), which should be solvable but still.

Making Performance Counters opt-in based on a new connection string parameter, Use Perf Counters.

@roji roji closed this in c66edde Feb 18, 2017

@roji roji changed the title from Performance counters drastically reduce startup time to Performance counters drastically slow down startup Feb 20, 2017

@roji roji changed the title from Performance counters drastically slow down startup to Don't initialize performance counters by default Mar 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment