-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
SqlDecimal generates garbage during SqlBulkCopy #1034
Comments
I think it's fine to still open issues here but note that most Sqlclient work is now in https://github.com/dotnet/SqlClient and then they sometimes port back here. |
Thanks @danmosemsft, good to know. I thought it would be appropriate to create here as the SqlDecimal class is defined in this repo, not SqlClient. I also figured the idea to expose decimal.GetBitsSpan belonged here as well. Should I close this one and create one over there and they’ll create a PR into here? Just not sure of the correct process. Apologies for any inconvenience and appreciate the help! |
Good point - let's leave it here and owners of this area can respond. |
It might be a good idea to spanify some of the other shared types in the same pass to provide everyone with allocation free paths where possible. It may also address some of the unpleasant hacks used by SqlClient in SqlTypeWorkArounds |
@cheenamalhotra @David-Engel has there already been a conversation about moving the "SqlTypes" (SqlInt32, SqlDecimal...) to https://github.com/dotnet/sqlclient? From a very cursory look, it seems like these do belong as part of SqlClient - no other ADO.NET provider uses them as far as I know. There are some references to SqlInt32 in DataColumn and other supporting infrastructure (System.Data.Filters), though I'm not sure as to the extent of the dependency. IMHO we should understand if we can/should move these types to SqlClient before considering any changes in System.Data.Common. /cc @ajcvickers (any historical knowledge on this?) |
I'm surprised that no other providers use them, does everyone make their own versions? |
@Wraith2 I haven't thoroughly investigated these, but AFAIK these types are the SqlClient "provider-specific types". Npgsql does define some provider-specific types as well; for example, NpgsqlDateTime can represent a PostgreSQL timestamp, which has different precision and range than BCL DateTime. Since different databases have different characteristics for data types, I can't really see how something like SqlDateTime could ever be useful as a cross-database type (I also don't understand what SqlInt32 actually provides over .NET int...). So to summarize, in my mental model, each provider can/should expose provider-specific types where their database type can't be properly represented with a built-in BCL type; these should follow the standard ADO.NET naming convention (SqlClient has SqlCommand, so it makes sense for it to also have SqlDateTime; Npgsql has NpgsqlCommand and NpgsqlDateTime). Unfortunately it seems that at some point the boundary between System.Data.Common and SqlClient got a bit blurry, which is why we're in this confusing situation. |
Hi @roji We have discussed about porting them to Microsoft.Data.SqlClient.SqlTypes namespace, but never planned to execute it due to it's impact and usage in existing client applications. But there are definitely no plans to move System.Data.SqlTypes out of System.Data.Common and change anything for System.Data.SqlClient driver. And, even if we plan to add them in Microsoft.Data.SqlClient.SqlTypes namespace someday, that wouldn't replace System.Data.SqlTypes as they'll continue to be used with System.Data.SqlClient, so fixing them here would be ideal IMO. |
My (outsider's) understanding is that these types existed to represent nullable values (e.g., see |
Interesting I didn't know that... I may have expressed myself badly... Of course there's no question of removing these types from System.Data.Common, as that would be a major breaking change (and System.Data.SqlClient would not work anymore at all). I was thinking only about whether these types should be copied across to Microsoft.Data.SqlClient, in which case M.D.SqlClient would use them and S.D.SqlClient would continue using the version in System.Data.Common; essentially doing the exact same thing with all other SqlClient-specific types. This would unfortunately mean another transition for (some) users of M.D.SqlClient, as applications using M.D.SqlClient still reference the SQL types from S.D.C and would need to change. On the upside, this would cleanly bring everything SqlClient to where it belongs - M.D.SqlClient - and allow the SQL types to evolve at the right pace with the new SqlClient itself (rather than be bound to the BCL as before).
I agree that this could be the blocking factor here - we'd need to investigate what the exact impact here would be.
Right, but in my mind the intention isn't to continue evolving System.Data.SqlClient - the proposals on SqlDecimal here aren't even bugfixes, only perf improvements. |
Currently the improvements will also apply to Microsoft.Data.SqlClient as we directly reference them too, so we're essentially improving both drivers. :) When we plan to add it to M.D.S (no near plans currently), that's when future improvements can be planned for M.D.S only. |
I think we might have missed the point, the original post is about |
@Wraith2 it's true that the original post proposes to add In any case, as it doesn't seem like these types are going to be moving any time soon, I guess it makes sense to optimize them here. |
@Wraith2 Did your PR linked above fix this issue? |
SqlDecimal was improved but I didn't add a new api because it hasn't been proposed or reviewed. So no. I do think it would be helpful to review various SqlTypes members including SqlDecimal and ensure we have span enabled paths for use. Recently I found that SqlBinary has a similar lack of access and that can have significant memory costs. |
There a few performance tweaks that could really help reduce heap allocations / GC pressure during SqlBulkCopy on SqlDecimal:
During SqlDecimal's construction, an int[] is created:
It would be great if
decimal
could expose aGetBitsSpan
so this could be stack allocated.During multiple operations invoked by various operations,
uint[]
is created instead of stackalloc'edex:
I'm happy to create a fork/PR to help get this in.
The text was updated successfully, but these errors were encountered: