Great!
I'm not in my development machine now, but when I get back home I'll check
it out and merge it.
Em 27/10/2013 03:42, "Glen Parker" notifications@github.com escreveu:

Francisco,

I was having trouble with Npgsql's ability to try protocol version 3, and
then fall back to version 2, since NpgsqlConnectionStringBuilder defaulted
to version 3. I changed ConnectionStringBuilder to work properly here, , as
well as adding a constructor to NpgsqlConnection.

So now, Npgsql once again attempts to automatically fall back to version 2
even when using ConnectionStringBuilder to set up the connection. However,
it's broken in another way as well, which I have fixed in PR #83#83
.

-Glen

You can merge this Pull Request by running

git pull https://github.com/glenebob/Npgsql2 connection_string_builder_2

Or view, comment on, or merge it at:

#85
Commit Summary

• NpgsqlConnectionStringBuilder

File Changes

• M src/Npgsql/NpgsqlConnection.cshttps://github.com/Connection string builder #85/files#diff-0(53)
• M src/Npgsql/NpgsqlConnectionStringBuilder.cshttps://github.com/Connection string builder #85/files#diff-1(211)

Patch Links:

• https://github.com/franciscojunior/Npgsql2/pull/85.patch
• https://github.com/franciscojunior/Npgsql2/pull/85.diff

Hi Glen!

I think this PR is almost ready to go. Just some comments:

1. Please, would you mind to explain more about the "Don't put values into base unless they are different from the value default, and also from that type's default value."
2. I wonder if there will be any backward break by changing the value of the encoding property from UNICODE to UTF-8?
3. Maybe it would be better to add some comments about this default values strategy change: The values which aren't specified in the static constructor have their default from the type system.

On Tue, Nov 12, 2013 at 9:51 AM, Francisco Figueiredo Jr. <
notifications@github.com> wrote:

Hi Glen!

I think this PR is almost ready to go. Just some comments:

Please, would you mind to explain more about the "Don't put values
into base unless they are different from the value default, and also from
that type's default value."

It's an attempt to keep the actual connection string as uncluttered as
possible, by not putting in values that the parser will derive via
default. The result is a more terse, more readable connection string,
which should offer a small performance improvement in the connection string
cache. Let's look at some examples, but first a couple definitions. I
like to think of the type default as the "implicit" default for any CS
value of that type (integers implicitly default to zero). Any default that
is specified other than the implicit default, is an "explicit" default.

1. Port=5432: Implicit default is zero, explicit default is 5432. Since
   5432 is different than one of those defaults, put it in the string,
2. Password=: There is no explicit default, and is the
   implicit default for string; do not put this value in the connection string.

Any value not explicitly represented in the connection string will result
in the default value for that value being used, so default values don't
need to be represented at all to achieve the correct behavior.

I felt that it would be confusing to not represent values (such as Port)
that match the explicit default when that default. Leaving the Port=5432
value out would be less readable IMO.

You'll notice I removed a bunch of explicit defaults, because if the
implicit default does the job, there's no reason to declare it explicitly.

I wonder if there will be any backward break by changing the value of
the encoding property from UNICODE to UTF-8?

Well, since the value hasn't been changeable for a long time, and since
"UNICODE" isn't even an encoding, I assume not. For it to break anything,
there would need to be client software somewhere checking for an encoding
that doesn't exist.

Maybe it would be better to add some comments about this default
values strategy change: The values which aren't

specified in the static constructor have their default from the type
system.

Sure, I can add some comments.

GMail seems to have messed up the numbered list in the quoted text...

-Glen

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/85#issuecomment-28316309
.

Got it! Thank you for the explanations, Glen.

I was a little worried about the complexity of the SetValue() method. But now I understand your motivations.

On Wed, Nov 13, 2013 at 8:25 AM, Francisco Figueiredo Jr. <
notifications@github.com> wrote:

Got it! Thank you for the explanations, Glen.

I was a little worried about the complexity of the SetValue() method. But
now I understand your motivations.

I was concerned with that, too. Hopefully I offset the complexity with
readability :)

-Glen

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/85#issuecomment-28409017
.

Great!

Thanks for the comments in the code. They will help a lot future changes.

Another thing I notice is about the performance NpgsqlConnectionStringBuilder.

In a test I did here, the creation of NpgsqlConnectionStringBuilder from a connection string improved 349%! Awesome!

On the down side, the performance of SetValue method had a performance drop of 38%. I think the call to TypeDefaultValue (which calls Activator.GetInstance()) method could be the culpirit ?

This is the test I used in the SpeedTests class:

