FEAT: Profiler - #552
Merged
Merged
Conversation
Tasks 1, 2, 3: Update profiler, add new profiling points, expand benchmarks Phase 1: Core Infrastructure (COMPLETE) - Add performance_counter.hpp with thread-safe RAII profiling - Integrate profiling submodule into ddbc_bindings.cpp - Port run_profiler.py and profiling_results.md from old branch - Support for enable/disable/get_stats/reset via Python API Phase 2: Documentation (COMPLETE) - PROFILER_SUMMARY.md: Executive summary and quick reference - PERF_TIMER_LOCATIONS.md: All 43 timer locations with code snippets - ENHANCED_PROFILING_PLAN.md: New profiling points and benchmarks - PROFILER_UPGRADE_STATUS.md: Status tracker and phases Phase 3: Implementation (TODO) - 43 PERF_TIMER calls need to be added (documented in detail) - New profiling points for types, transactions, pool, memory - Comprehensive benchmark suite (8 new categories) Key Features: - Platform detection (Windows/Linux/macOS) - Per-function timing with min/max/avg - Granular timers for construct_rows bottleneck - Designed for Windows vs Linux performance analysis Reference PR: #147 (original profiler branch) Based on analysis showing 2.3x Linux slowdown (now 16% after optimizations)
- perf_timer.py: Python phase-level profiling (perf_phase, perf_start/perf_stop) - performance_counter.hpp: C++ timeline recording, ddbc:: prefix via macro - cursor.py: Phase timers on execute, fetch*, executemany - ddbc_bindings.cpp, connection.cpp, connection_pool.cpp: PERF_TIMER calls - profiler/: CLI package (python -m profiler) with scenarios, timeline mode, custom script support (--script), aggregate + waterfall reporters - my_bench.py: Example custom profiling script
Re-apply py:: and ddbc:: profiling instrumentation on top of main's u16string signature migration, GIL-release changes, and the issue #531 charCtype fetch path. No behavior change to profiling; timers preserved across the refactored execute/fetch/connect paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Drop the root-level planning/status notes (ENHANCED_PROFILING_PLAN, LATEST_UPDATE, PERF_TIMER_LOCATIONS, PROFILER_SUMMARY, PROFILER_UPGRADE_STATUS, profiling_results) and my_bench.py. These were working scratch from building the profiler and shouldn't ship. The profiler tooling itself (profiler/, mssql_python/perf_timer.py, performance_counter.hpp) stays. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The merge resolution accidentally scoped main's 'skip detect_and_convert_parameters when re-executing the same SQL' fast path to only the multi-arg branch. Main applies it to both the single-container (execute(sql, (a, b))) and multi-arg forms. Restore main's structure: compute actual_params in the if/else, then run the same-SQL shortcut once for both, still inside the py::execute::param_unpack timer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/cursor.pyLines 2503-2513 2503
2504 # Check if this should be a DAE (data at execution) parameter based on column size
2505 if sample_value is not None:
2506 if isinstance(sample_value, str) and column_size > MAX_INLINE_CHAR:
! 2507 is_dae = True
2508 elif isinstance(sample_value, (bytes, bytearray)) and column_size > 8000:
! 2509 is_dae = True
2510
2511 # Sanitize precision/scale for numeric types
2512 if sql_type in (
2513 ddbc_sql_const.SQL_DECIMAL.value,Lines 2522-2543 2522 ddbc_sql_const.SQL_VARBINARY.value,
2523 ddbc_sql_const.SQL_LONGVARBINARY.value,
2524 ):
2525 # Find the maximum size needed for any row's binary data
! 2526 max_binary_size = 0
! 2527 for row in seq_of_parameters:
! 2528 value = row[col_index]
! 2529 if value is not None and isinstance(value, (bytes, bytearray)):
! 2530 max_binary_size = max(max_binary_size, len(value))
2531
2532 # For SQL Server VARBINARY(MAX), we need to use large object binding
! 2533 if column_size > 8000 or max_binary_size > 8000:
! 2534 sql_type = ddbc_sql_const.SQL_LONGVARBINARY.value
! 2535 is_dae = True
2536
2537 # Update column_size to actual maximum size if it's larger
2538 # Always ensure at least a minimum size of 1 for empty strings
! 2539 column_size = max(max_binary_size, 1)
2540
2541 paraminfo = param_info()
2542 paraminfo.paramCType = c_type
2543 paraminfo.paramSQLType = sql_typeLines 2547-2555 2547 paraminfo.isDAE = is_dae
2548
2549 # Ensure we never have SQL_C_DEFAULT (0) for C-type
2550 if paraminfo.paramCType == 0:
! 2551 paraminfo.paramCType = ddbc_sql_const.SQL_C_DEFAULT.value
2552
2553 parameters_type.append(paraminfo)
2554 else:
2555 # Use auto-detection for columns without explicit typesLines 2616-2625 2616 max_binary_size = max(max_binary_size, len(value))
2617
2618 # For SQL Server VARBINARY(MAX), we need to use large object binding
2619 if max_binary_size > 8000:
! 2620 paraminfo.paramSQLType = ddbc_sql_const.SQL_LONGVARBINARY.value
! 2621 paraminfo.isDAE = True
2622
2623 # Update column_size to actual maximum size
2624 # Always ensure at least a minimum size of 1 for empty strings
2625 paraminfo.columnSize = max(max_binary_size, 1)Lines 2890-2900 2890 column_map_lower=column_map_lower,
2891 )
2892 for row_data in rows_data
2893 ]
! 2894 except Exception:
2895 # On error, don't increment rownumber - rethrow the error
! 2896 raise
2897
2898 def fetchall(self) -> List[Row]:
2899 """
2900 Fetch all (remaining) rows of a query result.mssql_python/perf_timer.pyLines 236-244 236
237
238 def _record(name: str, elapsed: int, start_ns: int = 0):
239 if getattr(_local, "depth", 0):
! 240 return
241 with _bookkeeping():
242 # Prebind the no-argument release: RLock.__exit__ and method lookup can
243 # allocate before unlocking, which is unsafe around GC finalizers.
244 release = _lock.releaseLines 273-283 273 _lock.acquire()
274 try:
275 # Revalidate after allocations and any callbacks they triggered.
276 if not _enabled or window != _window_start_ns or stats is not _stats:
! 277 return
278 if stats.get(name) is not entry:
! 279 continue
280 stats[name] = updated
281 # A timeline-only restart must not discard valid aggregate samples.
282 if event is not None and _timeline_enabled and timeline is _timeline:
283 timeline.append(event)mssql_python/pybind/connection/connection.cppLines 212-220 212 }
213 }
214
215 void Connection::commit() {
! 216 PERF_TIMER("Connection::commit");
217 if (!_dbcHandle) {
218 ThrowStdException("Connection handle not allocated");
219 }
220 updateLastUsed();Lines 228-236 228 checkError(ret);
229 }
230
231 void Connection::rollback() {
! 232 PERF_TIMER("Connection::rollback");
233 if (!_dbcHandle) {
234 ThrowStdException("Connection handle not allocated");
235 }
236 updateLastUsed();Lines 244-252 244 checkError(ret);
245 }
246
247 void Connection::setAutocommit(bool enable) {
! 248 PERF_TIMER("Connection::setAutocommit");
249 if (!_dbcHandle) {
250 ThrowStdException("Connection handle not allocated");
251 }
252 SQLINTEGER value = enable ? SQL_AUTOCOMMIT_ON : SQL_AUTOCOMMIT_OFF;Lines 283-291 283 return value == SQL_AUTOCOMMIT_ON;
284 }
285
286 SqlHandlePtr Connection::allocStatementHandle() {
! 287 PERF_TIMER("Connection::allocStatementHandle");
288 if (!_dbcHandle) {
289 ThrowStdException("Connection handle not allocated");
290 }
291 updateLastUsed();mssql_python/pybind/ddbc_bindings.cppLines 859-869 859 ThrowStdException(errorString.str());
860 }
861 }
862 assert(SQLBindParameter_ptr && SQLGetStmtAttr_ptr && SQLSetDescField_ptr);
! 863 RETCODE rc;
! 864 {
! 865 PERF_TIMER("BindParameters::SQLBindParameter_call");
866 rc = SQLBindParameter_ptr(
867 hStmt, static_cast<SQLUSMALLINT>(paramIndex + 1), /* 1-based indexing */
868 static_cast<SQLUSMALLINT>(paramInfo.inputOutputType),
869 static_cast<SQLSMALLINT>(paramInfo.paramCType),Lines 867-876 867 hStmt, static_cast<SQLUSMALLINT>(paramIndex + 1), /* 1-based indexing */
868 static_cast<SQLUSMALLINT>(paramInfo.inputOutputType),
869 static_cast<SQLSMALLINT>(paramInfo.paramCType),
870 static_cast<SQLSMALLINT>(paramInfo.paramSQLType), paramInfo.columnSize,
! 871 paramInfo.decimalDigits, dataPtr, bufferLength, strLenOrIndPtr);
! 872 }
873 if (!SQL_SUCCEEDED(rc)) {
874 LOG("BindParameters: SQLBindParameter failed for param[%d] - "
875 "SQLRETURN=%d, C_Type=%d, SQL_Type=%d",
876 paramIndex, rc, paramInfo.paramCType, paramInfo.paramSQLType);Lines 1729-1737 1729 return rc;
1730 }
1731
1732 SQLRETURN SQLGetTypeInfo_Wrapper(SqlHandlePtr StatementHandle, SQLSMALLINT DataType) {
! 1733 PERF_TIMER("SQLGetTypeInfo_Wrapper");
1734 if (!SQLGetTypeInfo_ptr) {
1735 ThrowStdException("SQLGetTypeInfo function not loaded");
1736 }Lines 2803-2811 2803 }
2804 LOG("BindParameterArray: Calling SQLBindParameter - "
2805 "param_index=%d, buffer_length=%lld",
2806 paramIndex, static_cast<long long>(bufferLength));
! 2807 RETCODE rc;
2808 {
2809 PERF_TIMER("BindParameterArray::SQLBindParameter_call");
2810 rc = SQLBindParameter_ptr(hStmt, static_cast<SQLUSMALLINT>(paramIndex + 1),
2811 static_cast<SQLUSMALLINT>(info.inputOutputType),Lines 2999-3007 2999 }
3000
3001 // Wrap SQLNumResultCols
3002 SQLSMALLINT SQLNumResultCols_wrap(SqlHandlePtr statementHandle) {
! 3003 PERF_TIMER("SQLNumResultCols_wrap");
3004 LOG("SQLNumResultCols: Getting number of columns in result set for "
3005 "statement_handle=%p",
3006 (void*)statementHandle->get());
3007 if (!SQLNumResultCols_ptr) {Lines 4167-4175 4167 SQLRETURN ret;
4168 {
4169 // Release the GIL during the blocking ODBC fetch
4170 py::gil_scoped_release release;
! 4171 PERF_TIMER("FetchBatchData::SQLFetchScroll_call");
4172 ret = SQLFetchScroll_ptr(hStmt, SQL_FETCH_NEXT, 0);
4173 }
4174 if (ret == SQL_NO_DATA) {
4175 LOG("FetchBatchData: No data to fetch");Lines 4197-4205 4197 const bool useWideChar = (charCtype == SQL_C_WCHAR);
4198 std::vector<ColumnInfo> columnInfos(numCols);
4199 // Performance: Build function pointer dispatch table (once per batch).
4200 // This eliminates the switch statement from the hot loop - 10,000 rows × 10
! 4201 // cols reduces from 100,000 switch evaluations to just 10 switch evaluations.
4202 std::vector<ColumnProcessor> columnProcessors(numCols);
4203 std::vector<ColumnInfoExt> columnInfosExt(numCols);
4204 // Compute effective char encoding once for the batch (same for all columns)
4205 const std::string effectiveCharEnc = GetEffectiveCharDecoding(charEncoding);Lines 4203-4213 4203 std::vector<ColumnInfoExt> columnInfosExt(numCols);
4204 // Compute effective char encoding once for the batch (same for all columns)
4205 const std::string effectiveCharEnc = GetEffectiveCharDecoding(charEncoding);
4206
! 4207 {
! 4208 PERF_TIMER("FetchBatchData::cache_column_metadata");
! 4209 for (SQLUSMALLINT col = 0; col < numCols; col++) {
4210 const auto& columnMeta = columnNames[col].cast<py::dict>();
4211 columnInfos[col].dataType = columnMeta["DataType"].cast<SQLSMALLINT>();
4212 columnInfos[col].columnSize = columnMeta["ColumnSize"].cast<SQLULEN>();
4213 columnInfos[col].isLob =Lines 4214-4224 4214 std::find(lobColumns.begin(), lobColumns.end(), col + 1) != lobColumns.end();
4215 columnInfos[col].processedColumnSize = columnInfos[col].columnSize;
4216 HandleZeroColumnSizeAtFetch(columnInfos[col].processedColumnSize);
4217
! 4218 SQLSMALLINT dt = columnInfos[col].dataType;
! 4219 bool isCharType = (dt == SQL_CHAR || dt == SQL_VARCHAR || dt == SQL_LONGVARCHAR);
! 4220
4221 if (isCharType && useWideChar) {
4222 // When VARCHAR is bound as SQL_C_WCHAR, buffer size is in SQLWCHAR
4223 // units (same as NVARCHAR). +1 for null terminator.
4224 columnInfos[col].fetchBufferSize = columnInfos[col].processedColumnSize + 1;Lines 4241-4249 4241 }
4242
4243 for (SQLUSMALLINT col = 0; col < numCols; col++) {
4244 // Populate extended column info for processors that need it
! 4245 columnInfosExt[col].dataType = columnInfos[col].dataType;
4246 columnInfosExt[col].columnSize = columnInfos[col].columnSize;
4247 columnInfosExt[col].processedColumnSize = columnInfos[col].processedColumnSize;
4248 columnInfosExt[col].fetchBufferSize = columnInfos[col].fetchBufferSize;
4249 columnInfosExt[col].isLob = columnInfos[col].isLob;Lines 4259-4274 4259 SQLSMALLINT dataType = columnInfos[col].dataType;
4260 switch (dataType) {
4261 case SQL_INTEGER:
4262 columnProcessors[col] = ColumnProcessors::ProcessInteger;
! 4263 break;
! 4264 case SQL_SMALLINT:
! 4265 columnProcessors[col] = ColumnProcessors::ProcessSmallInt;
4266 break;
4267 case SQL_BIGINT:
4268 columnProcessors[col] = ColumnProcessors::ProcessBigInt;
4269 break;
! 4270 case SQL_TINYINT:
4271 columnProcessors[col] = ColumnProcessors::ProcessTinyInt;
4272 break;
4273 case SQL_BIT:
4274 columnProcessors[col] = ColumnProcessors::ProcessBit;Lines 4300-4310 4300 // For complex types (Decimal, DateTime, Guid, etc.), set to
4301 // nullptr and handle via fallback switch in the hot loop
4302 columnProcessors[col] = nullptr;
4303 break;
! 4304 }
! 4305 }
! 4306 } // end cache_column_metadata timer scope
4307
4308 // Performance: Single-phase row creation pattern
4309 // Create each row, fill it completely, then append to results list
4310 // This prevents data corruption (no partially-filled rows) and simplifiesmssql_python/pybind/performance_counter.hppLines 73-195 73 // keep the aggregate window and are handled separately in record().
74 std::atomic<uint64_t> generation_{0};
75
76 public:
! 77 static PerformanceCounter& instance() {
! 78 static PerformanceCounter counter;
! 79 return counter;
! 80 }
81
! 82 void enable() {
! 83 std::lock_guard<std::mutex> lock(mutex_);
! 84 // New window: move the generation so any timer still in flight from a
! 85 // previous window is rejected by record() instead of landing here.
! 86 generation_.fetch_add(1, std::memory_order_relaxed);
! 87 enabled_ = true;
! 88 }
! 89 void disable() {
! 90 std::lock_guard<std::mutex> lock(mutex_);
! 91 enabled_ = false;
! 92 }
! 93 bool is_enabled() const { return enabled_; }
! 94 uint64_t current_generation() const { return generation_.load(std::memory_order_relaxed); }
95
! 96 void enable_timeline() {
! 97 std::lock_guard<std::mutex> lock(mutex_);
! 98 // Clear stale events when (re)setting the epoch so every event in
! 99 // timeline_ shares the current epoch; a second enable_timeline() without
! 100 // an intervening reset() would otherwise mix offsets from two epochs.
! 101 timeline_.clear();
! 102 epoch_ = std::chrono::steady_clock::now();
! 103 timeline_enabled_ = true;
! 104 }
! 105 void disable_timeline() {
! 106 std::lock_guard<std::mutex> lock(mutex_);
! 107 timeline_enabled_ = false;
! 108 }
! 109 bool is_timeline_enabled() const { return timeline_enabled_; }
110
111 void record(const std::string& name, int64_t duration_ns,
! 112 std::chrono::time_point<std::chrono::steady_clock> start, uint64_t generation) {
! 113 if (!enabled_) return;
! 114
! 115 std::lock_guard<std::mutex> lock(mutex_);
! 116 // Check under the lock so disable(), enable() and resets cannot race the write.
! 117 if (!enabled_ || generation != generation_.load(std::memory_order_relaxed))
! 118 return;
! 119 auto& stats = counters_[name];
! 120 stats.total_time_ns += duration_ns;
! 121 stats.call_count++;
! 122 stats.min_time_ns = std::min(stats.min_time_ns, duration_ns);
! 123 stats.max_time_ns = std::max(stats.max_time_ns, duration_ns);
! 124
! 125 // Keep aggregate samples even if their timeline epoch has been replaced.
! 126 if (timeline_enabled_ && start >= epoch_) {
! 127 auto offset = std::chrono::duration_cast<std::chrono::microseconds>(start - epoch_).count();
! 128 timeline_.push_back({name, offset, duration_ns / 1000});
! 129 }
! 130 }
131
! 132 py::dict get_stats() {
! 133 std::unordered_map<std::string, PerfStats> snapshot;
! 134 {
! 135 std::lock_guard<std::mutex> lock(mutex_);
! 136 snapshot = counters_;
! 137 }
! 138 // Python allocation may run a finalizer that re-enters native recording.
! 139 // Only native data is copied under the mutex; build the report after unlocking.
! 140 py::dict result;
! 141
! 142 for (const auto& [name, stats] : snapshot) {
! 143 py::dict d;
! 144 // Convert accumulated nanoseconds to microseconds only here (never
! 145 // per-sample), keeping sub-microsecond precision as fractional us so
! 146 // high-frequency timers do not truncate to zero.
! 147 d["total_us"] = stats.total_time_ns / 1000.0;
! 148 d["calls"] = stats.call_count;
! 149 d["avg_us"] = stats.call_count > 0
! 150 ? static_cast<double>(stats.total_time_ns) / stats.call_count / 1000.0
! 151 : 0.0;
! 152 d["min_us"] = stats.min_time_ns == INT64_MAX ? 0.0 : stats.min_time_ns / 1000.0;
! 153 d["max_us"] = stats.max_time_ns / 1000.0;
! 154 d["platform"] = PROFILING_PLATFORM;
! 155 result[py::str(name)] = d;
! 156 }
! 157
! 158 return result;
! 159 }
160
! 161 void reset() {
! 162 std::lock_guard<std::mutex> lock(mutex_);
! 163 // Counters are cleared, so any timer that started before now belongs to a
! 164 // window that no longer exists; move the generation to reject it.
! 165 generation_.fetch_add(1, std::memory_order_relaxed);
! 166 counters_.clear();
! 167 timeline_.clear();
! 168 }
169
! 170 void reset_stats_only() {
! 171 std::lock_guard<std::mutex> lock(mutex_);
! 172 generation_.fetch_add(1, std::memory_order_relaxed);
! 173 counters_.clear();
! 174 }
175
! 176 py::list get_timeline() {
! 177 std::vector<TimelineEvent> snapshot;
! 178 {
! 179 std::lock_guard<std::mutex> lock(mutex_);
! 180 snapshot = timeline_;
! 181 }
! 182 py::list result;
! 183 for (const auto& ev : snapshot) {
! 184 py::dict d;
! 185 d["name"] = ev.name;
! 186 d["start_us"] = ev.start_us;
! 187 d["duration_us"] = ev.duration_us;
! 188 result.append(d);
! 189 }
! 190 return result;
! 191 }
192 };
193
194 // RAII timer - automatically records on destruction
195 class ScopedTimer {Lines 206-237 206 uint64_t startGeneration_{0};
207
208 public:
209 explicit ScopedTimer(const char* name)
! 210 : name_(name), active_(PerformanceCounter::instance().is_enabled()) {
! 211 if (active_) {
! 212 startGeneration_ = PerformanceCounter::instance().current_generation();
! 213 start_ = std::chrono::steady_clock::now();
! 214 }
! 215 }
216
! 217 ~ScopedTimer() {
! 218 if (active_) {
! 219 // A destructor is implicitly noexcept: if record() threw (its
! 220 // unordered_map insert / vector push_back can throw bad_alloc), the
! 221 // exception would call std::terminate and crash the driver — but only
! 222 // while profiling. Swallow any failure so profiling can never take the
! 223 // process down; a dropped sample is an acceptable cost under OOM.
! 224 try {
! 225 auto end = std::chrono::steady_clock::now();
! 226 auto duration_ns =
! 227 std::chrono::duration_cast<std::chrono::nanoseconds>(end - start_).count();
! 228 PerformanceCounter::instance().record(name_, duration_ns, start_, startGeneration_);
! 229 } catch (...) {
! 230 // ignore: never let a profiling timer abort the process
! 231 }
! 232 }
! 233 }
234 };
235
236 } // namespace mssql_profiling📋 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: 61.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.row.py: 77.6%
mssql_python.pybind.ddbc_bindings.cpp: 77.7%
mssql_python.pybind.connection.connection_pool.cpp: 81.8%
mssql_python.logging.py: 85.5%
mssql_python.helpers.py: 89.3%
mssql_python.pooling.py: 90.1%🔗 Quick Links
|
find_packages() was picking up the top-level profiler/ package, so the internal benchmark CLI (and a generic 'profiler' top-level name) would ship to PyPI. Exclude it. The runtime instrumentation it drives (perf_timer.py, the ddbc_bindings profiling submodule) lives inside mssql_python and still ships. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cover both layers: the python perf_timer (perf_phase/perf_start/perf_stop, enable/disable, stats, timeline, reset vs reset_stats_only) and the C++ ddbc_bindings.profiling submodule (toggle, live query capture, timeline, reset). Autouse fixture resets and disables both layers around every test so profiling state never leaks into the rest of the suite. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Note in performance_counter.hpp that the global mutex is taken only when profiling is enabled, targets single-threaded diagnostics where it is uncontended, and is a deliberate simplification (thread_local is the upgrade path if multithreaded profiling ever matters). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Profiling is now compiled out of normal builds. PERF_TIMER expands to a no-op and the ddbc_bindings.profiling submodule is not registered unless the C++ extension is built with -DENABLE_PROFILING (set ENABLE_PROFILING=1 for build.sh/build.bat). Released wheels therefore ship with zero profiler code. This replaces the old manual comment-toggle in performance_counter.hpp with a real CMake option. Internal/dev builds opt in to get the instrumentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Explains what the two-layer profiler is, the py:: / ddbc:: prefixes, how to make a profiling build (ENABLE_PROFILING), how to run it (CLI --script or the runtime API), how to read the output, and how to add a timer. Drops the stale scenario-specific and internal references. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Re-apply py:: and ddbc:: profiling instrumentation on top of main's C++ execute-pipeline rewrite (SQLExecute_wrap single-pipeline, no ParamInfo across the pybind boundary; param detection moved to C++), the #671 GIL/env-handle and disconnect changes, identity-aware pool keying (pool_key/token_factory), the musl call_once safety in loadDriver, and the new mssql_python_odbc packaging split (combined the setup.py find_packages excludes). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Gaurav Sharma (bewithgaurav)
September 8, 2026 04:55
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new C++ profiling counter has confirmed undefined-behavior hazards (data races and an uninitialized timer start) that should be fixed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces an internal, opt-in two-layer performance profiler for diagnosing latency across the Python DB-API layer and the native ddbc_bindings C++ layer, with a dev-only profiler/ runner and a new test suite covering both layers.
Changes:
- Adds a Python profiling module (
mssql_python/perf_timer.py) and instruments key phases inCursor.execute()/executemany()/fetch*(). - Adds a C++ RAII timing macro (
PERF_TIMER) + global counter/timeline, and exposes it asmssql_python.ddbc_bindings.profilingin profiling-enabled builds. - Adds a dev-only
profiler/CLI + scenarios/reporting, and excludes it from the shipped wheel viasetup.py.
File summaries
| File | Description |
|---|---|
| tests/test_025_profiler.py | Adds tests for Python and (optional) C++ profiling enable/disable, stats, and timeline. |
| setup.py | Excludes profiler/ from distribution packages while keeping runtime instrumentation shipped. |
| profiler/init.py | Exposes Profiler API for the internal runner package. |
| profiler/main.py | Implements python -m profiler CLI entrypoint and argument parsing. |
| profiler/core.py | Orchestrates scenarios and collects/prints combined C++ + Python profiling output. |
| profiler/reporter.py | Merges and formats stats/timeline from both layers for reporting. |
| profiler/scenarios.py | Defines benchmark scenarios and test-data setup helpers. |
| profiler/README.md | Documents how to build/run the profiler and interpret its output. |
| mssql_python/pybind/performance_counter.hpp | Adds C++ profiling counter/timeline and PERF_TIMER macro implementation. |
| mssql_python/pybind/ddbc_bindings.cpp | Adds C++ timers in key ODBC paths and exposes ddbc_bindings.profiling submodule in profiling builds. |
| mssql_python/pybind/connection/connection.cpp | Adds C++ timers around connection lifecycle and transactional operations. |
| mssql_python/pybind/connection/connection_pool.cpp | Adds C++ timers around pool acquire/release/close and pool-manager acquire. |
| mssql_python/pybind/CMakeLists.txt | Adds ENABLE_PROFILING CMake option and defines ENABLE_PROFILING compile definition when enabled. |
| mssql_python/pybind/build.sh | Passes -DENABLE_PROFILING=ON to CMake when ENABLE_PROFILING env var is set. |
| mssql_python/pybind/build.bat | Passes -DENABLE_PROFILING=ON to CMake when ENABLE_PROFILING env var is set. |
| mssql_python/perf_timer.py | Introduces Python-layer timing (stats + optional timeline) aligned with C++ schema. |
| mssql_python/cursor.py | Instruments execute/fetch paths with Python timers and adds a measured param-type-detection section in executemany. |
Review details
Suppressed comments (3)
mssql_python/pybind/performance_counter.hpp:59
- enabled_ and timeline_enabled_ are read/written from multiple threads (timer scopes can run concurrently with enable/disable from Python) but are plain bools accessed without synchronization; this is a C++ data race (undefined behavior). Use std::atomic (or guard all reads/writes with the mutex) to make enable checks thread-safe.
std::mutex mutex_;
bool enabled_ = false;
bool timeline_enabled_ = false;
std::chrono::time_point<std::chrono::high_resolution_clock> epoch_;
mssql_python/pybind/performance_counter.hpp:75
- enable_timeline() writes epoch_ without holding mutex_, but record() reads epoch_ while holding mutex_. Because the write is not synchronized with the read, this is also a data race; protect epoch_ updates with the same mutex (and ideally set epoch_ before flipping timeline_enabled_ on).
void enable_timeline() {
timeline_enabled_ = true;
epoch_ = std::chrono::high_resolution_clock::now();
}
void disable_timeline() { timeline_enabled_ = false; }
mssql_python/pybind/performance_counter.hpp:149
- ScopedTimer only initializes start_ when profiling is enabled at construction time, but the destructor checks is_enabled() again. If profiling is toggled on between construction and destruction, start_ is uninitialized and the duration calculation becomes undefined behavior. Capture an
active_flag at construction and use it in the destructor.
explicit ScopedTimer(const char* name) : name_(name) {
if (PerformanceCounter::instance().is_enabled()) {
start_ = std::chrono::high_resolution_clock::now();
}
}
- Files reviewed: 17/17 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Two thread-safety fixes in performance_counter.hpp, both only affecting profiling builds. (1) enabled_/timeline_enabled_ are now std::atomic and enable_timeline() writes epoch_ under the mutex, so enable/disable/enable_timeline are safe to call from a thread other than the one running timers (timers run with the GIL released). (2) ScopedTimer captures the enabled state once at construction into active_ and uses it in the destructor, instead of re-checking is_enabled(); this removes the window where profiling flipping on between ctor and dtor could read an uninitialized start_ or record a half-open interval. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Gaurav Sharma (bewithgaurav)
September 8, 2026 05:26
View session
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It contains a few correctness issues (missing standard includes in the new C++ header and missing cleanup on exceptions in Profiler.run_script()) that can break builds or leak resources.
Review details
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
mssql_python/pybind/performance_counter.hpp:16
PerformanceCounterusesint64_t,INT64_MAX, andstd::min/std::maxbut the header doesn’t include the standard headers that define them (<cstdint>/<stdint.h>and<algorithm>). This can cause build failures depending on transitive includes.
profiler/core.py:199Profiler.run_script()leaks the cursor (and leaves profiling enabled) if the user script fails to compile or raises duringexec(), because cleanup only happens on the success path. Wrap execution intry/finallysocursor.close()andself._ctx.collect()always run.
profiler/init.py:17- The module docstring shows
p.report(), butProfilerdoesn’t define areportmethod (and there’s nodef report(...)anywhere underprofiler/). This example will fail if copied.
mssql_python/perf_timer.py:108
perf_start()returns 0 when profiling is disabled, butperf_stop()will record a bogus duration if profiling gets enabled between start and stop (it will computenow - 0). Guarding against a zero start avoids corrupting stats in that edge case.
def perf_start() -> int:
if not _enabled:
return 0
return time.perf_counter_ns()
def perf_stop(name: str, t0: int):
if not _enabled:
return
_record(name, time.perf_counter_ns() - t0, t0)
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
…ate each other The profiler CLI recorded across window boundaries: collect() snapshotted and reset the counters but left profiling enabled, so a scenario's teardown (commit/close), the next scenario's setup, and a fetch scenario's pre-enable cursor.execute all kept recording and bled into the next scenario's numbers. Profiler.close() also left profiling on for programmatic callers. Make the window airtight in _ProfilingContext: enable() resets both layers before turning on (clean start, discards anything leaked between windows), collect() disables both layers after snapshotting (nothing outside a window is counted), and close() disables. Removed the now-redundant setup drain in _ensure_test_data. Added tests asserting collect() disables profiling and that work between two windows never appears in the next window's stats. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Gaurav Sharma (bewithgaurav)
September 8, 2026 05:32
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness/robustness issues in the new profiling implementation (missing required C++ includes and cleanup gaps in the profiler runner/scenarios) that can cause build failures or leaked profiling state.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
mssql_python/pybind/performance_counter.hpp:15
- performance_counter.hpp uses std::min/std::max, int64_t, and INT64_MAX but does not include the standard headers that define them. This can fail to compile depending on transitive includes. Add the missing headers explicitly (e.g., and ).
#include <chrono>
#include <string>
#include <vector>
#include <unordered_map>
#include <mutex>
#include <atomic>
#include <pybind11/pybind11.h>
#include <pybind11/stl.h>
- Files reviewed: 17/17 changed files
- Comments generated: 2
- Review effort level: Lite
…tion-safe profiler runner performance_counter.hpp: add the standard headers it actually uses (<algorithm>, <cstdint>, <limits>) instead of relying on transitive pybind/STL includes, which is not guaranteed across toolchains. perf_timer.perf_stop: ignore a falsy (t0==0) start so a perf_start() that ran while disabled cannot record a bogus now-minus-zero duration. profiler runner: guarantee profiling is disabled even when a scenario or a --script user script raises — one try/finally in run() covers every scenario (and future ones) rather than a guard in each scenario body, and run_script() closes its cursor and ends the window in a finally. Added a test asserting a raising script leaves profiling off. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Gaurav Sharma (bewithgaurav)
September 9, 2026 14:22
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Several profiler scenarios enable profiling but don’t reliably disable it on exception paths, risking process-wide profiling-state leaks in dev tooling/tests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
profiler/scenarios.py:197
- fetchall() enables profiling but only closes the cursor in finally. If fetchall() raises before ctx.collect() runs, profiling can remain enabled and leak into subsequent work.
def fetchall(conn, table, ctx) -> dict:
cursor = conn.cursor()
try:
cursor.execute(f"SELECT * FROM {table}")
ctx.enable()
t0 = time.perf_counter()
rows = cursor.fetchall()
wall_ms = (time.perf_counter() - t0) * 1000
cpp, py = ctx.collect()
finally:
cursor.close()
profiler/scenarios.py:223
- fetchone() enables profiling but doesn't disable it in finally. Any exception in the fetch loop before ctx.collect() will leave profiling enabled (global state leak).
def fetchone(conn, table, ctx, row_count: int = FETCHONE_ROWS) -> dict:
cursor = conn.cursor()
try:
cursor.execute(f"SELECT TOP {row_count} * FROM {table}")
ctx.enable()
t0 = time.perf_counter()
count = 0
while True:
row = cursor.fetchone()
if row is None:
break
count += 1
wall_ms = (time.perf_counter() - t0) * 1000
cpp, py = ctx.collect()
finally:
cursor.close()
return {
profiler/scenarios.py:248
- fetchmany() enables profiling but doesn't disable it in finally. If cursor.fetchmany() or subsequent code raises before ctx.collect(), profiling can remain enabled and affect later measurements.
def fetchmany(conn, table, ctx, batch_size: int = FETCHMANY_SIZE) -> dict:
cursor = conn.cursor()
try:
cursor.execute(f"SELECT * FROM {table}")
ctx.enable()
t0 = time.perf_counter()
total = 0
while True:
batch = cursor.fetchmany(batch_size)
if not batch:
break
total += len(batch)
wall_ms = (time.perf_counter() - t0) * 1000
cpp, py = ctx.collect()
finally:
cursor.close()
return {
profiler/scenarios.py:314
- fetch_arrow() enables profiling but only closes the cursor in finally. If arrow_batch() raises something other than ImportError (e.g., a driver error) before ctx.collect() runs, profiling can remain enabled and leak into later measurements.
finally:
cursor.close()
- Files reviewed: 19/19 changed files
- Comments generated: 2
- Review effort level: Lite
Copilot started reviewing on behalf of
Gaurav Sharma (bewithgaurav)
September 10, 2026 04:13
View session
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The profiling paths are correctly gated (native compile-time, Python runtime no-op), and the PR includes targeted tests that validate enable/disable, window isolation, and cleanup behavior.
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
Add a Python-only subprocess case that triggers cyclic GC during statistics collection and asserts completed finalizer cleanup and an unchanged snapshot. Bound the child to ten seconds so a regression fails rather than hanging CI. The case requires neither SQL Server nor profiling-enabled native instrumentation. It detects the previous non-reentrant-lock deadlock and an incomplete RLock-only change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Gaurav Sharma (bewithgaurav)
September 10, 2026 04:46
View session
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The new python -m profiler CLI does not handle ImportError from missing/failed native extension imports, causing a traceback instead of the intended one-line error output.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
profiler/main.py:68
- The CLI only converts ValueError/RuntimeError/FileNotFoundError into a one-line message, but a missing/failed native extension import (ImportError from mssql_python.ddbc_bindings) will still produce a traceback. Since the CLI aims to surface setup problems cleanly, include ImportError in this handler (or catch it in Profiler construction) so a missing build shows as a single-line error.
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
gargsaumya
reviewed
Sep 10, 2026
gargsaumya
left a comment
gargsaumya
left a comment
Contributor
There was a problem hiding this comment.
The latest head addresses the earlier window-boundary, finalizer, cleanup, and test-packaging findings. I found one remaining clock-resolution edge case in the Python window identity logic.
gargsaumya
approved these changes
Sep 10, 2026
gargsaumya
left a comment
gargsaumya
left a comment
Contributor
There was a problem hiding this comment.
I’ve added a suggestion, but it is non-blocking, so you can consider addressing it as part of future scope.
Approving this PR.
gargsaumya
approved these changes
Sep 10, 2026
gargsaumya
left a comment
gargsaumya
left a comment
Contributor
There was a problem hiding this comment.
I’ve added a suggestion, but it is non-blocking, so you can consider addressing it as part of future scope.
Approving this PR.
Sumit Sarabhai (sumitmsft)
approved these changes
Sep 10, 2026
Sumit Sarabhai (sumitmsft)
left a comment
Sumit Sarabhai (sumitmsft)
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the PR for correctness, security, reliability, performance, test coverage, repository conventions, and applicable architecture and design specifications. No actionable issues were identified. The implementation is consistent with repository standards and the applicable approved design requirements.
Recommendation: Approve
The previously reported runtime issues and the requested bounded GC/finalizer regression test are addressed. An independent rubber-duck pass found no additional actionable issues. No separate approved profiler design was available; the documented internal diagnostic scope was used for this review.
At submission, 27 checks have succeeded; the macOS SQL2022 job and parent validation status remain pending. This approval does not bypass CI or authorize merging.
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
September 10, 2026 06:32
View session
Jahnvi Thakkar (jahnvi480)
approved these changes
Sep 10, 2026
Jahnvi Thakkar (jahnvi480)
left a comment
Jahnvi Thakkar (jahnvi480)
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the PR! LGTM!
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new shipped mssql_python.perf_timer module is not reflected in the package’s type stub surface, which can break typed imports (e.g., from mssql_python import perf_timer).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 1
- Review effort level: Lite
Copilot started reviewing on behalf of
Jahnvi Thakkar (jahnvi480)
September 10, 2026 08:08
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are test gating and script-compilation issues that can cause incorrect test execution and failures in some environments.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
profiler/core.py:212
compile(path.read_bytes(), ...)decodes the source as UTF-8 and does not honor PEP 263 encoding cookies. That can make--scriptfail for valid Python files that declare a different encoding. Usetokenize.open()(or similar) to read the script with its declared encoding before compiling.
path = Path(script_path).resolve()
if not path.is_file():
raise FileNotFoundError(f"Script not found: {script_path}")
code = compile(path.read_bytes(), str(path), "exec", dont_inherit=True)
- Files reviewed: 19/19 changed files
- Comments generated: 1
- Review effort level: Lite
Copilot started reviewing on behalf of
Gaurav Sharma (bewithgaurav)
September 10, 2026 09:37
View session
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces substantial cross-language instrumentation and concurrency-sensitive logic in the native extension that warrants final human review despite only minor issues found.
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 2
- Review effort level: Lite
Gaurav Sharma (bewithgaurav)
added a commit
that referenced
this pull request
Sep 10, 2026
Merge main at 9122376, including profiler PR #552 and the other upstream updates. Preserve the cached-binding implementation and header extraction while resolving overlapping instrumentation in ddbc_bindings.cpp. Keep the upstream timers around the actual ODBC bind calls and retain the cache invalidation and handle guards. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
Adds a performance profiler for diagnosing where time goes inside a database call, split across the two layers the driver is built from: the Python layer (
cursor.py) and the native C++ layer (ddbc_bindings). A normal Python profiler sees the entire native layer as one opaque block; this instruments both layers with named timers so a slow call can be attributed to, for example, native parameter binding rather than Python.This is an internal development tool. The C++ instrumentation is compiled out of released wheels (see below), so the native driver carries no profiler code. The Python-side phase markers are lightweight and remain in the shipped
cursor.py; when profiling is disabled they are a no-op branch whose measured end-to-end cost is within run-to-run noise versus a no-profiler build.How it is built in
ENABLE_PROFILINGCMake option, off by default. Normal builds expand everyPERF_TIMERto a no-op and do not register the nativeddbc_bindings.profilingsubmodule, so there is no native profiler code, no unwind tables, and no runtime branch in the released binary. Internal/dev builds opt in withENABLE_PROFILING=1 bash build.sh(orset ENABLE_PROFILING=1+build.baton Windows).perf_timer.pyand theperf_phase(...)markers incursor.py) cannot be compiled out the same way, so it ships. When profiling is disabled,perf_phasereturns a shared no-op context manager (measured at ~110 ns per marker in isolation). Some markers do sit on per-row paths (e.g. three in thefetchoneloop), but that per-marker cost is orders of magnitude below per-row DB latency, so measured end-to-end overhead on execute/fetch workloads stays within run-to-run noise versus a no-profiler build.Components
performance_counter.hppprovides aPERF_TIMER("name")RAII macro and instrumentsddbc_bindings.cppand the connection sources, exposed to Python as theddbc_bindings.profilingsubmodule.perf_timer.pyprovidesperf_phase("name")context managers around the execute/fetch phases incursor.py. Theddbc::andpy::prefixes identify which layer a timer belongs to.profiler/CLI (python -m profiler) with built-in scenarios plus aggregate and timeline reports, and a--scriptflag for profiling an arbitrary workload.profiler/is dev-only and excluded from the shipped wheel.Controlled use and reentrancy
Use controlled diagnostic workloads with one controller owning the process-wide measurement windows: enable, run the workload, wait for worker threads, then collect. Recording supports worker threads; independent concurrent profiling sessions and always-on production profiling are not supported.
Reports use detached snapshots. Native counters release their mutex before allocating Python report objects. Python bookkeeping uses immutable records and omits only same-thread recursively triggered samples; application cleanup, ordinary nested timers and other threads still run.
Enabled instrumentation adds bookkeeping overhead. Compare runs using the same profiling configuration; the disabled Python fast path and native compile-out remain unchanged.
Tests
Existing runtime-instrumentation tests are retained. Native cases require a profiling-enabled build; tests importing the dev-only
profilerpackage skip when it is absent from an installed wheel. Broader profiler testing and profiling-enabled CI remain follow-up work.Follow-up work (intentionally out of scope for this PR)
This PR ships the internal profiler tooling only. The following are deliberately deferred to follow-up PRs and are not oversights:
ENABLE_PROFILING, so the profiling build and its native-layer tests are not exercised in CI. A follow-up will add anENABLE_PROFILINGbuild/test leg (Linux + Windows). This is tied to the planned work to drive CI benchmarks from profiler data, where these builds and the scenario set become the fixed workloads a regression gate runs.