Conversation
There was a problem hiding this comment.
Pull request overview
This PR enables buffer bounds checking unconditionally in the low-level read buffer and simplifies buffered converter read/write paths by removing the explicit “buffer requirement respected” guard, relying on the new bounds checks to fail when attempting to read beyond what’s currently buffered.
Changes:
- Make
NpgsqlReadBufferbounds checks always-on (no longer gated by assertion toggles), and use them in additional read helpers. - Move the “read scope” (field-size) bounds-check toggle into
PgReaderand renameCheckBounds→CheckScopeBounds. - Remove
PgBufferedConverter<T>’s upfront “buffer requirement” enforcement checks for reads/writes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Npgsql/Internal/PgReader.cs |
Introduces CheckReadScopeBounds, renames/updates scope bounds checks, and adjusts some read helpers to use buffer APIs. |
src/Npgsql/Internal/PgBufferedConverter.cs |
Removes explicit buffered requirement checks from Read/Write. |
src/Npgsql/Internal/NpgsqlReadBuffer.cs |
Makes bounds checks unconditional and replaces some Debug.Assert calls with runtime checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
NinoFloris
force-pushed
the
enable-buffer-bounds-checks
branch
from
2a9fe13 to
4d22736
Compare
NinoFloris
force-pushed
the
enable-buffer-bounds-checks
branch
2 times, most recently
from
97a5c07 to
1afcfce
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
NinoFloris
force-pushed
the
enable-buffer-bounds-checks
branch
from
1afcfce to
2c5ed17
Compare
|
Vibe coded some faster read buffer bookkeeping so we can also enable field bounds checks. This adds minimal cost compared to a direct call to the read buffer (5% on an int read vs GetInt32()). Compared to the first iteration of this PR (just buffer bounds checks), we actually improved perf in absolute sense due to the faster bookkeeping. Benchmark used: https://gist.github.com/NinoFloris/7ecb7b966a66bff916502fa8f6614564 EDIT: removed accidental local commit working around SDK 10.0.200 issues. |
NinoFloris
force-pushed
the
enable-buffer-bounds-checks
branch
from
2c5ed17 to
94a35ab
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
# Conflicts: # src/Npgsql/Internal/PgBufferedConverter.cs
NinoFloris
force-pushed
the
enable-buffer-bounds-checks
branch
from
94a35ab to
b9a740d
Compare
And remove buffered requirement check. This can be done because any buffered converter that will actually attempt to read data that wasn't properly buffered will now just fail due to the buffer bounds checks instead.
From a perf perspective I couldn't find a regression in the types we benchmark, though I assume it might slightly impact complex structures like TsVector, which may do a lot of individual reads. For single read buffered converters it's actually an improvement because the bounds check is much cheaper than the requirement check.