Skip to content

Commit 8fb3c3b

Browse files
jahnvi480Copilotbewithgaurav
authored
PERF: Optimize checked temporal fetch construction (#795)
### Work Item / Issue Reference > GitHub Issue: #554 > > ADO Task: [AB#48255](https://sqlclientdrivers.visualstudio.com/c6d89619-62de-46a0-8b46-70b92a84d85e/_workitems/edit/48255) ------------------------------------------------------------------- ### Summary **Reduce Python object-construction overhead when fetching DATE, TIME, and TIMESTAMP values.** Previously, native temporal fields were converted into Python arguments and passed through a generic call to an already-cached constructor. Repeated imports were not the bottleneck. The new helper uses checked CPython construction APIs when the cached constructor is the exact standard type; substituted constructors retain the original call. Six row-wise/batch conversion sites change. Field validation and final object allocation remain. NULLs, precision, timezone/fold, ownership, and exception behavior are preserved; DATETIMEOFFSET, UUID, Decimal, and text are untouched by this PR. ```mermaid flowchart LR A["Native temporal fields after NULL checks"] --> B["Before: Python arguments and generic cached-constructor call"] A --> C{"After: exact standard type?"} C -->|"Yes"| D["Direct checked CPython construction"] C -->|"No: original fallback"| B B --> E["Validated Python object in result row"] D --> E ``` #### Fresh measurements Temporal cases contain NULLs every seventh row. Pure cases have eight temporal columns; the row-wise case adds one harmless MAX column. Mixed has DATE/TIME/DATETIME2/DATETIMEOFFSET; narrow is an unchanged int/text/float control. | Workload / path | API / requested batch | Rows × columns | Before → after fetch time | Reduction | | --- | --- | ---: | ---: | ---: | | DATE / bounded | `fetchmany(1000)` | 4,000 × 8 | 6.060 → 5.050 ms | **16.67%** | | TIME(7) / bounded | `fetchmany(1000)` | 4,000 × 8 | 6.815 → 5.479 ms | **19.60%** | | DATETIME2(7) / bounded | `fetchmany(1000)` | 4,000 × 8 | 8.077 → 5.359 ms | **33.65%** | | DATETIME2(7) / MAX-forced row-wise | `fetchall()` / all remaining | 4,000 × 9 | 13.906 → 9.562 ms | **31.24%** | | Mixed temporal | Repeated `fetchone()` / 1 | 4,000 × 4 | 234.946 → 231.518 ms | 1.46%; inconclusive | | Unchanged narrow | `fetchmany(1000)` | 10,000 × 3 | 6.493 → 6.634 ms | **-2.16%** | **Method/build:** September 21 Docker Linux x64; Python 3.13.15, pybind11 3.0.1, GCC 12.2 Release `-O3 -DNDEBUG`, profiling OFF, SQL Server 16.0.4225.2, ODBC 18.6.2.1. Main `c963ee1e` versus PR `5aaa6aae`: 10 counterbalanced pairs × 5 samples × 14 cases, totaling 1,400 validated drains. Reductions are ratios of medians, excluding execute/validation; no outlier removal or retries. Fallback checks observed three callbacks per temporal type with 3/4/7 positional arguments—not optimized-path or allocation counts. **Limits:** identical-build A/A calibration was noisy (speed-ratio interval 0.882×–1.345×). Mixed `fetchone` and bounded DATE `fetchall` remain inconclusive; unchanged narrow many/all had negative point estimates with intervals spanning zero. These are scoped bulk-temporal gains, not universal speedups or a no-regression guarantee. Both builds passed six fresh-process compatibility modes covering boundaries, NULLs, types, substitutions, exceptions, recovery, and both fetch paths. No full-suite or all-OS success is claimed. Complete samples, intervals, provenance, and historical limitations remain in retained local evidence. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Gaurav Sharma <sharmag@microsoft.com>
1 parent a5faa32 commit 8fb3c3b

8 files changed

Lines changed: 574 additions & 124 deletions

File tree

‎.github/workflows/pr-profiler-report.yml‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,10 @@
11
name: PR Performance Report
22

3-
# Privileged reporting only. No PR checkout, builds, or artifact execution here.
3+
# Same-repo PRs may exercise their formatter directly. Forks use trusted base code.
44
on:
5+
pull_request:
6+
branches: [main]
7+
types: [opened, synchronize, reopened, ready_for_review]
58
pull_request_target:
69
branches: [main]
710
types: [opened, synchronize, reopened, ready_for_review]
@@ -16,12 +19,17 @@ concurrency:
1619

1720
jobs:
1821
report:
22+
if: >-
23+
(github.event_name == 'pull_request' &&
24+
github.event.pull_request.head.repo.full_name == github.repository) ||
25+
(github.event_name == 'pull_request_target' &&
26+
github.event.pull_request.head.repo.full_name != github.repository)
1927
runs-on: ubuntu-latest
2028
timeout-minutes: 230
2129
steps:
2230
- uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0
2331
with:
24-
ref: ${{ github.event.pull_request.base.sha }}
32+
ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.event.pull_request.base.sha }}
2533
persist-credentials: false
2634
- uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0
2735
with:

