Skip to content

Redo timestamp handling#3969

Merged
roji merged 10 commits into
mainfrom
UntilTheEndOfTime
Sep 13, 2021
Merged

Redo timestamp handling#3969
roji merged 10 commits into
mainfrom
UntilTheEndOfTime

Conversation

@roji
Copy link
Copy Markdown
Member

@roji roji commented Sep 9, 2021

@roji roji requested review from Brar and vonzshik as code owners September 9, 2021 16:54
@roji roji force-pushed the UntilTheEndOfTime branch from ec56687 to 5bb0fae Compare September 9, 2021 16:59
NinoFloris
NinoFloris reviewed Sep 9, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@NinoFloris NinoFloris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! As you know it's a change that gets a warm welcome from me, and I know it was a lot of extra work on your plate to get there, it's appreciated.

Comments below, I've reviewed everything except the tests as it's far too late over here for that 😄

Comment thread src/Npgsql.NodaTime/Internal/NodaTimeUtils.cs
Comment thread src/Npgsql/Internal/TypeHandling/TypeHandlerResolver.cs
Comment thread src/Npgsql/Internal/TypeHandling/TypeHandlerResolverFactory.cs
Comment thread src/Npgsql/TypeMapping/BuiltInTypeHandlerResolver.cs
Comment thread src/Npgsql/TypeMapping/ConnectorTypeMapper.cs Outdated
Comment thread src/Npgsql/TypeMapping/BuiltInTypeHandlerResolver.cs
@roji roji force-pushed the UntilTheEndOfTime branch from fa96555 to 5f27de5 Compare September 10, 2021 12:35
@roji roji force-pushed the UntilTheEndOfTime branch from 5f27de5 to bf76fd9 Compare September 10, 2021 12:50
@roji
Copy link
Copy Markdown
Member Author

roji commented Sep 10, 2021

FYI implemented the optimization for resolving value types generically (via NpgsqlParameter<T>), which bypasses the Dictionary lookup: here are our new resolution numbers:

|                   Method | NumPlugins |      Mean |     Error |    StdDev |  Gen 0 | Allocated |                                           
|------------------------- |----------- |----------:|----------:|----------:|-------:|----------:|                                           
|               ResolveOID |          0 | 10.252 ns | 0.0391 ns | 0.0366 ns |      - |         - |                                           
|      ResolveNpgsqlDbType |          0 | 13.495 ns | 0.2350 ns | 0.2308 ns |      - |         - |                                                                                                                                                                                        
|      ResolveDataTypeName |          0 | 21.024 ns | 0.0777 ns | 0.0727 ns |      - |         - |                                           
| ResolveClrTypeNonGeneric |          0 | 20.084 ns | 0.0921 ns | 0.0862 ns | 0.0055 |      24 B |                                           
|    ResolveClrTypeGeneric |          0 |  6.616 ns | 0.0451 ns | 0.0422 ns |      - |         - |

Pretty cool 🚀

@roji roji requested a review from NinoFloris September 10, 2021 15:19
vonzshik
vonzshik reviewed Sep 13, 2021
View reviewed changes
Comment thread src/Npgsql/Internal/TypeHandlers/DateTimeHandlers/DateTimeUtils.cs Outdated
Comment thread src/Npgsql/Internal/TypeHandlers/DateTimeHandlers/TimestampHandler.cs Outdated
Comment thread src/Npgsql/Internal/TypeHandlers/DateTimeHandlers/TimestampHandler.cs Outdated
roji and others added 3 commits September 13, 2021 18:38
@roji @vonzshik
Update src/Npgsql/Internal/TypeHandlers/DateTimeHandlers/TimestampHan…
7558e01 
…dler.cs

Co-authored-by: Nikita Kazmin <vonzshik@gmail.com>
@roji @vonzshik
Update src/Npgsql/Internal/TypeHandlers/DateTimeHandlers/TimestampHan…
0f75c53 
…dler.cs

Co-authored-by: Nikita Kazmin <vonzshik@gmail.com>
@roji @vonzshik
Update src/Npgsql/Internal/TypeHandlers/DateTimeHandlers/DateTimeUtil…
a1db651 
…s.cs

Co-authored-by: Nikita Kazmin <vonzshik@gmail.com>
@roji roji requested a review from vonzshik September 13, 2021 16:52
@roji roji merged commit a579c17 into main Sep 13, 2021
@roji roji deleted the UntilTheEndOfTime branch September 13, 2021 17:47
@roji
Copy link
Copy Markdown
Member Author

roji commented Sep 13, 2021

... and let the breakage begin ...

@Emill
Copy link
Copy Markdown
Contributor

Emill commented Sep 13, 2021

Nice! Just make sure to document this properly somehow to avoid surprises...

@roji
Copy link
Copy Markdown
Member Author

roji commented Sep 13, 2021

Oh yeah, definitely... This will be front and center on the release notes, and I'll be sure to put something there before actually releasing this in rc1...

Before that I also need to finish the corresponding EF Core changes...

@roji roji mentioned this pull request Sep 15, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vonzshik vonzshik vonzshik approved these changes

@Brar Brar Awaiting requested review from Brar Brar is a code owner

@NinoFloris NinoFloris Awaiting requested review from NinoFloris

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Redo timestamp handling Remove intermediate NpgsqlDateTime conversions in timestamp handlers

4 participants

@roji @Emill @NinoFloris @vonzshik