Handler rework #5123
Handler rework #5123
Conversation
NinoFloris
force-pushed
the
handler-rework
branch
2 times, most recently
from
16b8193 to
be9cfe4
Compare
NinoFloris
force-pushed
the
handler-rework
branch
3 times, most recently
from
d395ef6 to
5389a54
Compare
NinoFloris
force-pushed
the
handler-rework
branch
14 times, most recently
from
0ef77f7 to
6898385
Compare
NinoFloris
force-pushed
the
handler-rework
branch
from
897d566 to
945ad65
Compare
vonzshik
left a comment
vonzshik
left a comment
There was a problem hiding this comment.
I have a general grasp on architecture and I'm more or less ok with it. There might be some small problems or nits (like missing sealed or some other things) but they can be fixed at a later date. While I haven't looked through every single file I'll try to do that after PR is merged.
| @@ -52,6 +53,7 @@ public sealed class NpgsqlDataReader : DbDataReader, IDbColumnSchemaGenerator | |||
| internal ReaderState State = ReaderState.Disposed; | |||
|
|
|||
| internal NpgsqlReadBuffer Buffer = default!; | |||
There was a problem hiding this comment.
What about setting it to connector's buffer in the constructor?
There was a problem hiding this comment.
I'm not sure, I haven't touched this line.
There was a problem hiding this comment.
Reason being, you added a ReferenceEqual check to NpgsqlDataReader to replace the buffer if it differs from the one connector has. My thoughts were that it might help pgo a bit to assume that the buffer almost never changes.
There was a problem hiding this comment.
I added the check so we avoid the write barrier of unconditionally setting the buffer reference. Afaik PGO cannot assume anything that isn't a static readonly will stay constant (or do anything fancy to optimize a reference check).
There was a problem hiding this comment.
I added the check so we avoid the write barrier of unconditionally setting the buffer reference. Afaik PGO cannot assume anything that isn't a static readonly will stay constant (or do anything fancy to optimize a reference check).
Yeah, currently it can reason about the implementation of the interface (testing first again the most probable), don't think there is anything what I described yet (doesn't mean it will not be implemented at some point).
Anyway, that's more of a suggestion (we don't really lose anything from it) and I'm okay not doing it.
NinoFloris
force-pushed
the
handler-rework
branch
from
31b162a to
f244c63
Compare
NinoFloris
force-pushed
the
handler-rework
branch
from
5a24079 to
346386a
Compare
| } | ||
|
|
||
| protected override void WriteCore(PgWriter writer, NpgsqlCidr value) | ||
| => NpgsqlInetConverter.WriteImpl(writer, (value.Address, value.Netmask), isCidr: false); |
There was a problem hiding this comment.
shouldn't it be isCidr: true?
There was a problem hiding this comment.
There was a problem hiding this comment.
Yeah, looks like a mistake - submitted #5338, thanks @MaceWindu. It seems that this is actually ignored on the PG side (there's a comment that says so at least), but definitely good to correct this.
Should bring Npgsql back to or better than pre #5123 performance numbers
6 participants
Just a little monster PR 😄
There is a ton to do still but I want to start doing the remainder of the work in public.Let's start off with some general context, we're working to get Npgsql in a good state for NativeAOT, which means no reflection and ideally a small binary footprint. We've gotten quite far with the former, though things like arrays currently flat out do not work under nativeaot as we are not rooting the types correctly.
The footprint issue however is a lot more intricate. First off is that our handler architecture relies on GVMs, generic virtual methods, which are a huge no-no for footprint (don't trust me, read it from the experts)
Secondly we're throwing around async statemachines everywhere, as a result each valuetype T that we implement creates a huge amount of bloat, I've touched on this here in more detail dotnet/runtime#83902
And finally as we're coupling the concept of a postgres type to a single handler type we are unable to split handlers up by clr types it supports, making it difficult to create smaller baselines without having to create a new bespoke set of handlers.
A good example is our json support, we always support reading and writing strings but each serializer has their own handler that adds support for more types. Additionally if somebody wanted to use both STJ and Newtonsoft for whatever reason they would be responsible for creating a composite of those handlers again, and so on.
Another result of this approach is the inability to selectively opt into streaming support, a handler always supports async io, just in case one of the INpgsqlTypeHandler implementations needs it, and as a result we always pay for the size bloat.
The design we have today made a lot of sense when it was created but with trimming and AOT having become a thing we have some redesigning to do and that's what this PR hopes to bring.
We're just at the point where the general shape looks good and the main project + tests compile. It's fleshed out enough that everybody should be able to make sense of the direction this is going in, after which it's a good moment to discuss the finer points.
The next step for me is to get some first tests to pass, it shouldn't be far off but it needs PgReader/PgWriter to be actually implemented on top of NpgsqlRead/WriteBuffer.
Afterwards I have a tentative list to go through (help appreciated when we're at this point!):
Closes #4340, closes #4341, closes #3898, closes #4970, closes #5099, closes #5009, closes #4988, closes #5125, closes #4893, closes #4543, closes #3782, closes #3775, closes #3656, closes #3648, closes #1965, closes #2013, closes #2355, closes #3639, closes #3679, closes #4663, closes #4711, closes #4870, closes #4862, closes #5004, closes #4988, closes #5141, closes #5136, closes #5139, closes #5142, closes #5143, closes #5210, closes #5137, closes #4626