‎CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
5757
does not change the default provider or ship any Rust driver binaries.
5858

5959
### Changed
60+
- DATE, TIME, and TIMESTAMP fetch conversion uses checked CPython constructors
61+
for the standard datetime types, while preserving cached substitute constructors,
62+
their positional arguments and exceptions, and fractional-second truncation.
63+
DATETIMEOFFSET, UUID, and Decimal conversion are unchanged.
6064
- `mssql-python` now depends on `mssql-python-rs==0.1.0` for `mssql_py_core`
6165
instead of embedding files owned by that separately published distribution.
6266
- **GH-769 deprecation policy:** The misplaced `GetInfoConstants` members

‎eng/profiler_benchmarks/README.md‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,12 +30,12 @@ Partial results never produce a verdict.
3030
Two environments publish raw samples: Unix on Ubuntu with SQL Server 2022/2025.
3131
Routine Windows and macOS profiling is intentionally excluded because neutral PRs
3232
showed platform variance above the regression threshold, while both platforms
33-
remain covered by functional CI. The privileged publisher runs
34-
trusted base code, selects the exact PR-head ADO build, and validates bounded
35-
artifacts as data. It publishes as soon as both profiler artifacts exist,
36-
without waiting for unrelated matrix legs. After build completion, missing
37-
artifacts receive a two-minute propagation grace before a partial result is
38-
published. A failed aggregate build can still publish usable profiler artifacts.
33+
remain covered by functional CI. Same-repository PRs run their formatter directly;
34+
fork PRs retain the trusted-base publisher. Both select the exact PR-head ADO build
35+
and validate bounded artifacts as data. Publication begins as soon as both profiler
36+
artifacts exist, without waiting for unrelated matrix legs. After build completion,
37+
missing artifacts receive a two-minute propagation grace before a partial result
38+
is published. A failed aggregate build can still publish usable profiler artifacts.
3939
Exact-head reports may finalize after merge; stale heads are ignored. Missing,
4040
malformed, canceled, incomplete, or invalid data remains unavailable.
4141

‎eng/profiler_benchmarks/report.py‎

Lines changed: 95 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
MAX_BYTES = 8 * 1024 * 1024
4343
MAX_COMMENT_CHARS = 60000
4444
MAX_DIAGNOSTIC_ROWS = 20
45+
MAX_FINGERPRINT_TASKS = 4
4546
MARKER = "<!-- mssql-python-profiler-ci -->"
4647
THRESHOLD = 0.20
4748
MIN_DELTA_MS = 1.0
@@ -376,75 +377,100 @@ def render(reports, head, build_id, issues=()):
376377
]
377378
missing = len(LEGS) - len(completed)
378379

