High memory traffic because of boxed SqlGuid struct

[wilbit]

Is your feature request related to a problem? Please describe.

When our application is starting it loads a lot of data from DB and it causes memory consumption growth and high pressure to GC because of big memory traffic.
On the third place of killed objects is boxed System.Data.SqlTypes.SqlGuid struct.
These boxed values are 99.9% created when TdsParser is putting SqlGuid struct to SqlBuffer.SqlGuid property (see the image bellow).

Describe the solution you'd like

SqlBuffer already supports an efficient way to store Guid values. What I would like to do is:

• change implementation of Microsoft.Data.SqlClient.SqlBuffer
  • remove SqlGuid value from StorageType enum (taking the second alternative approach, removing of SqlGuid value from enum looks like a bad idea);
  • save Guid representation of SqlGuid to _value._guid (similar to what Guid setter does, but with checking if SqlGuid value is null)
  • read SqlGuid value from _value._guid

Describe alternatives you've considered

1. add a field of SqlGuid type to SqlBuffer, but it seems memory inefficient
2. same as the main approach, but without removing SqlGuid value from StorageType enum, in case it is really important. The only place where it looks important to me is s_dbTypeToStorageType mapping array in Microsoft.Data.SqlClient.Server.ValueUtilsSmi, and I am not sure if it is safe to change the mapping to SqlBuffer.StorageType.Guid. It seems like it is not safe, because implementation of SqlBuffer.Guid does not support nulls.

Additional context

• Microsoft.Data.SqlClient version is 5.1.0;
• We use Microsoft.Data.SqlClient together with NHibernate;
• Almost all our db tables contain Guid columns, so, memory traffic is really high in our case.

[wilbit]

The changes are simple. So, I can do it if/when the issue is approved.

[kf-gonzalez]

@wilbit All contributors are welcomed to create PRs; we will review them and address them

[Wraith2]

That's a bit strange. If you're calling IsDBNull it shouldn't be forcing the sql type it should be using the guid type and only creating the sql type if you request it. Can you provide a small repro?

[wilbit]

@Wraith2 , have you seen the stack trace on the image and code of SqlDataReader.TryReadColumnInternal(i: int_value, readHeaderOnly: true, forStreaming: false), TdsParser.GetNullSqlValue and SqlBuffer.set_SqlGuid?
I think it is a straight forward from the code base, especially from SqlBuffer.set_SqlGuid.

@kf-gonzalez I will prepare PR tomorrow.

[Wraith2]

I saw the trace. I just remember specifically optimizing guid in the past so that it doesn't allocate. It looks like this specific case slipped past me.

Looking at the code I think we should probably be calling SqlBuffer.SetToNullOfType(SqlBuffer.StorageType.SqlGuid) instead of using the SqlBuffer.set_SqlGuid property setter, that's going to box the SqlGuid.Null value every time we call because it's assigning the struct to the object member.
Can you try changing the two places in the codebase that call SqlBuffer.set_SqlGuid with SqlGuid.Null to use the setter and see if it works?

[wilbit]

I will have a look, @Wraith2

[wilbit]

Here is the PR: #2306, it removed boxing of SqlGuid 100%.

PS: I have also noticed, that SqlBinary has a very similar issue for SqlBinary.Null and not null values (it just affects us ~100 times less). I have a couple thoughts how to avoid boxing there too.

[Wraith2]

I noticed the SqlBinary thing too. I think it's more complex than SqlGuid but if you're willing to dive in and improve things it'll be appreciated.

[wilbit]

This is not fixed yet, because PR #2306 was reverted (#2311)