[Test]
        public void NpgsqlConnectionStringBuilderPerformance()
        {
            string connString = "server=127.0.0.1;user id=user;password=password;database=database";

            using (var metrics = TestMetrics.Start(TestRunTime, true))
            {
                while (!metrics.TimesUp)
                {
                    NpgsqlConnectionStringBuilder n = new NpgsqlConnectionStringBuilder(connString);
                    metrics.IncrementIterations();
                }
            }

            NpgsqlConnectionStringBuilder n2 = new NpgsqlConnectionStringBuilder(connString);

            using (var metrics = TestMetrics.Start(TestRunTime, true))
            {
                while (!metrics.TimesUp)
                {
                    n2.UserName = "test";

                    metrics.IncrementIterations();
                }
            }
        }

And these are the results I got running from master and from your PR:

master

Creating test database schema
***** NpgsqlTests.SpeedTests.NpgsqlConnectionStringBuilderPerformance
Elapsed: 00:10.00, CPU: 00:09.999664, Queries: 149819; 14981,10/second, 14982,40/CPU second
Elapsed: 00:10.00, CPU: 00:09.984064, Queries: 14032008; 1403198,91/second, 1405440,51/CPU second
Total test running time: 20010ms


PR 85
***** NpgsqlTests.SpeedTests.NpgsqlConnectionStringBuilderPerformance
Elapsed: 00:10.00, CPU: 00:09.984064, Queries: 523288; 52326,02/second, 52412,32/CPU second
Elapsed: 00:10.00, CPU: 00:09.968463, Queries: 8732740; 873273,27/second, 876036,68/CPU second
Total test running time: 20013ms

I'm thinking about the scenario where the user sets a lot of value through accessors or properties.

Or maybe I'm over thinking as usual... :)

On Thu, Nov 14, 2013 at 8:48 AM, Francisco Figueiredo Jr. <
notifications@github.com> wrote:

Great!

Thanks for the comments in the code. They will help a lot future changes.

Another thing I notice is about the performance
NpgsqlConnectionStringBuilder.

In a test I did here, the creation of NpgsqlConnectionStringBuilder from a
connection string improved 349%! Awesome!

On the down side, the performance of SetValue method had a performance
drop of 38%. I think the call to TypeDefaultValue (which calls
Activator.GetInstance()) method could be the culpirit ?

Slower!!! We can't have that.

I added another test, maybe you can try it out on your system... It tests
value retrieval performance.

        using (var metrics = TestMetrics.Start(TestRunTime, true))
        {
            while (!metrics.TimesUp)
            {
                string un = n2.UserName;

                metrics.IncrementIterations();
            }
        }

So after a bunch more reworking, I was able to get both of your tests to
run faster than master, and faster then the previous commit, and my new
test as well. Take a look :)

In addition, I discovered a couple bugs that slipped past. Clone() was
broken, and I think the previous commit introduced a problem with Clear(),
where some of the values would not have been properly reset to defaults.

-Glen

This is the test I used in the SpeedTests class:

[Test]
public void NpgsqlConnectionStringBuilderPerformance()
{
string connString = "server=127.0.0.1;user id=user;password=password;database=database";

        using (var metrics = TestMetrics.Start(TestRunTime, true))
        {
            while (!metrics.TimesUp)
            {
                NpgsqlConnectionStringBuilder n = new NpgsqlConnectionStringBuilder(connString);
                metrics.IncrementIterations();
            }
        }

        NpgsqlConnectionStringBuilder n2 = new NpgsqlConnectionStringBuilder(connString);

        using (var metrics = TestMetrics.Start(TestRunTime, true))
        {
            while (!metrics.TimesUp)
            {
                n2.UserName = "test";

                metrics.IncrementIterations();
            }
        }
    }

And these are the results I got running from master and from your PR:

master

Creating test database schema
***** NpgsqlTests.SpeedTests.NpgsqlConnectionStringBuilderPerformance
Elapsed: 00:10.00, CPU: 00:09.999664, Queries: 149819; 14981,10/second, 14982,40/CPU second
Elapsed: 00:10.00, CPU: 00:09.984064, Queries: 14032008; 1403198,91/second, 1405440,51/CPU second
Total test running time: 20010ms

PR 85
***** NpgsqlTests.SpeedTests.NpgsqlConnectionStringBuilderPerformance
Elapsed: 00:10.00, CPU: 00:09.984064, Queries: 523288; 52326,02/second, 52412,32/CPU second
Elapsed: 00:10.00, CPU: 00:09.968463, Queries: 8732740; 873273,27/second, 876036,68/CPU second
Total test running time: 20013ms

I'm thinking about the scenario where the user sets a lot of value through
accessors or properties.

Or maybe I'm over thinking as usual... :)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/85#issuecomment-28500329
.

Awesome, Glen! You are an optimization master!!