Repository navigation
PERF: Speed up output-converter batching and add CI coverage - #852
Open
Jahnvi Thakkar (jahnvi480) wants to merge 5 commits into
Open
Jahnvi Thakkar (jahnvi480) wants to merge 5 commits into
Jahnvi Thakkar (jahnvi480) wants to merge 5 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
PR Performance Report✅ Performance improved2 database tasks consistently improved across 2 measured environments. No consistent slowdowns were detected. 2 IMPROVEMENTS 0 SLOWDOWNS 2/2 ENVIRONMENTS Signal fingerprint
Coverage: 2 of 2 environments completed. Advisory result; does not block merging. Measured timings
Performance diagnosticsPhase times are inclusive diagnostics and must not be added together. They identify where measured time changed, not why it changed. Unix / SQL Server 202210,000 output-converted rows / fetchall(): py::fetchall::row_wrap -7.769 ms; ddbc::SQLDescribeCol_wrap -0.000 ms; ddbc::FetchBatchData::cache_column_metadata -0.000 ms. Unix / SQL Server 202510,000 output-converted rows / fetchall(): py::fetchall::row_wrap -7.898 ms; ddbc::FetchBatchData::SQLFetchScroll_call -0.249 ms; ddbc::FetchBatchData -0.131 ms. All database tasks and timingsUnix / SQL Server 2022
Unix / SQL Server 2025
Build and measurement detailsPR head:
A consistent change requires more than 20% median paired movement, at least 1 ms between the median runtimes, and at least 80% of pairs exceeding the relative threshold in the same direction. A slowdown without enough pair agreement is reported as inconsistent. The displayed change is the median of paired before-and-after ratios. It is not recalculated from the two displayed median runtimes. Both revisions use profiling-enabled builds on the same agent and database, with alternating order and discarded warmups. Results are diagnostic and do not represent production-wheel latency. Raw samples and logs are attached to the ADO run as |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Native ownership and dispatch changes require human review, with performance qualification on HOLD and cross-platform validation still limited.
No open issues 🎉
What changed in this PR
Optimizes output-converter fetching across the Python and native driver layers and adds CI benchmark coverage. The change remains draft-only and performance qualification is on HOLD.
Changes:
- Retains owned UTF-16LE bytes and adds guarded native Row construction.
- Adds converter correctness, fallback, mutation, ownership, and dispatch tests.
- Adds two converter-fetch benchmarks, expanding the registry to 24 tasks without changing comparison thresholds.
| File | Description |
|---|---|
| tests/test_output_converter_bytes.py | Tests converter semantics, ownership, and native dispatch. |
| tests/test_036_profiler_ci.py | Tests benchmark boundaries, cleanup, and reporting. |
| mssql_python/row.py | Delegates list-based conversion to native code. |
| mssql_python/pybind/output_converter.hpp | Implements owned text, conversion, and guarded Row construction. |
| mssql_python/pybind/ddbc_bindings.h | Adds deferred text handling to column processors. |
| mssql_python/pybind/ddbc_bindings.cpp | Passes converter maps through fetches and exposes helpers. |
| mssql_python/cursor.py | Integrates native converter paths and fallbacks. |
| eng/profiler_benchmarks/workloads.py | Adds fetchall and fetchmany converter workloads. |
| eng/profiler_benchmarks/report.py | Adds readable benchmark report labels. |
| eng/profiler_benchmarks/README.md | Documents workloads and measurement boundaries. |
Review effort: Balanced
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/pybind/ddbc_bindings.cppLines 3643-3651 3643 py::object text;
3644 if (OutputConverters::enabled(converters, i - 1))
3645 text = OutputConverters::text(textData, textSize);
3646 row.append(text ? text
! 3647 : FetchText::from_utf16_native(textData, textSize));
3648 LOG("SQLGetData: CHAR column %d fetched as WCHAR, "
3649 "length=%lu",
3650 i, (unsigned long)numCharsInData);
3651 } else {Lines 3823-3836 3823 // omits the terminator.
3824 const char* textData =
3825 reinterpret_cast<const char*>(dataBuffer.data());
3826 Py_ssize_t textSize =
! 3827 static_cast<Py_ssize_t>(numCharsInData * sizeof(SQLWCHAR));
! 3828 py::object text;
! 3829 if (dataType != SQL_SS_VARIANT &&
! 3830 OutputConverters::enabled(converters, i - 1))
! 3831 text = OutputConverters::text(textData, textSize);
! 3832 row.append(text ? text
3833 : FetchText::from_utf16_native(textData, textSize));
3834 LOG("SQLGetData: Appended NVARCHAR string "
3835 "length=%lu for column %d",
3836 (unsigned long)numCharsInData, i);Lines 4482-4490 4482 SQLRETURN FetchBatchData(SQLHSTMT hStmt, ColumnBuffers& buffers, const Metadata& columnNames,
4483 py::list& rows, SQLUSMALLINT numCols, SQLULEN& numRowsFetched,
4484 const std::vector<SQLUSMALLINT>& lobColumns,
4485 const std::string& charEncoding = "utf-16le", int charCtype = SQL_C_WCHAR,
! 4486 py::handle messages = {}, py::handle converters = {}) {
4487 PERF_TIMER("FetchBatchData");
4488 LOG("FetchBatchData: Fetching data in batches");
4489 SQLRETURN ret;
4490 {mssql_python/pybind/output_converter.hppLines 62-70 62 for (Py_ssize_t i = 0; i < PyList_GET_SIZE(values.ptr()); ++i) {
63 if (raw_text(PyList_GET_ITEM(values.ptr(), i))) {
64 py::object value = original(py::handle(PyList_GET_ITEM(values.ptr(), i)));
65 if (PyList_SetItem(values.ptr(), i, value.release().ptr()) < 0)
! 66 throw py::error_already_set();
67 }
68 }
69 return values;
70 }Lines 108-116 108 if (!replaced && i < PyList_GET_SIZE(result.ptr()) &&
109 raw_text(PyList_GET_ITEM(result.ptr(), i))) {
110 py::object restored = original(py::handle(PyList_GET_ITEM(result.ptr(), i)));
111 if (PyList_SetItem(result.ptr(), i, restored.release().ptr()) < 0)
! 112 throw py::error_already_set();
113 }
114 }
115 for (; i < PyList_GET_SIZE(result.ptr()); ++i) {
116 if (raw_text(PyList_GET_ITEM(result.ptr(), i))) {Lines 115-123 115 for (; i < PyList_GET_SIZE(result.ptr()); ++i) {
116 if (raw_text(PyList_GET_ITEM(result.ptr(), i))) {
117 py::object restored = original(py::handle(PyList_GET_ITEM(result.ptr(), i)));
118 if (PyList_SetItem(result.ptr(), i, restored.release().ptr()) < 0)
! 119 throw py::error_already_set();
120 }
121 }
122 return py::reinterpret_borrow<py::list>(result);
123 }Lines 201-211 201 return false;
202 PyObject* current_apply = PyDict_GetItemWithError(
203 PyModule_GetDict(bindings_.ptr()), apply_name_.ptr());
204 if (!current_apply) {
! 205 if (PyErr_Occurred()) throw py::error_already_set();
! 206 return false;
! 207 }
208 if (current_apply != apply_.ptr()) return false;
209 // Changing function.__code__ does not invalidate the owning type's tag.
210 for (size_t i = 0; i < code_.size(); ++i) {
211 if (code_[i] && PyFunction_GetCode(hooks_[i].ptr()) != code_[i].ptr())Lines 240-254 240 for (Py_ssize_t i = 0; i < PyList_GET_SIZE(rows_data.ptr()); ++i) {
241 py::object values = borrow(PyList_GET_ITEM(rows_data.ptr(), i));
242 PyObject* selected = PyDict_GetItemWithError(cursor_globals.ptr(), row_name_.ptr());
243 if (!selected) {
! 244 if (!PyErr_Occurred()) PyErr_SetString(PyExc_NameError, "name 'Row' is not defined");
! 245 throw py::error_already_set();
! 246 }
247 py::object row_class = borrow(selected);
248 py::object names = cursor.attr("_cached_result_columns");
249 if (!PyList_Check(values.ptr()))
! 250 throw py::type_error("native converter rows must contain lists");
251 result.append(construct(py::reinterpret_borrow<py::list>(values), row_class, converters,
252 column_map, cursor, uuid_indices, lower_map, names));
253 }
254 return result;📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.performance_counter.hpp: 0.7%
mssql_python.pybind.logger_bridge.cpp: 57.9%
mssql_python.pybind.ddbc_bindings.h: 65.1%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 80.1%
mssql_python.pybind.row_mapping_cache.hpp: 81.5%
mssql_python.pybind.connection.connection_pool.cpp: 81.8%
mssql_python.pybind.connection.connection.cpp: 82.5%
mssql_python.logging.py: 86.2%
mssql_python.pybind.row_mapping.hpp: 86.8%🔗 Quick Links
|
Jahnvi Thakkar (jahnvi480)
marked this pull request as ready for review
October 19, 2026 05:17
Jahnvi Thakkar (jahnvi480)
changed the title
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The intertwined native allocation, ownership, and fetch changes require rebuilt-extension, native lifetime, and live SQL Server validation across supported Python/platform combinations, which could not be performed in this review environment.
No open issues 🎉
Review effort: Balanced
Preserve converter and row-to-dictionary profiler workloads and their contract tests; reconcile the combined 25-task registry. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The intertwined native UTF-16 ownership, Row-constructor bypass, and hook invalidation changes require rebuilt-extension, live SQL Server, and cross-platform validation beyond the available source-only review.
No open issues 🎉
Review effort: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Work Item / Issue Reference
Summary
Reduces the overhead of fetching rows with output converters, without changing the public API.
Rowbehavior.fetchall()andfetchmany(1000).Local profiling-OFF benchmarks showed about 33% lower batch-fetch time than pinned main. The
fetchone()improvement target and faster-than-pyodbc target remain unmet.