379-
if len(regressions) == 1:
380-
leg, row = regressions[0]
381-
opening = (
382-
f"This PR consistently slows {TASK_NAMES[row['name']].lower()} on "
383-
f"{environment_name(leg)} by {row['change_pct']:.1f}%."
384-
)
385-
elif regressions:
380+
highlighted = [
381+
(leg, row) for leg, (_, rows) in completed.items() for row in rows if row["status"] != "ok"
382+
]
383+
if regressions:
386384
tasks = len({row["name"] for _, row in regressions})
387385
environments = len({leg for leg, _ in regressions})
388386
opening = (
389-
f"This PR has {len(regressions)} consistent slowdown signals across "
390-
f"{tasks} database tasks and {environments} environments."
387+
f"{tasks} database task{'s' if tasks != 1 else ''} consistently slowed down across "
388+
f"{environments} measured environment{'s' if environments != 1 else ''}."
391389
)
390+
verdict = "⚠️ Performance regression detected"
392391
elif noisy:
393-
if len(noisy) == 1:
394-
leg, row = noisy[0]
395-
opening = (
396-
f"{TASK_NAMES[row['name']]} was slower on {environment_name(leg)}, "
397-
"but the repeated comparisons were inconsistent."
398-
)
399-
else:
400-
tasks = len({row["name"] for _, row in noisy})
401-
environments = len({leg for leg, _ in noisy})
402-
opening = (
403-
f"No consistent slowdowns detected. {len(noisy)} inconsistent comparisons "
404-
f"need review across {tasks} database tasks and {environments} environments."
405-
)
406-
elif len(improvements) == 1:
407-
leg, row = improvements[0]
392+
tasks = len({row["name"] for _, row in noisy})
393+
environments = len({leg for leg, _ in noisy})
408394
opening = (
409-
f"This PR consistently makes {TASK_NAMES[row['name']].lower()} faster on "
410-
f"{environment_name(leg)} by {abs(row['change_pct']):.1f}%."
395+
f"{tasks} database task{'s' if tasks != 1 else ''} produced inconsistent slowdown "
396+
f"signals across {environments} measured environment"
397+
f"{'s' if environments != 1 else ''}."
411398
)
399+
verdict = "🔍 Performance needs review"
412400
elif improvements:
413401
tasks = len({row["name"] for _, row in improvements})
414402
environments = len({leg for leg, _ in improvements})
415403
opening = (
416-
f"This PR has {len(improvements)} consistent improvement signals across "
417-
f"{tasks} database tasks and {environments} environments."
404+
f"{tasks} database task{'s' if tasks != 1 else ''} consistently improved across "
405+
f"{environments} measured environment{'s' if environments != 1 else ''}. "
406+
"No consistent slowdowns were detected."
418407
)
408+
verdict = "✅ Performance improved"
419409
elif not completed:
420410
opening = (
421411
"Performance could not be assessed because no environment produced a complete result."
422412
)
413+
verdict = "⛔ Performance unavailable"
423414
elif not missing:
424415
opening = f"No consistent slowdowns detected across all {len(LEGS)} environments."
416+
verdict = "✅ No regression detected"
425417
else:
426418
completed_label = "environment" if len(completed) == 1 else "environments"
427419
missing_label = "environment" if missing == 1 else "environments"
428420
opening = (
429421
f"No consistent slowdowns in the {len(completed)} completed {completed_label}. "
430422
f"No result is available for {missing} {missing_label}."
431423
)
424+
verdict = "✅ No regression detected"
432425

