Repository navigation
PERF: Speed up output-converter batching and add CI coverage - #852
Open
Jahnvi Thakkar (jahnvi480) wants to merge 1 commit into
Open
Jahnvi Thakkar (jahnvi480) wants to merge 1 commit into
Jahnvi Thakkar (jahnvi480) wants to merge 1 commit 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 -8.158 ms; py::fetchall::cpp_call -0.112 ms; ddbc::FetchAll_wrap -0.053 ms. Unix / SQL Server 202510,000 output-converted rows / fetchall(): py::fetchall::row_wrap -7.891 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 3621-3629 3621 py::object text;
3622 if (OutputConverters::enabled(converters, i - 1))
3623 text = OutputConverters::text(textData, textSize);
3624 row.append(text ? text
! 3625 : FetchText::from_utf16_native(textData, textSize));
3626 LOG("SQLGetData: CHAR column %d fetched as WCHAR, "
3627 "length=%lu",
3628 i, (unsigned long)numCharsInData);
3629 } else {Lines 3801-3814 3801 // omits the terminator.
3802 const char* textData =
3803 reinterpret_cast<const char*>(dataBuffer.data());
3804 Py_ssize_t textSize =
! 3805 static_cast<Py_ssize_t>(numCharsInData * sizeof(SQLWCHAR));
! 3806 py::object text;
! 3807 if (dataType != SQL_SS_VARIANT &&
! 3808 OutputConverters::enabled(converters, i - 1))
! 3809 text = OutputConverters::text(textData, textSize);
! 3810 row.append(text ? text
3811 : FetchText::from_utf16_native(textData, textSize));
3812 LOG("SQLGetData: Appended NVARCHAR string "
3813 "length=%lu for column %d",
3814 (unsigned long)numCharsInData, i);Lines 4460-4468 4460 SQLRETURN FetchBatchData(SQLHSTMT hStmt, ColumnBuffers& buffers, const Metadata& columnNames,
4461 py::list& rows, SQLUSMALLINT numCols, SQLULEN& numRowsFetched,
4462 const std::vector<SQLUSMALLINT>& lobColumns,
4463 const std::string& charEncoding = "utf-16le", int charCtype = SQL_C_WCHAR,
! 4464 py::handle messages = {}, py::handle converters = {}) {
4465 PERF_TIMER("FetchBatchData");
4466 LOG("FetchBatchData: Fetching data in batches");
4467 SQLRETURN ret;
4468 {Lines 6262-6270 6262 fetchStateGuard.configure(&numRowsFetched, fetchSize);
6263
6264 while (ret != SQL_NO_DATA) {
6265 ret = FetchBatchData(hStmt, buffers, columnNames, rows, numCols, numRowsFetched, lobColumns,
! 6266 charEncoding, charCtype, messages, converters);
6267 CheckFetchError(StatementHandle, ret);
6268 if (!SQL_SUCCEEDED(ret) && ret != SQL_NO_DATA) {
6269 LOG("FetchAll_wrap: Error when fetching data - SQLRETURN=%d", ret);
6270 return ret;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.connection.connection_pool.cpp: 82.3%
mssql_python.pybind.connection.connection.cpp: 83.1%
mssql_python.logging.py: 86.2%
mssql_python.pooling.py: 90.1%
mssql_python.pybind.fetch_temporal.hpp: 92.1%🔗 Quick Links
|
Jahnvi Thakkar (jahnvi480)
marked this pull request as ready for review
October 19, 2026 05:17
Jahnvi Thakkar (jahnvi480)
changed the title
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.