433-
lines = [MARKER, "## PR Performance Report", "", f"**{opening}**", ""]
434-
highlighted = regressions or noisy or improvements
435-
if highlighted:
436-
if not regressions and noisy:
437-
lines += ["Inconsistent slowdowns to review:", ""]
426+
improvement_tasks = len({row["name"] for _, row in improvements})
427+
regression_tasks = len({row["name"] for _, row in regressions})
428+
lines = [
429+
MARKER,
430+
"## PR Performance Report",
431+
"",
432+
f"### {verdict}",
433+
"",
434+
f"**{opening}**",
435+
"",
436+
f"<kbd>{improvement_tasks} IMPROVEMENT"
437+
f"{'S' if improvement_tasks != 1 else ''}</kbd> "
438+
f"<kbd>{regression_tasks} SLOWDOWN"
439+
f"{'S' if regression_tasks != 1 else ''}</kbd> "
440+
f"<kbd>{len(completed)}/{len(LEGS)} ENVIRONMENTS</kbd>",
441+
"",
442+
]
443+
if noisy:
444+
noisy_tasks = len({row["name"] for _, row in noisy})
438445
lines += [
439-
"| Environment | Affected task | Before | After | Change |",
440-
"|---|---|---:|---:|---:|",
446+
f"<kbd>{noisy_tasks} INCONSISTENT SLOWDOWN" f"{'S' if noisy_tasks != 1 else ''}</kbd>",
447+
"",
441448
]
442-
for leg, row in highlighted:
443-
lines.append(
444-
f"| {environment_name(leg)} | {TASK_NAMES[row['name']]} | "
445-
f"{row['base_ms']:.3f} ms | {row['candidate_ms']:.3f} ms | "
446-
f"{row['change_pct']:+.1f}% |"
447-
)
449+
affected_tasks = [name for name in CASES if any(row["name"] == name for _, row in highlighted)]
450+
if highlighted and len(affected_tasks) <= MAX_FINGERPRINT_TASKS:
451+
affected_legs = [leg for leg in LEGS if any(item_leg == leg for item_leg, _ in highlighted)]
452+
by_signal = {(leg, row["name"]): row for leg, row in highlighted}
453+
lines += [
454+
"### Signal fingerprint",
455+
"",
456+
"| Database task | "
457+
+ " | ".join(environment_name(leg) for leg in affected_legs)
458+
+ " |",
459+
"|---|" + "|".join("---:" for _ in affected_legs) + "|",
460+
]
461+
for name in affected_tasks:
462+
cells = []
463+
for leg in affected_legs:
464+
row = by_signal.get((leg, name))
465+
if row is None:
466+
cells.append("No signal")
467+
elif row["status"] == "improvement":
468+
cells.append(f"**{abs(row['change_pct']):.1f}% faster**")
469+
elif row["status"] == "regression":
470+
cells.append(f"**{abs(row['change_pct']):.1f}% slower**")
471+
else:
472+
cells.append(f"**{abs(row['change_pct']):.1f}% inconsistent**")
473+
lines.append(f"| {escape(TASK_NAMES[name])} | " + " | ".join(cells) + " |")
448474
lines.append("")
449475
if regressions:
450476
lines.append(
@@ -461,24 +487,37 @@ def render(reports, head, build_id, issues=()):
461487
lines += [
462488
f"**Coverage:** {len(completed)} of {len(LEGS)} environments completed. "
463489
"Advisory result; does not block merging.",
464-
"",
465-
"| Environment | Status |",
466-
"|---|---|",
467490
]
468-
for leg in LEGS:
469-
report = by_leg.get(leg)
470-
status = (
471-
"Completed"
472-
if leg in completed
473-
else f"No result available ({escape(issue_reason(leg, issues))})"
474-
)
475-
lines.append(f"| {environment_name(leg)} | {status} |")
491+
unavailable_legs = [
492+
f"{environment_name(leg)} ({escape(issue_reason(leg, issues))})"
493+
for leg in LEGS
494+
if leg not in completed
495+
]
496+
if unavailable_legs:
497+
lines += ["", "Unavailable: " + "; ".join(unavailable_legs) + "."]
498+
499+
if highlighted:
500+
lines += [
501+
"",
502+
"<details>",
503+
"<summary><b>Measured timings</b></summary>",
504+
"",
505+
"| Environment | Database task | Before | After | Change |",
506+
"|---|---|---:|---:|---:|",
507+
]
508+
for leg, row in highlighted:
509+
lines.append(
510+
f"| {environment_name(leg)} | {TASK_NAMES[row['name']]} | "
511+
f"{row['base_ms']:.3f} ms | {row['candidate_ms']:.3f} ms | "
512+
f"**{row['change_pct']:+.1f}%** |"
513+
)
514+
lines += ["", "</details>"]
476515

477516
diagnostics_start = len(lines)
478517
lines += [
479518
"",
480519
"<details>",
481-
"<summary>Affected phases and call counts</summary>",
520+
"<summary><b>Performance diagnostics</b></summary>",
482521
"",
483522
"Phase times are inclusive diagnostics and must not be added together. "
484523
"They identify where measured time changed, not why it changed.",
@@ -516,7 +555,7 @@ def render(reports, head, build_id, issues=()):
516555
lines += [
517556
"",
518557
"<details>",
519-
"<summary>All database tasks and timings</summary>",
558+
"<summary><b>All database tasks and timings</b></summary>",
520559
]
521560

522561
for leg, (report, rows) in completed.items():
@@ -542,7 +581,7 @@ def render(reports, head, build_id, issues=()):
542581
"</details>",
543582
"",
544583
"<details>",
545-
"<summary>Build, commits and measurement details</summary>",
584+
"<summary><b>Build and measurement details</b></summary>",
546585
"",
547586
]
548587
lines += [
@@ -591,7 +630,7 @@ def render(reports, head, build_id, issues=()):
591630
lines[diagnostics_start:diagnostics_end] = [
592631
"",
593632
"<details>",
594-
"<summary>Affected phases and call counts</summary>",
633+
"<summary><b>Performance diagnostics</b></summary>",
595634
"",
596635
f"{total_diagnostics} diagnostic rows are available in the raw ADO artifacts.",
597636
"",

‎mssql_python/pybind/ddbc_bindings.cpp‎

Lines changed: 15 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
#include "param_detect.hpp"
1313
#include "py_ref.hpp"
1414
#include "py_type_cache.hpp"
15+
#include "fetch_temporal.hpp"
1516
#include "row_factory.hpp"
1617
#include "utf_utils.h"
1718
#include "fetch_text.hpp"
@@ -3766,8 +3767,8 @@ SQLRETURN SQLGetData_wrap(SqlHandlePtr StatementHandle, SQLUSMALLINT colCount, p
37663767
ret = SQLGetData_ptr(hStmt, i, SQL_C_TYPE_DATE, &dateValue, sizeof(dateValue),
37673768
&indicator);
37683769
if (SQL_SUCCEEDED(ret) && indicator != SQL_NULL_DATA) {
3769-
row.append(PyTypeCache::get_date_class_obj()(dateValue.year, dateValue.month,
3770-
dateValue.day));
3770+
row.append(
3771+
FetchTemporal::date(dateValue.year, dateValue.month, dateValue.day));
37713772
} else {
37723773
row.append(py::none());
37733774
}
@@ -3779,7 +3780,7 @@ SQLRETURN SQLGetData_wrap(SqlHandlePtr StatementHandle, SQLUSMALLINT colCount, p
37793780
SQLLEN indicator = 0;
37803781
ret = SQLGetData_ptr(hStmt, i, SQL_C_SS_TIME2, &t2, sizeof(t2), &indicator);
37813782
if (SQL_SUCCEEDED(ret) && indicator != SQL_NULL_DATA) {
3782-
row.append(PyTypeCache::get_time_class_obj()(
3783+
row.append(FetchTemporal::time(
37833784
t2.hour, t2.minute, t2.second, t2.fraction / 1000)); // ns to µs
37843785
} else {
37853786
if (!SQL_SUCCEEDED(ret)) {
@@ -3803,7 +3804,7 @@ SQLRETURN SQLGetData_wrap(SqlHandlePtr StatementHandle, SQLUSMALLINT colCount, p
38033804
break;
38043805
}
38053806
if (SQL_SUCCEEDED(ret)) {
3806-
row.append(PyTypeCache::get_datetime_class_obj()(
3807+
row.append(FetchTemporal::datetime(
38073808
timestampValue.year, timestampValue.month, timestampValue.day,
38083809
timestampValue.hour, timestampValue.minute, timestampValue.second,
38093810
timestampValue.fraction / 1000 // Convert back ns to µs
@@ -4451,32 +4452,23 @@ SQLRETURN FetchBatchData(SQLHSTMT hStmt, ColumnBuffers& buffers, py::list& colum
44514452
case SQL_TYPE_TIMESTAMP:
44524453
case SQL_DATETIME: {
44534454
const SQL_TIMESTAMP_STRUCT& ts = buffers.timestampBuffers[col - 1][i];
4454-
PyObject* datetimeObj = PyTypeCache::get_datetime_class_obj()(
4455-
ts.year, ts.month, ts.day, ts.hour, ts.minute,
4456-
ts.second, ts.fraction / 1000)
4457-
.release()
4458-
.ptr();
4459-
PyList_SET_ITEM(row, col - 1, datetimeObj);
4455+
py::object datetimeObj = FetchTemporal::datetime(
4456+
ts.year, ts.month, ts.day, ts.hour, ts.minute, ts.second,
4457+
ts.fraction / 1000);
4458+
PyList_SET_ITEM(row, col - 1, datetimeObj.release().ptr());
44604459
break;
44614460
}
44624461
case SQL_TYPE_DATE: {
4463-
PyObject* dateObj =
4464-
PyTypeCache::get_date_class_obj()(buffers.dateBuffers[col - 1][i].year,
4465-
buffers.dateBuffers[col - 1][i].month,
4466-
buffers.dateBuffers[col - 1][i].day)
4467-
.release()
4468-
.ptr();
4469-
PyList_SET_ITEM(row, col - 1, dateObj);
4462+
const SQL_DATE_STRUCT& value = buffers.dateBuffers[col - 1][i];
4463+
py::object dateObj = FetchTemporal::date(value.year, value.month, value.day);
4464+
PyList_SET_ITEM(row, col - 1, dateObj.release().ptr());
44704465
break;
44714466
}
44724467
case SQL_SS_TIME2: {
44734468
const SQL_SS_TIME2_STRUCT& t2 = buffers.timeBuffers[col - 1][i];
4474-
PyObject* timeObj =
4475-
PyTypeCache::get_time_class_obj()(t2.hour, t2.minute, t2.second,
4476-
t2.fraction / 1000) // ns to µs
4477-
.release()
4478-
.ptr();
4479-
PyList_SET_ITEM(row, col - 1, timeObj);
4469+
py::object timeObj =
4470+
FetchTemporal::time(t2.hour, t2.minute, t2.second, t2.fraction / 1000);
4471+
PyList_SET_ITEM(row, col - 1, timeObj.release().ptr());
44804472
break;
44814473
}
44824474
case SQL_SS_TIMESTAMPOFFSET: {

0 commit comments

Comments
 (0)